Skip to content

Update fpm manifest to enforce module naming #121

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
Closed
wants to merge 1 commit into from
Closed

Update fpm manifest to enforce module naming #121

wants to merge 1 commit into from

Conversation

perazz
Copy link

@perazz perazz commented Feb 9, 2023

This commit adds a suitable module-naming convention to the fpm manifest for toml-f. Because toml-f already enforces a valid unique naming (module tomlf_*), there is no need to modify any of the source files. The current commit explicitly adds the naming convention in place to the manifest.

When module names become enforced by default in fpm, this flag will be necessary to build fpm from fpm.

@awvwgk
Copy link
Member

awvwgk commented Feb 9, 2023

Will merging this break all fpm builds with existing fpm versions? So we best have to wait for a new fpm release before applying this change.

@codecov
Copy link

codecov bot commented Feb 9, 2023 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.95%. Comparing base (1c68ea9) to head (2c42dd9).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #121   +/-   ##
=======================================
  Coverage   65.95%   65.95%           
=======================================
  Files          29       29           
  Lines        3169     3169           
  Branches     1350     1350           
=======================================
  Hits         2090     2090           
  Misses        393      393           
  Partials      686      686

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@perazz
Copy link
Author

perazz commented Feb 9, 2023

As far as I can tell, having the [build] structure in fpm.toml have one more key-value pair should not break anything, because it is not even queried in any of the former fpm versions. So as long as it is properly read in by the toml parser, I think it should be safe to add

@perazz perazz mentioned this pull request Feb 9, 2023
Merged
@awvwgk
Copy link
Member

awvwgk commented Feb 9, 2023

As far as I can tell, having the [build] structure in fpm.toml have one more key-value pair should not break anything, because it is not even queried in any of the former fpm versions. So as long as it is properly read in by the toml parser, I think it should be safe to add

That is not true for any version of fpm due to the allowed keyword checks in

https://github.com/fortran-lang/fpm/blob/main/src/fpm/manifest/build.f90#L119

@perazz
Copy link
Author

perazz commented Feb 9, 2023 •
edited
Loading

Wow, this is a pretty severe limitation.

[dependencies]
[dependencies.toml-f]
git = "https://github.com/toml-f/toml-f"
rev = "aee54c5a480d623af99828c76df0447a15ce90dc"

But, the current fpm build points to a given commit. I guess we should look at how far backward this was prescribed.

@awvwgk
Copy link
Member

awvwgk commented Feb 9, 2023

Fpm would be fine with a pinned commit, but merging this patch would break TOML Fortran for everyone else using the latest version with fpm, so this is not acceptable at the moment.

@perazz
Copy link
Author

perazz commented Feb 9, 2023

Yes I agree the major improvements planned on fpm will break many things. Let's leave this PR here for when the time comes.

@certik certik mentioned this pull request Mar 16, 2023
Closed
@perazz @awvwgk
Update fpm manifest to enforce module naming
2c42dd9 
This commit adds a suitable `module-naming` convention to the fpm manifest for toml-f. 
Because toml-f already enforces a valid unique naming (`module tomlf_*`), there is no need to modify any of the source files. 
The current commit explicitly adds the naming convention in place to the manifest.

When module names become enforced by default in fpm, this flag will be necessary to build fpm from fpm.
@awvwgk awvwgk force-pushed the enforce_module_names branch from 273609b to 2c42dd9 Compare May 21, 2023 13:21
@awvwgk awvwgk closed this May 18, 2025
@perazz perazz deleted the enforce_module_names branch May 18, 2025 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@perazz @awvwgk