matthew-brett / delocate

Find and copy needed dynamic libraries into python wheels
BSD 2-Clause "Simplified" License
267 stars 59 forks source link

Support for SOURCE_DATE_EPOCH (reproducible builds) #187

Closed dalcinl closed 1 year ago

dalcinl commented 1 year ago

https://reproducible-builds.org/docs/source-date-epoch/

dalcinl commented 1 year ago

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

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dalcinl commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

dalcinl commented 1 year ago

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.