Skip to content

gh-78274: Produce consistent output from marshal.dumps(). - #28379

Closed
ericsnowcurrently wants to merge 8 commits into
python:mainfrom
ericsnowcurrently:marshal-consistent-output
Closed

gh-78274: Produce consistent output from marshal.dumps().#28379
ericsnowcurrently wants to merge 8 commits into
python:mainfrom
ericsnowcurrently:marshal-consistent-output

Conversation

@ericsnowcurrently

@ericsnowcurrentlyericsnowcurrently commented Sep 16, 2021

Copy link
Copy Markdown
Member

The result on debug vs. non-debug builds were slightly different. This PR fixes that by making an additional pass over the object to ensure "refs" are used more than once. Consequently, marshal.dumps() is substantially slower. To mitigate this, I've added to it a keyword-only arg, "stable", to allow folks to opt out.

FYI, I discovered gh-8293 after the fact, which turns out to have a lot of similarities to this PR.

@ericsnowcurrently

Copy link
Copy Markdown
MemberAuthor

@gvanrossum, is something like this what you had in mind?

@gvanrossumgvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, but I'm probably not going to be able to review it before Monday.

Comment threadPython/marshal.c Outdated
Comment threadPython/marshal.c Outdated
@ericsnowcurrently
ericsnowcurrently removed the request for review from tiranSeptember 16, 2021 22:06
@ericsnowcurrentlyericsnowcurrently changed the title bpo-45186: Produce consistent output from marshal.dumps().bpo-34093: Produce consistent output from marshal.dumps().Sep 16, 2021
Comment threadPython/marshal.c
Indicates the data format that dumps should use.
/
*
stable: bool = True

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably make sense to default to False for now, so users would effectively opt-in to the performance penalty. (The same goes for marshal.dump().)

@ericsnowcurrently

ericsnowcurrently commented Sep 20, 2021

Copy link
Copy Markdown
MemberAuthor

As I noted in [bpo-34093](https://bugs.python.org/issue34093#msg402244), we could take a different approach to get the same stable output without having to call w_object() twice. However, this PR would probably be worth landing in the meantime.

@ericsnowcurrently

Copy link
Copy Markdown
MemberAuthor

This PR should probably also add a flag to determine if the import machinery would use marshal.dumps(code, stable=True). The same goes for the compileall module.

@github-actions

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label Oct 21, 2021
@gvanrossum

gvanrossum commented Oct 21, 2021 via email

Copy link
Copy Markdown
Member

@methane

Copy link
Copy Markdown
Member

We need to this to support "reproducible build" in downstream.

@github-actionsgithub-actionsBot removed the stale Stale PR or inactive for long period of time. label Oct 22, 2021

@iritkatrieliritkatriel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has merge conflicts now.

@bedevere-bot

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@gvanrossumgvanrossum changed the title bpo-34093: Produce consistent output from marshal.dumps().gh-78274: Produce consistent output from marshal.dumps().Nov 29, 2022
@encukou

Copy link
Copy Markdown
Member

Closing; see discussion in the issue.

@encukouencukou closed this Mar 26, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@ericsnowcurrently@gvanrossum@methane@bedevere-bot@encukou@iritkatriel@the-knights-who-say-ni@ezio-melotti