8000 BUG: Replace dots with underscores in f2py meson backend for lib identifiers by mathomp4 · Pull Request #26634 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG: Replace dots with underscores in f2py meson backend for lib identifiers #26634

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 9, 2024
Merged

BUG: Replace dots with underscores in f2py meson backend for lib identifiers #26634

merged 2 commits into from
Jun 9, 2024

Conversation

mathomp4
Copy link
Contributor
@mathomp4 mathomp4 commented Jun 6, 2024

As seen in #26623, if a library itself has a dot, e.g., -lMAPL.base, then the meson backend produces code like:

MAPL.base = declare_dependency(link_args : ['-lMAPL.base'])

But meson apparently sees MAPL.base as saying this is the base method in the MAPL type (or class?). Thus, it expects parentheses.

So this PR uses a replace() call to replace all the . to _ in the lib identifier. This has to be done both in the declare_dependency and in the dependencies bit of py.extension_module.

With this fix I get:

MAPL_constants = declare_dependency(link_args : ['-lMAPL.constants'])
MAPL_base = declare_dependency(link_args : ['-lMAPL.base'])

This seems to fix any issues in my testing, but my fear is cases I don't have. (Though perhaps libraries with dots is a rare case anyway and mainly a style used in the our code.)

Closes #26623

NOTE: I tested this with Numpy 1.26.4, and I see main is now in v2 land. It would be nice to get this into a v1 tag as well if v2 is still a ways off. I could backport this to maintenance/1.26.x if desired.

@mathomp4
Copy link
Contributor Author
mathomp4 commented Jun 6, 2024

Huh. I think I might be hitting the issue seen by @luxedo in #26633 as I can't see how my change could break this test...

@luxedo
Copy link
Contributor
luxedo commented Jun 6, 2024

Yes! Cirlce CI has reported a false positive from a previous merge. Merging #26633 should fix this issue.

@mattip mattip changed the title BUG: Replace dots with underscores in meson backend for lib identifiers BUG: Replace dots with underscores in f2py meson backend for lib identifiers Jun 6, 2024
@mathomp4
Copy link
Contributor Author
mathomp4 commented Jun 6, 2024

Also for the admins: should I make changes to the "numpy with meson" docs for this PR as well? It seems a bit "internal" since it doesn't really change anything in how f2py is called so probably not but let me know.

@ngoldbaum
Copy link
Member

Also for the admins: should I make changes to the "numpy with meson" docs for this PR as well?

If there's no user-facing impact I don't think there's any need to mention it in the docs. I'm not familiar with f2py so can't help otherwise with the review but thought 8000 I'd add that bit of context.

@mathomp4
Copy link
Contributor Author
mathomp4 commented Jun 7, 2024

I can report that the end user of this code said things worked for her with the fix, so that's nice.

HaoZeke
HaoZeke requested changes Jun 8, 2024
View reviewed changes
Copy link
Member
@HaoZeke HaoZeke left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is a pragmatic change, thanks for the fix @mathomp4 but since we don't have a nice way to have a concrete test for this, maybe a comment pointing to the issue / pr?

@mathomp4
Copy link
Contributor Author
mathomp4 commented Jun 9, 2024

This is a pragmatic change, thanks for the fix @mathomp4 but since we don't have a nice way to have a concrete test for this, maybe a comment pointing to the issue / pr?

Well this: #26623 does seem to show the issue. But even simpler, take this code as test.F90:

module test
   implicit none
   integer :: i = 0
end module test

now try and run f2py on it assuming it needs to link to library libfoo.bar.so:

$ f2py -m test_ -lfoo.bar -c test.F90
Cannot use distutils backend with Python>=3.12, using meson backend instead.
Using meson backend
Will pass --lower to f2py
See https://numpy.org/doc/stable/f2py/buildtools/meson.html
Reading fortran codes...
	Reading file 'test.F90' (format:free)
Post-processing...
	Block: test_
			Block: test
Applying post-processing hooks...
  character_backward_compatibility_hook
Post-processing (stage 2)...
	Block: test_
		Block: unknown_interface
			Block: test
Building modules...
    Building module "test_"...
		Constructing F90 module support for "test"...
		  Variables: i
    Wrote C/API module "test_" to file "./test_module.c"
    Fortran 90 wrappers are saved to "./test_-f2pywrappers2.f90"
The Meson build system
Version: 1.4.1
Source dir: /gpfsm/dnb34/tdirs/login/discover33.240116.mathomp4/tmpti7gdlfy
Build dir: /gpfsm/dnb34/tdirs/login/discover33.240116.mathomp4/tmpti7gdlfy/bbdir
Build type: native build

meson.build:35:8: ERROR: Expecting lparen got assign.
foo.bar = declare_dependency(link_args : ['-lfoo.bar'])
        ^

A full log can be found at /gpfsm/dnb34/tdirs/login/discover33.240116.mathomp4/tmpti7gdlfy/bbdir/meson-logs/meson-log.txt
Traceback (most recent call last):
  File "/usr/local/other/GEOSpyD/24.4.0-0_py3.12/2024-05-31/bin/f2py", line 10, in <module>
    sys.exit(main())
             ^^^^^^
  File "/usr/local/other/GEOSpyD/24.4.0-0_py3.12/2024-05-31/lib/python3.12/site-packages/numpy/f2py/f2py2e.py", line 766, in main
    run_compile()
  File "/usr/local/other/GEOSpyD/24.4.0-0_py3.12/2024-05-31/lib/python3.12/site-packages/numpy/f2py/f2py2e.py", line 738, in run_compile
    builder.compile()
  File "/usr/local/other/GEOSpyD/24.4.0-0_py3.12/2024-05-31/lib/python3.12/site-packages/numpy/f2py/_backends/_meson.py", line 178, in compile
    self.run_meson(self.build_dir)
  File "/usr/local/other/GEOSpyD/24.4.0-0_py3.12/2024-05-31/lib/python3.12/site-packages/numpy/f2py/_backends/_meson.py", line 171, in run_meson
    self._run_subprocess_command(setup_command, build_dir)
  File "/usr/local/other/GEOSpyD/24.4.0-0_py3.12/2024-05-31/lib/python3.12/site-packages/numpy/f2py/_backends/_meson.py", line 167, in _run_subprocess_command
    subprocess.run(command, cwd=cwd, check=True)
  File "/usr/local/other/GEOSpyD/24.4.0-0_py3.12/2024-05-31/lib/python3.12/subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['meson', 'setup', 'bbdir']' returned non-zero exit status 1.

It's not like f2py is failing because it can't find the library. It's failing because it's producing illegal meson.

From my reading of the Meson Grammar, a dot in a variable means a method expression:

method_expression: postfix_expression "." function_expression

so:

foo.bar = ...

dies because meson is expecting bar to be a function and per the Grammar:

function_expression: id_expression "(" [argument_list] ")"

that means the next character must be a left paren.

@HaoZeke @mathomp4
TST: Add one for numpygh-26623
a48ce39 
On creating incorrect meson.build files

Co-authored-by: mathomp4 <mathomp4@users.noreply.github.com>
@HaoZeke HaoZeke force-pushed the bugfix/mathomp4/26623-fix-meson-lib branch from f52f57c to a48ce39 Compare June 9, 2024 19:22
HaoZeke
HaoZeke approved these changes Jun 9, 2024
View reviewed changes
Copy link
Member
@HaoZeke HaoZeke left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @mathomp4 , LGTM, will merge after the CI run finishes.

@HaoZeke HaoZeke merged commit 5aaede6 into numpy:main Jun 9, 2024
67 of 68 checks passed
@HaoZeke
Copy link
Member
HaoZeke commented Jun 10, 2024

@charris as a regression (from distutils to meson) this might need a backport.

@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Jun 10, 2024
@charris charris mentioned this pull request Jun 10, 2024
Merged
@charris charris removed 09 - Backport-Candidate PRs tagged should be backported labels Jun 10, 2024
This was referenced Jun 16, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HaoZeke HaoZeke HaoZeke approved these changes

Assignees
No one assigned
Labels
00 - Bug component: numpy.f2py
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: Bad meson code generated by f2py with dotted library
6 participants
@mathomp4 @luxedo @ngoldbaum @HaoZeke @charris @mattip
0