Skip to content
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

Metapackage for NetCDF #1120

Merged
merged 7 commits into from
Apr 9, 2025
Merged

Conversation

krystophny
Copy link
Contributor

@krystophny krystophny commented Apr 5, 2025

Apply once #1123 is merged. Docs in fortran-lang/fpm-docs#160

Besides NetCDF support and refactoring the way pkg-config is used from there and HDF5, there are modifications on the way flags are handled. Now all are converted from strings to arrays on the way. This was required because Intel Fortran dies on empty "-I" flags returned by pkg-config. In addition that allowed to avoid duplicate flags - a feature that could later be added more generally (there are a few also generated in other parts of the code, see fpm build --verbose) @perazz if you have time, please check if I use your split function correctly.

@krystophny krystophny changed the title Metapackage for NetCDF Draft: Metapackage for NetCDF Apr 5, 2025
@krystophny krystophny force-pushed the metapackage_netcdf branch 2 times, most recently from 53f90ed to c2fea1d Compare April 5, 2025 20:14
@krystophny krystophny changed the title Draft: Metapackage for NetCDF Metapackage for NetCDF Apr 5, 2025
@krystophny krystophny force-pushed the metapackage_netcdf branch 2 times, most recently from 15d4e0b to 6a61a70 Compare April 6, 2025 11:08
@krystophny krystophny changed the title Metapackage for NetCDF Draft: Metapackage for NetCDF Apr 6, 2025
@krystophny
Copy link
Contributor Author

... on hold for now, the tokenization of the flags still causes some issues.

@perazz
Copy link
Member

perazz commented Apr 7, 2025

tokenization of the flags

@krystophny I see you already tested using shlex. Behavior from Python shlex is the same as fortran-shlex.
Mind to please try using fortran-shlex 1.1.0 where I've implemented token joining?

@krystophny
Copy link
Contributor Author

krystophny commented Apr 7, 2025

@perazz thanks! Yeah, my problem is that it should support

'-I/path/to/include -I /test -I"/path/to/include with spaces" -I "spaces here too" -L/path/to/lib -lmylib'

and split it into 6 parts and keep the spaces between flag and path as well as double quotes (and also single quotes if present)

['-I/path/to/include', '-I /test', '-I"/path/to/include with spaces"', '-I "spaces here too"', '-L/path/to/lib', '-lmylib']

Is this possible with shlex and/or some customization?

@perazz
Copy link
Member

perazz commented Apr 7, 2025

I see. So what we need is an option to retain the quotes from the shell lexer (that are typically removed, see i.e. this.). I guess the best place to do this is from within fortran-shlex

@perazz
Copy link
Member

perazz commented Apr 7, 2025

I have deployed a keep_quotes option in v1.2.0 that should do the job. Please keep me posted.

@krystophny krystophny force-pushed the metapackage_netcdf branch 6 times, most recently from bab9dcf to 01fb971 Compare April 7, 2025 20:35
@krystophny krystophny force-pushed the metapackage_netcdf branch from 01fb971 to 5dd6013 Compare April 7, 2025 21:02
@krystophny krystophny changed the title Draft: Metapackage for NetCDF Metapackage for NetCDF Apr 7, 2025
@krystophny
Copy link
Contributor Author

Alright! A small fix is still needed in fortran-shlex - see perazz/fortran-shlex#6 . In addition, I split this PR now into three, so the order to merge is

  1. Refactor: Move common functionality into fpm_meta_util #1122
  2. Process command line args as arrays #1123 (let me know once you bumped the fortran-shlex version to fix it in fpm.toml)
  3. Metapackage for NetCDF #1120
  4. Metapackage for BLAS #1121

@krystophny
Copy link
Contributor Author

Thanks a lot for the prompt review and merge, @perazz ! Now both, NetCDF and BLAS are finished. For BLAS I go in the search order of cmake: Apple Accelerate -> Intel MKL -> OpenBLAS -> Reference BLAS .

Copy link
Member

@perazz perazz left a comment

Choose a reason for hiding this comment

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

Thank you for this nice addition @krystophny!
I have only removed a couple unused variables from the test program.

LGTM otherwise.

krystophny and others added 2 commits April 9, 2025 09:52
Co-authored-by: Federico Perini <[email protected]>
Co-authored-by: Federico Perini <[email protected]>
@krystophny
Copy link
Contributor Author

Great, thanks @perazz !

@perazz perazz merged commit 290ecc5 into fortran-lang:main Apr 9, 2025
25 checks passed
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