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

Support for SOURCE_DATE_EPOCH (reproducible builds) #187

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

dalcinl
Copy link
Contributor

@dalcinl dalcinl commented Aug 25, 2023

@dalcinl
Copy link
Contributor Author

dalcinl commented Aug 25, 2023

I'm not familiar enough with the codebase to add tests. Any tips would be most appreciated.

@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1c038e8) 94.86% compared to head (493fc5b) 94.93%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
+ Coverage   94.86%   94.93%   +0.06%     
==========================================
  Files          14       14              
  Lines        1111     1125      +14     
==========================================
+ Hits         1054     1068      +14     
  Misses         57       57              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dalcinl
Copy link
Contributor Author

dalcinl commented Aug 27, 2023

coverage will not pass unless I add tests, but I would need directions on the best course of action.
Otherwise, I could rewrite my code in such a way that coverage would pass, but that would be cheating.

@HexDecimal
Copy link
Collaborator

I'm not sure. I guess a test could be added for calling delocate_wheel on a copy of PURE_WHEEL with and without this environment variable set, see test_wheelies.py. Maybe copy the test_fix_pure_python test.

@HexDecimal HexDecimal self-requested a review August 27, 2023 15:06
delocate/tools.py Outdated Show resolved Hide resolved
@HexDecimal
Copy link
Collaborator

The last thing to do would be to add the changes to dir2zip and the support for SOURCE_DATE_EPOCH to the changelog file.

@dalcinl
Copy link
Contributor Author

dalcinl commented Aug 27, 2023

I should mention that native macOS tools (e.g. ld64) do not really support SOURCE_DATE_EPOCH. They do support ZERO_AR_DATE, though. The behavior is different, they just zero timestamps (or set them -1, don't remember exactly).
Perhaps it would be nice to assume that ZERO_AR_DATE means the user has an intention to get reproducible binaries including the outer wheel file. With the code I put in place, the implementation would be a couple extra lines. I'll leave this nit to the consideration of the maintainers.

@dalcinl
Copy link
Contributor Author

dalcinl commented Aug 27, 2023

The last thing to do would be to add the changes to dir2zip

Do you mean to add to the changelog that dir2zip now has an extra optional keyword-only argument? Or something else? IMHO, it is a quite trivial and inconsequential addition not worth of a changelog entry. Last time dir2zip was modified, new kw-only args were added, and yet no entry in the changelog for such change.

@HexDecimal
Copy link
Collaborator

Do you mean to add to the changelog that dir2zip now has an extra optional keyword-only argument?

I did mean that, but it's vague how much changes in public functions should be compared the actual features and CLI arguments. Personally, I'd rather consider all but the most public facing tools to be private, but you can never be sure unless you've added that leading underscore to everything ahead of time. I'm not sure there's consensus on this at the moment.



@contextmanager
def Environment(**env):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't name this like a class, add annotations, and a docstring, maybe also make it private.

Suggested change
def Environment(**env):
def _scope_env(**env: str) -> Iterator[None]:
"""Add the provided environment variables for the duration of this context."""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't name this like a class,

Done. I named the new context manager like a class precisely because of the other context manager function in the same file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed the rename suggestion. I'll push a fix.

@dalcinl
Copy link
Contributor Author

dalcinl commented Aug 27, 2023

but it's vague how much changes in public functions

FWIW, a new kw-only argument (in a function without **kwargs) has absolutely zero backward compatibility implications.

Personally, I'd rather consider all but the most public facing tools to be private

That's what I assumed for this particular project, and why I did not care in my original sumision about making private the new utility function I added. This is a command line tool. A note in README should be enough to make the point clear.

@HexDecimal HexDecimal merged commit f639c4d into matthew-brett:master Nov 14, 2023
17 checks passed
@dalcinl dalcinl deleted the source-date-epoch branch May 15, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants