Skip to content

retry fetching extensions from PyPI with alternative download_filename #4943

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 2 commits into
base: develop
Choose a base branch
from

Conversation

smoors
Copy link
Contributor

@smoors smoors commented Jun 28, 2025

retry by replacing hyphen with underscore in the name: abc-def-%(version).tar.gz -> abc_def-%(version).tar.gz

only done if:

  • it's an extension
  • download from PyPI
  • there's only 1 source url
  • default filename is used (no sources or source_tmpl specified)

at some point we should probably switch to using the underscore in the first try, as that is the recommended name format, see
https://packaging.python.org/en/latest/specifications/source-distribution-format/#source-distribution-file-name

recommendation: always specify the name of the package as shown on PyPI:

  • it ensures EB tries to download the 2 name formats if there are hyphens in the name
  • users are more likely to search for it
  • it standardizes the naming of extensions in Lmod (module spider), avoiding that the same package is available with 2 different names

download_instructions=download_instructions)
download_instructions=download_instructions,
warning_only=warning_only)
if not src_path:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to rename warning_only to something like is_pypi_source and change this to:

Suggested change
if not src_path:
if not src_path and is_pypi_source:

That makes it clear when/why this is retried as the error if the file isn't found and we don't reach this code unless is_pypi_source == True is a bit hidden by src_path = ... (indicating it returns) especially with the (redundant?) check for src_path below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the check for src_path is indeed redundant, but i figured it might be good to keep it as a safety net for future changes to obtain_file, i don't know. happy to remove it if you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 04da499

Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove it as it may lead to confusion here as it basically communicates that None (or empty) would be valid results of obtain_file which they aren't. Doesn't need to be done bow/in this PR though, I've just fallen into this "trap" during the review of this PR and had to check the code of obtain_file to verify.

@Flamefire
Copy link
Contributor

at some point we should probably switch to using the underscore in the first try,

Couldn't there be packages like very_cool-software? And some packages are named foo_bar or on PyPI so we should keep the spelling of PyPI although in general it should by hyphens.

@smoors
Copy link
Contributor Author

smoors commented Jun 30, 2025

at some point we should probably switch to using the underscore in the first try,

Couldn't there be packages like very_cool-software? And some packages are named foo_bar or on PyPI so we should keep the spelling of PyPI although in general it should by hyphens.

yes, we should keep the spelling of PyPI. i meant first trying with underscore for the tarball, as the hyphen should normally only be used to separate the name from the version.

@boegel boegel modified the milestones: 5.1.1, release after 5.1.1 Jul 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants