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

No standard configure constants in public headers #1417

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

mezzarobba
Copy link
Contributor

This is an attempt to fix #1390 without hiding the FLINT_ constants
from users.

This is only taking care of the autotools part; I have no idea if the
cmake stuff needs changes. And I am certainly no autotools expert, so
there are probably things that could/should be improved. In particular,
I don't known if

CPPFLAGS:=@CPPFLAGS@ -DBUILDING_FLINT

is the right thing to do to include config.h only when buiding flint.

- Rename the header managed by autoheader to config.h.
- Only #include config.h from flint.h when building flint itself, not
  when flint.h is sourced by another program.
- Otherwise, include flint-config.h, which now contains only a subset of
  the definitions present in config.h (namely, the FLINT_* constants,
  as opposed to generic stuff like PACKAGE_NAME that would conflict with
  other packages).
@AndreasEnge
Copy link
Contributor

I have just tried it, and it solves the problem.

A drawback is the use of an additional preprocessor constant to diistinguish building flint from building a project using flint, but this has the advantage of minimising the modifications. Alternatively, one could include config.h into each flint source file, which would mean a lot of places to change; or include it only in places where it is needed, which would require a lot of work and present the danger of forgetting places.

Andreas

@mezzarobba
Copy link
Contributor Author

A drawback is the use of an additional preprocessor constant to diistinguish building flint from building a project using flint, but this has the advantage of minimising the modifications.

I agree -- but I thought we could always simplify the architecture later on if this proves a problem. I'm not familiar enough with the flint source code to put the internal includes in exactly the right places. (If Fredrik likes it better to create a new header for internal stuff and include it in every C file, I can do that, of course.)

@mezzarobba mezzarobba marked this pull request as ready for review October 9, 2023 09:49
@mezzarobba
Copy link
Contributor Author

@albinahlback, @fredrik-johansson, what do you think?

@albinahlback
Copy link
Collaborator

I will take a look at this after lunch time!

Copy link
Collaborator

@fingolfin fingolfin 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!

This looks good to me in principle, although I did not try it out locally (but @AndreasEnge did and it worked for him, that's good enough for me :-).

One caveat is that one has to be a bit careful when modifying the build system to ensure things keep working (i.e. when things are added/removed to config.h they may also need to be added/removed to flint-config.h...). Ideally we therefore would have at least one test which does make install and then tests the result. I think the Nemo.jl CI test in this repo does that?

There are more things I could say, but I think this is a very sensible quick solutions, and I agree that any improvements can be done later, if and when someone has the time and energy.

@mezzarobba
Copy link
Contributor Author

One caveat is that one has to be a bit careful when modifying the build system to ensure things keep working (i.e. when things are added/removed to config.h they may also need to be added/removed to flint-config.h...).

Both config.h and flint-config.h are generated files; the only part that needs to be maintained is flint-config.h.in, and it is just a list of the definitions managed by autoconf that are considered public.

@fredrik-johansson fredrik-johansson merged commit 28dcaa1 into flintlib:trunk Oct 11, 2023
13 checks passed
@mezzarobba mezzarobba deleted the config-constants branch October 11, 2023 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redefinition of configure constants
5 participants