No standard configure constants in public headers#1417
No standard configure constants in public headers#1417fredrik-johansson merged 1 commit intoflintlib:trunkfrom
Conversation
0f3c309 to
c56689f
Compare
c56689f to
733a9fb
Compare
- 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).
|
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 |
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.) |
|
@albinahlback, @fredrik-johansson, what do you think? |
|
I will take a look at this after lunch time! |
fingolfin
left a comment
There was a problem hiding this comment.
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.
Both |
This is an attempt to fix #1390 without hiding the
FLINT_constantsfrom 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
is the right thing to do to include
config.honly when buiding flint.