Add SHA256 hash of .whl as info output #5908

ianw · 2018-10-22T04:52:22Z

Currently I'm trying to debug some issues with what appear to be
corrupt wheels. It would be very useful to see what pip thought the
state of things was as it wrote the wheel output; if a final corrupt
distributed file is then different to what pip has saved in its build
logs, you know the problem is somewhere after pip but before
distribution.

Currently we get a log of the initial creation, then the stamp when it
gets moved in the final output location, e.g.:

 creating '/tmp/pip-wheel-71CpBe/foo-1.2.3-py2.py3-none-any.whl
 ...
 Stored in directory: /opt/wheel/workspace

A lot happens in between this, so my suggestion is we add the final
output file and it's hash before the "Stored in directory:", e.g. you
now see:

 Building wheels for collected packages: simple
   Running setup.py bdist_wheel for simple: started
   Running setup.py bdist_wheel for simple: finished with status 'done'
   Finished: simple-3.0-py3-none-any.whl sha256=39005a57a6327972575072af82e11d0817439fe6a069381f6f2a123a8c0bf1cf
   Stored in directory: /tmp/pytest-of-iwienand/pytest-18/test_pip_wheel_success0/workspace/scratch
 Successfully built simple

Despite the hash being fairly important for things like
--require-hashes, AFAICS the final hash is not put in the logs at all
currently, so I think this is generically helpful.

BrownTruck · 2018-12-16T15:45:07Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

BrownTruck · 2019-01-25T00:15:12Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

BrownTruck · 2019-02-05T11:30:47Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

Currently I'm trying to debug some issues with what appear to be corrupt wheels. It would be very useful to see what pip thought the state of things was as it wrote the wheel output; if a final corrupt distributed file is then different to what pip has saved in its build logs, you know the problem is somewhere after pip but before distribution. Currently we get a log of the initial creation, then the stamp when it gets moved in the final output location, e.g.: creating '/tmp/pip-wheel-71CpBe/foo-1.2.3-py2.py3-none-any.whl ... Stored in directory: /opt/wheel/workspace A lot happens in between this, so my suggestion is we add the final output file and it's hash before the "Stored in directory:", e.g. you now see: Building wheels for collected packages: simple Running setup.py bdist_wheel for simple: started Running setup.py bdist_wheel for simple: finished with status 'done' Finished: simple-3.0-py3-none-any.whl sha256=39005a57a6327972575072af82e11d0817439fe6a069381f6f2a123a8c0bf1cf Stored in directory: /tmp/pytest-of-iwienand/pytest-18/test_pip_wheel_success0/workspace/scratch Successfully built simple Despite the hash being fairly important for things like --require-hashes, AFAICS the final hash is not put in the logs at all currently, so I think this is generically helpful.

ianw · 2019-05-06T08:04:55Z

Any chance I could get a review of this? Either way? We keep a local copy of pip going to have this in it, which I would certainly rather dispense with if possible.

cjerdonek

Thanks for this PR, and thanks for your great patience. It would be great to get this into the next release (next month). I'll add it to the 19.2 milestone now.

cjerdonek · 2019-06-22T16:23:07Z

news/5908.feature

@@ -0,0 +1,2 @@
+Wheel builds will output the final filename and SHA256 hash of .whl


You should use the imperative tone (see https://pip.pypa.io/en/stable/development/contributing/#news-entries ). Maybe something like this?

Log the final filename and SHA256 hash of a .whl file when done building a wheel.

I have addressed this with 09e3184

src/pip/_internal/wheel.py

cjerdonek · 2019-06-22T16:29:41Z

src/pip/_internal/wheel.py

@@ -885,7 +892,10 @@ def _build_one_inside_env(self, req, output_dir, python_tag=None):
                wheel_name = os.path.basename(wheel_path)
                dest_path = os.path.join(output_dir, wheel_name)
                try:
+                    wheel_hash, _ = hash_file(wheel_path)


I think it would be helpful for readers of the code to give a name to the second variable in the return value. You could add a code comment above the line saying, "We don't need to use the second value." if you would like to communicate that.

_length should do, or also output the file size :)

I have put the length into the output string with a8e382b, so this value is now used

cjerdonek · 2019-06-22T16:33:52Z

src/pip/_internal/wheel.py

                    shutil.move(wheel_path, dest_path)
+                    logger.info('Finished: %s sha256=%s',
+                                wheel_name, wheel_hash.hexdigest())


The other log messages in your post have the form "Running setup.py bdist_wheel for simple: finished ..." So maybe this could be "Finished wheel for <name>: ..."

Also, this info() message can be merged with the one right after. Maybe the directory path can go on a separate line like it is now.

I agree on the format and have changed that with a8e382b

I did leave the following info message (Stored in directory:) somewhat on purpose, because I felt like it might be the type of thing that people had greps for in various ways in existing code. I have made this point explicit with a comment in a8e382b, and additionally added a match for it to the functional test.

The line will become quite long with the full path too; however if you feel strongly the backwards compatibility issues aren't a concern I can change it.

I guess I don't feel strongly about merging the log messages. However, I'd rather not create any expectation among users (or future maintainers for that matter) that log messages won't or shouldn't change. One reason is that I think we should be open to improving these messages when we can as a matter of UX. Thus, my preference would be for the comment not to be there.

The line will become quite long with the full path too;

By the way, to clarify, my suggestion was for the second log message to be in the same log message as the first but still be on a separate line (i.e. a newline character would go in the message). Thus, the lengths of lines needn't increase, and in fact the same greps could even work if the text was kept the same.

Thus, my preference would be for the comment not to be there.

Removed with 45a6415

By the way, to clarify, my suggestion was for the second log message to be in the same log message as the first but still be on a separate line (i.e. a newline character would go in the message).

Ahh ... yes I wasn't sure about multi-line log strings. This probably comes from some ancient history of mine where I was dealing with syslog() and /dev/msg a lot where multi-line strings are not handled well at all :) Again if you feel strongly happy to convert it to a suggested format

xavfernandez · 2019-06-24T19:45:56Z

src/pip/_internal/wheel.py

    """Return (hash, length) for path using hashlib.sha256()"""
    h = hashlib.sha256()
    length = 0
    with open(path, 'rb') as f:
        for block in read_chunks(f, size=blocksize):
            length += len(block)
            h.update(block)
+    return (h, str(length))  # type: ignore


This could return the length as int since rehash uses str(length) and _build_one_inside_env ignores it

Good observation. For that matter (but not for this PR), rehash() should probably also be made to return an int! (And the caller can convert to a string as needed.)

I have made hash_file return a int with 53c38d8. I will leave rehash() for now to avoid further confusion :)

xavfernandez · 2019-06-24T19:47:57Z

src/pip/_internal/wheel.py

@@ -885,7 +892,10 @@ def _build_one_inside_env(self, req, output_dir, python_tag=None):
                wheel_name = os.path.basename(wheel_path)
                dest_path = os.path.join(output_dir, wheel_name)
                try:
+                    wheel_hash, _ = hash_file(wheel_path)


_length should do, or also output the file size :)

ianw

Thank you for review; I'm not sure if you prefer a squashed change or multiple commits, I'm happy to do either.

ianw · 2019-06-25T08:59:33Z

src/pip/_internal/wheel.py

                    shutil.move(wheel_path, dest_path)
+                    logger.info('Finished: %s sha256=%s',
+                                wheel_name, wheel_hash.hexdigest())


I agree on the format and have changed that with a8e382b

I did leave the following info message (Stored in directory:) somewhat on purpose, because I felt like it might be the type of thing that people had greps for in various ways in existing code. I have made this point explicit with a comment in a8e382b, and additionally added a match for it to the functional test.

The line will become quite long with the full path too; however if you feel strongly the backwards compatibility issues aren't a concern I can change it.

cjerdonek · 2019-06-25T09:10:30Z

Thank you for review; I'm not sure if you prefer a squashed change or multiple commits, I'm happy to do either.

Thanks! Either way is fine. We tend to squash when committing anyways using the button (unless the proposer has made deliberately clean and crafted commits, which usually requires rebasing after addressing review comments).

Returning an int for the length makes for a better interface for this new function.

This rewords the output to be more like the form of the preceeding messages. Additionally the size is added, since we have calculated it anyway. The output will now look like: Collecting simple==3.0 Building wheels for collected packages: simple Building wheel for simple (setup.py): started Building wheel for simple (setup.py): finished with status 'done' Created wheel for simple: filename=simple-3.0-py3-none-any.whl size=1138 sha256=2a980a802c9d38a24d29aded2dc2df2b080e58370902e5fdf950090ff67aec10 Stored in directory: /tmp/pytest-of-iwienand/pytest-0/test_pip_wheel_success0/workspace/scratch Successfully built simple A match for the following line is added to the functional test too.

cjerdonek · 2019-06-26T09:46:34Z

Thanks again, @ianw! 🎉

ianw force-pushed the path-and-hash branch 3 times, most recently from bb8f03d to 8be73da Compare October 23, 2018 00:25

ianw changed the title ~~Add more info to built wheel output~~ Add SHA256 hash of .whl as info output Oct 23, 2018

ianw force-pushed the path-and-hash branch from 8be73da to 247f80d Compare October 23, 2018 06:29

cjerdonek added type: enhancement Improvements to functionality C: wheel The wheel format and 'pip wheel' command C: logging Information Logging labels Oct 24, 2018

cjerdonek added this to the Print Better Error Messages milestone Oct 24, 2018

BrownTruck added the needs rebase or merge PR has conflicts with current master label Dec 16, 2018

ianw force-pushed the path-and-hash branch from 247f80d to 929a6b8 Compare December 17, 2018 04:53

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Dec 17, 2018

ianw force-pushed the path-and-hash branch from 929a6b8 to 8d0c708 Compare December 17, 2018 05:42

BrownTruck added the needs rebase or merge PR has conflicts with current master label Jan 25, 2019

ianw force-pushed the path-and-hash branch from 8d0c708 to 45a0faa Compare February 4, 2019 01:20

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Feb 4, 2019

BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 5, 2019

ianw force-pushed the path-and-hash branch from 45a0faa to b995b1e Compare February 7, 2019 00:10

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Feb 7, 2019

ianw force-pushed the path-and-hash branch from b995b1e to 4e8b6e9 Compare May 6, 2019 05:19

cjerdonek suggested changes Jun 22, 2019

View reviewed changes

cjerdonek modified the milestones: Print Better Error Messages, 19.2 Jun 22, 2019

xavfernandez reviewed Jun 24, 2019

View reviewed changes

Use imperative tone for log message

09e3184

ianw commented Jun 25, 2019

View reviewed changes

Add unit tests for wheel.hash_file and wheel.rehash

3c68b84

ianw force-pushed the path-and-hash branch from a8e382b to 8cfd581 Compare June 25, 2019 09:08

Make hash_file return a int length

2639514

Returning an int for the length makes for a better interface for this new function.

ianw force-pushed the path-and-hash branch 2 times, most recently from 81762b3 to 45a6415 Compare June 25, 2019 10:56

ianw force-pushed the path-and-hash branch from 45a6415 to 8d4e17c Compare June 25, 2019 11:03

xavfernandez approved these changes Jun 25, 2019

View reviewed changes

cjerdonek approved these changes Jun 26, 2019

View reviewed changes

cjerdonek merged commit 0dbab23 into pypa:master Jun 26, 2019

lock bot added the auto-locked Outdated issues that have been locked by automation label Jul 26, 2019

lock bot locked as resolved and limited conversation to collaborators Jul 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SHA256 hash of .whl as info output #5908

Add SHA256 hash of .whl as info output #5908

ianw commented Oct 22, 2018 •

edited

Loading

BrownTruck commented Dec 16, 2018

BrownTruck commented Jan 25, 2019

BrownTruck commented Feb 5, 2019

ianw commented May 6, 2019

cjerdonek left a comment

cjerdonek Jun 22, 2019

ianw Jun 25, 2019

cjerdonek Jun 22, 2019

xavfernandez Jun 24, 2019

ianw Jun 25, 2019

cjerdonek Jun 22, 2019

cjerdonek Jun 22, 2019

ianw Jun 25, 2019

cjerdonek Jun 25, 2019

cjerdonek Jun 25, 2019

ianw Jun 25, 2019

xavfernandez Jun 24, 2019

cjerdonek Jun 24, 2019

ianw Jun 25, 2019

xavfernandez Jun 24, 2019

ianw left a comment

ianw Jun 25, 2019

cjerdonek commented Jun 25, 2019

cjerdonek commented Jun 26, 2019

		@@ -0,0 +1,2 @@
		Wheel builds will output the final filename and SHA256 hash of .whl

Add SHA256 hash of .whl as info output #5908

Add SHA256 hash of .whl as info output #5908

Conversation

ianw commented Oct 22, 2018 • edited Loading

BrownTruck commented Dec 16, 2018

BrownTruck commented Jan 25, 2019

BrownTruck commented Feb 5, 2019

ianw commented May 6, 2019

cjerdonek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjerdonek commented Jun 25, 2019

cjerdonek commented Jun 26, 2019

ianw commented Oct 22, 2018 •

edited

Loading