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

Dependency-level macro setting #952

Merged
merged 9 commits into from
Jul 10, 2023
Merged

Conversation

perazz
Copy link
Contributor

@perazz perazz commented Jul 4, 2023

I'm opening a PR to address #950.

The request is to enable dependencies to have user-specified pre-processor macros.

I suggest to do it via the package manifest, under the [dependencies] section: each dependency can have an optionalpreprocess node with the same contents and structure as the one in the package manifest:

[dependencies]
utils = { path = "crate/utils" , preprocess.cpp.macros=["DEPENDENCY_MACRO"] }

Seems a bit verbose, but I think it's important that the same syntax for preprocessing is used here or in the package manifest (BTW - cpp is the only supported thus far...)

Of course, it is the user's responsibility to ensure that user-enabled macros do not clash with package-level macros, there is no way to check that.

CC @zoziha @awvwgk @certik @arteevraina @henilp105 @minhqdao

@ivan-pi
Copy link
Member

ivan-pi commented Jul 4, 2023

Can this be extended to macro definitions with a token string (no expectations to do it now, but just in terms of the manifest syntax)? For example

! bar.F90
print *, FOO
end
$ gfortran -DFOO=42 bar.F90 
$ ./a.out
          42

Would the name definitions instead of macros be more suitable? It's closer to CMake's add_compile_definitions

@perazz
Copy link
Contributor Author

perazz commented Jul 4, 2023

Yes (untested) that syntax is possible:

preprocess.cpp.macros=["FOO=42", "REAL_PRECISION=real64"]

@perazz
Copy link
Contributor Author

perazz commented Jul 4, 2023

Would the name definitions instead of macros be more suitable? It's closer to CMake's

I think several keywords were possible, including features. However, a name change should then backward apply to preprocess.cpp.macros in the manifest, and that is a non-starter I think.

@zoziha
Copy link
Contributor

zoziha commented Jul 5, 2023

Thank you very much for the solution, @perazz , it looks great and it solves #950 .
The only thing I'm worried about is that preprocess.cpp.macros seems a bit long, but I understand that it's for fpm compatibility, uniformity, so this PR looks good, thanks~

@perazz
Copy link
Contributor Author

perazz commented Jul 5, 2023

Thank you for your comments @zoziha.
I also think preprocess.cpp.macros is a bit long (macros would be enough IMHO).
However, the reason is that in the future we may want to add more dependency-level options, so, I think it's a good idea to keep the same names and maintain backward compatibility.

Copy link
Member

@henilp105 henilp105 left a comment

Choose a reason for hiding this comment

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

Thanks @perazz , Looks good to me.

@perazz
Copy link
Contributor Author

perazz commented Jul 6, 2023

@zoziha I would wait for a few more days, and if there are no further comments, I think this is ready to merge.

@perazz
Copy link
Contributor Author

perazz commented Jul 10, 2023

Merging.

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.

None yet

5 participants