Skip to content

fix: Warn only when all SHAs fail to match a requirement #2922

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stefanpenner
Copy link

@stefanpenner stefanpenner commented May 21, 2025

I believe the missing requirement logging is over reporting when a requirement has multiple SHAS and not all match.

  • This PR addresses that logging.
  • This PR also improves the logging by including the requirement that failed to be satisfied

That being said, It's possible I'm misunderstanding and this warning is working as intended.

@stefanpenner stefanpenner changed the title fix: When a requirement has more then one possible SHA, only warn that if all SHA returns nothing fix: Warn only if all SHAs fail to match a requirement May 21, 2025
@stefanpenner stefanpenner changed the title fix: Warn only if all SHAs fail to match a requirement fix: Warn only when all SHAs fail to match a requirement May 21, 2025
@stefanpenner stefanpenner force-pushed the fix-missing-requirement-warning branch from 7238ba3 to 7eb4977 Compare May 21, 2025 22:05
@@ -334,8 +334,8 @@ def _add_dists(*, requirement, index_urls, logger = None):
sdist = maybe_sdist
continue

if logger:
logger.warn(lambda: "Could not find a whl or an sdist with sha256={}".format(sha256))
if logger and (len(whls) == 0 and sdist == None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

The vars are undefined here. What is that you are trying to achieve?

Copy link
Author

@stefanpenner stefanpenner May 22, 2025

Choose a reason for hiding this comment

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

The vars are undefined here.

@aignas maybe I'm missing something, but whls & sdist appear to be defined at:

https://github.com/bazel-contrib/rules_python/pull/2922/files#diff-cea723aaf48fab4acf923b65c3b4fe6c9bb5dec514cacf7c20fb3f92116151d3R311

Copy link
Author

Choose a reason for hiding this comment

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

Oh sha256 in the log call isn't defined, let me fix that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah sorry, somehow I missed it.

However, the intention of the check though was to tell the users that something is wrong andthat the lock file contains hashes that we could not find on the index.

What would be the use case where such behaviour is not desired?

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