Skip to content

BUG: Disallow shadowed modulenames #25181

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

Merged
merged 8 commits into from
Nov 19, 2023
Merged

BUG: Disallow shadowed modulenames #25181

merged 8 commits into from
Nov 19, 2023

Conversation

HaoZeke
Copy link
Member

@HaoZeke HaoZeke commented Nov 19, 2023

This is a better version of #25114.
Closes #22819. Closes #25182.
Enforces the following:

  • Warns (but doesn't break, for BC) when -m is passed with a .pyf in a -c call, the name is ignored and taken from the .pyf file (the only logical option)
  • Each -c run will only produce one python module, so there can only be one .pyf file
  • Will not write out ambiguous wrapper files when -m is passed without -c and .pyf files are present (new, BUG: f2py generates incorrect wrapper files when passed -m and a .pyf #25182)

For the last point -m blah is internally replaced with -m modname from the .pyf file, so no additional (incorrect) wrappers are produced.

There already is a note in the documentation, but this is a much better solution (and also paves the way for #25111). The documentation could probably use an update, and since this is technically a user facing change it might need a release note.

@HaoZeke HaoZeke mentioned this pull request Nov 19, 2023
7 tasks
@HaoZeke HaoZeke marked this pull request as draft November 19, 2023 01:57
@HaoZeke
Copy link
Member Author

HaoZeke commented Nov 19, 2023

Strangely doesn't fix #25111 (test_return_real) like #25114 did. Draft for now. Should be good to go now.

Comment on lines +453 to +462
# gh-22819 -- begin
parser = make_f2py_parser()
args, comline_list = parser.parse_known_args(comline_list)
pyf_files, _ = filter_files("", "[.]pyf([.]src|)", comline_list)
# Checks that no existing modulename is defined in a pyf file
# TODO: Remove all this when scaninputline is replaced
if "-h" not in comline_list and args.module_name: # Can't check what doesn't exist yet, -h creates the pyf
modname = validate_modulename(pyf_files, args.module_name)
comline_list += ['-m', modname] # needed for the rest of scaninputline
# gh-22819 -- end
Copy link
Member Author

Choose a reason for hiding this comment

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

This is implemented this way for BC reasons. Since there are global variables in f2py2e it makes sense to only incrementally make changes, #24552 details why and #25179 has some more information about a possible cleanup.

@HaoZeke HaoZeke marked this pull request as ready for review November 19, 2023 02:56
@HaoZeke HaoZeke requested a review from mattip November 19, 2023 02:56
@HaoZeke
Copy link
Member Author

HaoZeke commented Nov 19, 2023

@mattip, apart from the issues this closes, the PR is required for #25111, since the command invoked for building the modules used is f2py_opts = ["-c", "-m", module_name] + options + f2py_sources, which is ambiguous in many cases (fixed by this PR). It was actually first written while converting the tests to run under meson but it makes more sense to spin it out here.

distutils used to have some special handling for this case, but for meson usage this PR is needed. As another point in its favor I think it is cleaner to handle this at the f2py level, its a generation issue not a build issue.

@mattip mattip merged commit 18c6157 into numpy:main Nov 19, 2023
@mattip
Copy link
Member

mattip commented Nov 19, 2023

Makes sense, thanks @HaoZeke

@mattip
Copy link
Member

mattip commented Nov 19, 2023

Backport needed if #25123 is to be backported.

@charris
Copy link
Member

charris commented Jan 1, 2024

@HaoZeke The release note snippet for this lacks a heading, same for #25186, which also needs hard line breaks.

@HaoZeke
Copy link
Member Author

HaoZeke commented Jan 4, 2024

@HaoZeke The release note snippet for this lacks a heading, same for #25186, which also needs hard line breaks.

Sorry about that, thanks for the note, won't happen again :)

@HaoZeke HaoZeke restored the newGH22819 branch June 15, 2024 23:40
DimitriPapadopoulos added a commit to DimitriPapadopoulos/numpy that referenced this pull request Apr 17, 2025
Asserting on a non-empty string literal will always pass

This line had been added by numpy#25181 / 0f6e357 without any explanation.
I believe it's an error because the assertion is a no-op. So remove it.
DimitriPapadopoulos added a commit to DimitriPapadopoulos/numpy that referenced this pull request Apr 17, 2025
Asserting on a non-empty string literal will always pass

This line had been added by numpy#25181 / 0f6e357 without any explanation.
I believe it's an error because the assertion is a no-op. So remove it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants