Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-34022: Stop forcing of hash-based invalidation with SOURCE_DATE_EPOCH #9607

Merged
merged 2 commits into from Oct 10, 2018

Conversation

Projects
None yet
9 participants
@elprans
Copy link
Contributor

elprans commented Sep 27, 2018

Unconditional forcing of CHECKED_HASH invalidation was introduced in
3.7.0 in bpo-29708. The change is bad, as it unconditionally overrides
invalidation_mode, even if it was passed as an explicit argument to
py_compile.compile() or compileall. An environment variable
should never override an explicit argument to a library function.
That change leads to multiple test failures if the SOURCE_DATE_EPOCH
environment variable is set.

This changes py_compile.compile() to only look at
SOURCE_DATE_EPOCH if no explicit invalidation_mode was specified.
I also made various relevant tests run with explicit control over the
value of SOURCE_DATE_EPOCH.

While looking at this, I noticed that zipimport does not work
with hash-based .pycs at all, though I left the fixes for
subsequent commits.

https://bugs.python.org/issue34022

bpo-34022: Stop forcing of hash-based invalidation with SOURCE_DATE_E…
…POCH

Unconditional forcing of ``CHECKED_HASH`` invalidation was introduced in
3.7.0 in bpo-29708.  The change is bad, as it unconditionally overrides
*invalidation_mode*, even if it was passed as an explicit argument to
``py_compile.compile()`` or ``compileall``.  An environment variable
should *never* override an explicit argument to a library function.
That change leads to multiple test failures if the ``SOURCE_DATE_EPOCH``
environment variable is set.

This changes ``py_compile.compile()`` to only look at
``SOURCE_DATE_EPOCH`` if no explicit *invalidation_mode* was specified.
I also made various relevant tests run with explicit control over the
value of ``SOURCE_DATE_EPOCH``.

While looking at this, I noticed that ``zipimport`` does not work
with hash-based .pycs _at all_, though I left the fixes for
subsequent commits.
.. versionchanged:: 3.7.2
The :envvar:`SOURCE_DATE_EPOCH` environment variable no longer
overrides the value of the *invalidation_mode* argument, and determines
it's default value instead.

This comment has been minimized.

Copy link
@merwok

merwok Sep 27, 2018

Contributor

its

@vstinner vstinner requested a review from benjaminp Sep 28, 2018

@vstinner
Copy link
Member

vstinner left a comment

LGTM, but I have a few minor comments.

