Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Lcov testing #1289
Lcov testing #1289
Changes from all commits
ea48fdd
1d71b20
74f6369
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess tastes vary, but I like
.strip("=")
here more than[:-2]
since it's more explicit what it's doing.And "ascii" as encoding also seems more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this is where we need to decide what to do about md5: it's not available on FIPS-enabled systems. Here are some choices:
I don't know what the effect would be of omitting the hash, so I don't know which to pick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hash is mainly used to ensure the correct lines are marked as covered, to my knowledge. Upstream's
genhtml
, for example, which uses an lcov file and the original source tree to produce an html page with annotated source files, will throw an error if it finds a hash that mismatches: https://github.com/linux-test-project/lcov/blob/master/bin/genhtml#L5308.Genhtml is of course an unlikely usecase with pycoverage, but it's the reference consumer.
It is up to the implementation of the consumer to define what the md5 is used for, but it is marked as optional, so any tools that break on a missing md5 are inherently broken. There shouldn't be any issue with simply omitting the hash when it can't be set, and I can't think of many other uses than "has our user accidentally changed their source". I think that was probably a more common use case around the time lcov added that feature :)
It would be sad if pycoverage produced different outputs depending on the machine it is run on. Maybe hence it's best to remove this functionality in general.
I think a fifth option is to lobby upstream for supporting a more modern hash, as much as that seems unnecessary. It's honestly a bit silly that python mandates thinking about FIPS compliance with no opt-out if you use its md5 hash, and even more silly that some default python distributions seem to enable FIPS compliance (this seems to be a MacOS thing?).
It'll be hard to come up with a backwards-compatible design for that, so I suspect the solution would be to deprecate having hashes at all, since those don't really seem that useful anymore in 2022 when checking for accidental source changes is anyway so easy and often unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the backstory on the hash values.
Is this Python's fault? FIPS forbids md5 as an algorithm.
I haven't encountered this. Can you provide more information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not at all. I just think the situation is a bit silly for backwards-compatibility with older formats that use md5 for legitimate reasons (or even just for migration purposes). It's besides the point, sorry for venting here.
This support page seems to suggest a FIPS-compliance mode is enabled by default. Reading further, it may come down to processor support for the hash, rather than something explicitly disabled on the software side, but this is all ultimately besides the point.
I think I'll spend a bit more time reading up on where exactly the limitation comes from, and then raise an issue on the lcov upstream to see if they're interested enough in allowing lcov to be run in FIPS-compliant environments. No need for coveragepy to be involved in that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TLATER is right here on the use of the hash within LCOV. I am perfectly happy to remove the md5 hash if you would prefer @nedbat.