Sort file lists#1068
Conversation
jaraco
left a comment
There was a problem hiding this comment.
Overall, this change is reasonable, though I'd really like to see the functionality encapsulated in a single place that might have a docstring to explain why it's there. The PR would also be substantially better with a test case to capture the expectation (as otherwise this change could get optimized away without any test failures).
Importantly, a change like this needs an entry in the CHANGES.txt file (probably 0.1 bump).
| for base, dirs, files in os.walk(self.bdist_dir): | ||
| for filename in files: | ||
| dirs.sort() | ||
| for filename in sorted(files): |
There was a problem hiding this comment.
Why use sorted for files but .sort for dirs?
| """Walk an unpacked egg's contents, skipping the metadata directory""" | ||
| walker = os.walk(egg_dir) | ||
| base, dirs, files = next(walker) | ||
| dirs.sort() |
There was a problem hiding this comment.
It's starting to feel like this change is getting sprinkled around a few places. Maybe it would make more sense to have a "sorted walk" function or similar.
to generate reproducible zip files that do not differ depending on (random) filesystem order See https://reproducible-builds.org/ for why this matters.
|
Thanks for your nice feedback. I added a sorted_walk function with docstring, but I have not fully tested it
|
|
This new PR is much cleaner. And given that eggs are largely deprecated in favor of wheels, I'm not going to be a stickler about the test. |
to generate reproducible zip files
that do not differ depending on (random) filesystem order
See https://reproducible-builds.org/ for why this matters.