I will not approve this PR since it's the work of @benjaminp and I'm not sure that the change respects his design (PEP 552) (I'm not sure that he agrees with the change).

bytecode cache files at runtime.
The default is ``timestamp`` if the :envvar:`SOURCE_DATE_EPOCH` environment
variable is not set, and ``checked-hash`` if the ``SOURCE_DATE_EPOCH``
environment variable is set.

This comment has been minimized.

Copy link
@vstinner

vstinner Sep 28, 2018

Member

"is set": I suggest "is set to a non-empty string". See other existing examples:
https://docs.python.org/dev/using/cmdline.html#envvar-PYTHONOPTIMIZE

Maybe add a reference to the PEP 552 somewhere (you can use syntax :pep:552).

By the way, should we mention SOURCE_DATE_EPOCH somewhere in https://docs.python.org/dev/using/cmdline.html ? The variable is not specific to Python, but it changes its behavior.

enum and controls how the generated bytecode cache is invalidated at
runtime. The default is :attr:`PycInvalidationMode.CHECKED_HASH` if
the :envvar:`SOURCE_DATE_EPOCH` environment variable is set, otherwise
the default is :attr:`PycInvalidationMode.TIMESTAMP`.

This comment has been minimized.

Copy link
@vstinner

vstinner Sep 28, 2018

Member

Maybe add a reference to the PEP 552 somewhere (you can use syntax :pep:552).

This comment has been minimized.

Copy link
@elprans

elprans Sep 28, 2018

Author Contributor

PEP 552 actually has no mention of SOURCE_DATE_EPOCH, so I'm not sure such a reference would be useful here.

This comment has been minimized.

Copy link
@vstinner

vstinner Sep 28, 2018

Member

invalidation_mode is a new thing coming from the PEP 552. Moreover, I proposed to update the PEP:
https://bugs.python.org/issue34022#msg326638

@functools.wraps(fxn)
def wrapper(*args, **kwargs):
with support.EnvironmentVarGuard() as env:
env.unset('SOURCE_DATE_EPOCH')

This comment has been minimized.

Copy link
@vstinner

vstinner Sep 28, 2018

Member

Is there at least one unit test for SOURCE_DATE_EPOCH set to an empty string?

@elprans

This comment has been minimized.

Copy link
Contributor Author

elprans commented Sep 28, 2018

I will not approve this PR since it's the work of @benjaminp and I'm not sure that the change respects his design (PEP 552) (I'm not sure that he agrees with the change).

Benjamin's comment on the issue:

I dislike that having SOURCE_DATE_EPOCH set magically changes command line tools behavior. In my view, this problem should be fixed by reverting ccbe581.

Reverting seems like a drastic measure, I think this PR achieves a better balance.

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Sep 28, 2018

cc @bmwiedemann who authored PR #5200.

@bmwiedemann

This comment has been minimized.

Copy link
Contributor

bmwiedemann commented Sep 28, 2018

I really like this PR as it is what I should have written, if I was a python guru.

We do need some way to get reproducible .pyc files across all of openSUSE's ~1800 python packages without having to touch each of them individually to add an invalidation_mode.
If there is some other preferable approach to achieve that, I'd love to hear about that.

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Oct 10, 2018

Hum, a few tests still fail with SOURCE_DATE_EPOCH=0:

  • test_multiprocessing_main_handling
  • test_cmd_line_script
  • test_runpy

@elprans: Do you want to fix these tests in the same PR, or do you prefer to fix them in a different PR?

@elprans

This comment has been minimized.

Copy link
Contributor Author

elprans commented Oct 10, 2018

@vstinner Separate PR. Those are unrelated to py_compile and are due to broken zipimport.

@vstinner vstinner merged commit a6b3ec5 into python:master Oct 10, 2018

5 checks passed

Azure Pipelines PR #20180927.16 succeeded
Details
bedevere/issue-number Issue number 34022 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Oct 10, 2018

@ned-deily, @benjaminp, @elprans: I guess that this change can wait Python 3.7.2, it's not a blocker issue nor a major bug. So I suggest to wait until 3.7.1 final is released before backporting this change to the 3.7 branch.

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Oct 10, 2018

@vstinner Separate PR. Those are unrelated to py_compile and are due to broken zipimport.

Perfect. In this case, I see no other reason to no merge your PR :-) I merged it, good job! Thanks @elprans.

@ned-deily

This comment has been minimized.

Copy link
Member

ned-deily commented Oct 10, 2018

Unless a release manager says otherwise, please don't wait between RC's and finals to merge PRs. It's better to get them exposed on buildbots. We'll cherry-picked fixes as necessary into the finals.

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Oct 10, 2018

Unless a release manager says otherwise, please don't wait between RC's and finals to merge PRs. It's better to get them exposed on buildbots. We'll cherry-picked fixes as necessary into the finals.

The merged change is a backward incompatible and it mentions the version 3.7.2 in the modified documentation :-)

@ned-deily

This comment has been minimized.

Copy link
Member

ned-deily commented Oct 10, 2018

The docs can be easily changed for the final. The question is how much of an impact is it for 3.7.1 users by not having this change in vs having it in..

yan12125 added a commit to archlinuxcn/repo that referenced this pull request Oct 11, 2018

python-git: fix some tests
Some SOURCE_DATE_EPOCH-related tests are fixed in
python/cpython#9607
@mcepl

This comment has been minimized.

Copy link
Contributor

mcepl commented Oct 15, 2018

@elprans @vstinner Separate PR. Those are unrelated to py_compile and are due to broken zipimport.

Is there such PR somewhere? Or at least a bug?

@elprans

This comment has been minimized.

Copy link
Contributor Author

elprans commented Oct 16, 2018

@mcepl I'm working on a followup PR to fix these.

@mcepl

This comment has been minimized.

Copy link
Contributor

mcepl commented Oct 16, 2018

@elprans thanks

yahya-abou-imran added a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 2018

bpo-34022: Stop forcing of hash-based invalidation with SOURCE_DATE_E…
…POCH (pythonGH-9607)

Unconditional forcing of ``CHECKED_HASH`` invalidation was introduced in
3.7.0 in bpo-29708.  The change is bad, as it unconditionally overrides
*invalidation_mode*, even if it was passed as an explicit argument to
``py_compile.compile()`` or ``compileall``.  An environment variable
should *never* override an explicit argument to a library function.
That change leads to multiple test failures if the ``SOURCE_DATE_EPOCH``
environment variable is set.

This changes ``py_compile.compile()`` to only look at
``SOURCE_DATE_EPOCH`` if no explicit *invalidation_mode* was specified.
I also made various relevant tests run with explicit control over the
value of ``SOURCE_DATE_EPOCH``.

While looking at this, I noticed that ``zipimport`` does not work
with hash-based .pycs _at all_, though I left the fixes for
subsequent commits.
@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Nov 28, 2018

Thanks @elprans for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒🤖

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Nov 28, 2018

GH-10775 is a backport of this pull request to the 3.7 branch.

miss-islington added a commit to miss-islington/cpython that referenced this pull request Nov 28, 2018

bpo-34022: Stop forcing of hash-based invalidation with SOURCE_DATE_E…
…POCH (pythonGH-9607)

Unconditional forcing of ``CHECKED_HASH`` invalidation was introduced in
3.7.0 in bpo-29708.  The change is bad, as it unconditionally overrides
*invalidation_mode*, even if it was passed as an explicit argument to
``py_compile.compile()`` or ``compileall``.  An environment variable
should *never* override an explicit argument to a library function.
That change leads to multiple test failures if the ``SOURCE_DATE_EPOCH``
environment variable is set.

This changes ``py_compile.compile()`` to only look at
``SOURCE_DATE_EPOCH`` if no explicit *invalidation_mode* was specified.
I also made various relevant tests run with explicit control over the
value of ``SOURCE_DATE_EPOCH``.

While looking at this, I noticed that ``zipimport`` does not work
with hash-based .pycs _at all_, though I left the fixes for
subsequent commits.
(cherry picked from commit a6b3ec5)

Co-authored-by: Elvis Pranskevichus <elvis@magic.io>

miss-islington added a commit that referenced this pull request Nov 28, 2018

bpo-34022: Stop forcing of hash-based invalidation with SOURCE_DATE_E…
…POCH (GH-9607)

Unconditional forcing of ``CHECKED_HASH`` invalidation was introduced in
3.7.0 in bpo-29708.  The change is bad, as it unconditionally overrides
*invalidation_mode*, even if it was passed as an explicit argument to
``py_compile.compile()`` or ``compileall``.  An environment variable
should *never* override an explicit argument to a library function.
That change leads to multiple test failures if the ``SOURCE_DATE_EPOCH``
environment variable is set.

This changes ``py_compile.compile()`` to only look at
``SOURCE_DATE_EPOCH`` if no explicit *invalidation_mode* was specified.
I also made various relevant tests run with explicit control over the
value of ``SOURCE_DATE_EPOCH``.

While looking at this, I noticed that ``zipimport`` does not work
with hash-based .pycs _at all_, though I left the fixes for
subsequent commits.
(cherry picked from commit a6b3ec5)

Co-authored-by: Elvis Pranskevichus <elvis@magic.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.