Project

General

Profile

Bug #1038

CFLAGS are not updated when ACCLERATION is changed

Added by Roland Schulz over 6 years ago. Updated over 6 years ago.

Status:
Closed
Priority:
High
Assignee:
Category:
build system
Target version:
Affected version - extra info:
Affected version:
Difficulty:
uncategorized
Close

Description

cmake -DGMX_CPU_ACCELERATION=sse2 ..
cmake -DGMX_CPU_ACCELERATION=avx_256 ..

results in compiler errors, because the acceleration level is changed but the CFLAGS aren't updated. I'm not sure what the best way to fix this is. We can't get rid of GROMACS_C_FLAGS_SET because otherwise the flags are appended every time cmake is run the flags are appended again.

One question is whether only the change should only effect the acceleration flag or everything added to GROMACS_C_FLAGS.
If only changes to the acceleration flag should affect the flags, the acceleration flags should be stored in their own variable (e.g. ACCELERATION_C_FLAGS).

Options to set either GROMACS_C_FLAGS/ACCELERATION_C_FLAGS so that changes get reflected but duplicates don't occur:
  • Appending without duplicates using
        string(REPLACE " " ";" CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${GROMACS_C_FLAGS}")
        list(REMOVE_DUPLICATES CMAKE_C_FLAGS)
        string(REPLACE ";" " " CMAKE_C_FLAGS "${CMAKE_C_FLAGS}")
    
  • Instead of adding it to CMAKE_C_FLAGS use add_definitions
  • Or instead use COMPILE_FLAGS

Another question is whether their show be a way for the user to overwrite the GROMACS_C_FLAGS/ACCELERATION_C_FLAGS. If so we should use something like:

if(NOT DEFINED ACCELERATION_C_FLAGS)
    set(ACCELERATION_C_FLAGS SUGGESTED_ACCELERATION_C_FLAGS)
endif()


Related issues

Related to GROMACS - Bug #1040: inconsistency in passing CMAKE_C[XX]_FLAGS on the command lineClosed

Associated revisions

Revision e6541abb (diff)
Added by Mark Abraham over 6 years ago

Reduce use of monolithic sets of compiler flags

This prepares to fix the Redmine issue below (but perhaps could
be folded into it).

Refs #1038

Change-Id: I6407dbbd331e58c30ab1b275748a211040c9c414

Revision 446d5dab (diff)
Added by Roland Schulz over 6 years ago

Handle compiler flags more flexibly

Flags aren't cached anymore. This prevents all problems with
caching dependent variables. It is the approached taken by other
projectes (VTK,libssh,...). The cached CMAKE_C*_FLAGS* only contain
the user provided flags. The user can add flags by setting the
variables directly or by setting environment variables.
All flags added by Gromacs are only added to the uncached version
with the same name. For the user to overwrite flags the new option
GMX_SKIP_DEFAULT_CFLAGS is added. This option gives the user total
control. Gromacs doesn't add any flags automatically and instead
shows which flags would be added. The user can manually add those
flags she wants.

Fixes #1038, #1040.

Change-Id: I8655f93fac60bce2c9a35152937d672b3786ff32

History

#1 Updated by Roland Schulz over 6 years ago

Another question is do we need to have different flags for C++ and C compilers. Thus whether it is important to check separate what flags are required for C/C++. Because add_defintions and COMPILER_FLAGS isn't language specific, so these solutions problably wouldn't work if we want to keep checking C/C++ separately.

#2 Updated by Szilárd Páll over 6 years ago

  • Priority changed from Normal to High

I took the liberty of bumping the priority of this issue and set the target version to 4.6. Even if the issue does not get fully solved by 4.6, this will certainly cause serious annoyance, so it should be either fixed properly or documented and prevented e.g. by setting an internal variable that replicates the value of GMX_CPU_ACCELERATION gets checked at every cmake invocation and based on this a warning can be issued.

#3 Updated by Teemu Murtola over 6 years ago

Things could be simpler if we would try to avoid caching dependent variables. So in this case, not cache the final CMAKE_C_FLAGS, but just add GROMACS_C_FLAGS to CMAKE_C_FLAGS without changing the value of CMAKE_C_FLAGS in the cache. And to make this as simple as possible, also have a separate cached variable for the result of each test that may need to vary independently.

The only issue I see with this is that there will not be a single cache variable that would immediately show the flags used. Such a variable may be useful for debugging user issues if they upload the CMakeCache.txt, but can be added separately if it is not straightforward to deduce the set of flags from a few other cache variables.

#4 Updated by Roland Schulz over 6 years ago

I agree with Teemu. Utilizing the fact that non-cache variables overwrite cache variables could be good idea here. At least if it is clearly documented in the variable cache name, so that it is obviously to the user what does on. This avoid the problem that C/C++ options can be set differently, and thus is better than the options I proposed before.

Another option would be to set it as a cache variable, but ignore that cache variable on the next execution unless it is modified by the user. Thus it would be build every time from the dependent flags as suggested by Teemu. The difference to Teemu's suggestion is that the user could modify it freely (e.g. remove flags) and we still have a flag which contains all options (for debugging). If it is modified by the user, than the flags should not be changed at all automatically (not even accelerations flags) and the user should get exactly what he specified (and is responsible - should be documented: e.g. "Don't change unless you know what you are doing").
On the first invocation of cmake we could detect whether it was modified by comparing e.g. CMAKE_C_FLAGS to CMAKE_C_INIT_FLAGS (one test for each of all the flags CMAKE_(C|CXX)(_|_DEBUG_|_RELEASE)_FLAGS flags). If it is identical then it wasn't set by the user. For subsequent invocation we could add a hidden GMX_CMAKE_C_FLAGS variable which correspond to the last automatically set value of CMAKE_C_FLAGS. If they agree the user didn't change it. This would also solve the inconsistency pointed out in #1040.

#5 Updated by Teemu Murtola over 6 years ago

I agree that my suggestion needs some work if/when we want to provide the user a possibility to override the flags. I left that out on purpose, partly to keep things simple and partly because I did not have time to think it through, as there are quite a few different options. Your suggestion is one alternative, as is having separate cache variables for the override (e.g., GMX_C_FLAGS, which, if set, would set the exact set of compiler flags). I'm sure there are other options as well; I think here it would be best to consider what is really the functionality we want to offer to the user (and whether using the CMAKE_C_FLAGS* for override purpose is clear or confusing), and then look at how that is best implemented.

#6 Updated by Mark Abraham over 6 years ago

I think the problem is that the design of CMakeLists.txt tries to be autotools and construct a monolithic set of persistent CFLAGS, and respond to environment variables. That's intrinsically broken for a multi-pass configuration system.

IMO the design needs to be such that the user should not want to override the CFLAGS. As soon as we allow for overriding then the whole system is unworkable because nothing about any state of anything is known. I'm skeptical that there should be the option to append flags. There's existing infrastructure for Release and Debug builds. That's where compiler flags should be tweaked (even in a User build type), not on the command line or environment. Libraries should be searched for from paths, not imposed by the user. Normal usage shouldn't need it; a user adding a tool with a new dependency needs to follow our (good) example for how to do that.

Instead, one should detect what flags are required for some feature, store them in an cache variable and use that only where required and do not cache any combined variable. Now the problem of setting and re-setting reduces to managing the behaviour of a FindXXX module on a case-by-case basis. It's only when you append to variables and write the result to the cache that re-runs can keep spamming variables.

There are definitely GROMACS compiler settings that should be common across all source files (optimization flags, warning suppression, acceleration flags, probably not much else), and the CMake methodology for that is to use CMAKE_C_FLAGS or its relatives. If the user set something that's already cached. We can prepend or append to that to give the user the chance to specify things (but again I don't think we should plan for this to happen much). That happens at run time (and so is effective for managing the final generation of files by CMake), but it does not persist unless we make it so. The things we want to persist are the things we've set, not the combination of everything, and I think this has been a key design flaw until now.

Those things we've set can/should be modular for our own sanity, so the acceleration flags should go in a (cached) ACCELERATION_{FEATURE}_C_FLAGS (rather than a polyglot GROMACS_C_FLAGS or ACCELERATION_C_FLAGS), and get added to CMAKE_C_FLAGS and the result not cached.

Currently it's not possible to change the acceleration flags in a subsequent run of CMake because it all goes in a polyglot CMAKE_C_FLAGS variable that has an internal cache variable to control whether it gets reset. Fixing that would need another variable to detect whether acceleration has changed, and then more infrastructure to try to re-construct the polyglot variable without the old acceleration settings, with the new ones, and preserving anything else that got thrown in the soup along the way.

I have what I consider a proper fix for acceleration flags in progress, building on some stuff currently on gerrit. WIP

#7 Updated by Roland Schulz over 6 years ago

Mark Abraham wrote:

I think the problem is that the design of CMakeLists.txt tries to be autotools and construct a monolithic set of persistent CFLAGS, and respond to environment variables. That's intrinsically broken for a multi-pass configuration system.

By monolithic you mean that the all stored in one variable? If they are all added by the same logic to CMAKE_C_*_FLAGS how does storing them into independent variables change anything (besides organizing it a bit)? How do we respond to environment variables?

I think a major problem is that we try to reinvent the wheel and don't try to learn from other big projects using cmake. And your new approach seems to do the same mistake again.

IMO the design needs to be such that the user should not want to override the CFLAGS.

I STRONG disagree. That implies we foresee any possible build the user wants to do? What about extra tools (e.g. adress-sanatizer)? We clearly can't add a cmake option for every debug/profile/... tool out there and add the appropriate flags. Also this makes it impossible for the user to set the appropriate flags for compiler (versions) we haven't tested.

As soon as we allow for overriding then the whole system is unworkable because nothing about any state of anything is known.

That's why we shouldn't imply state but test state. If we rerun most tests on every invocation the problem of how one option influence another is mostly gone. And we don't need all the logic of how they are related.

I'm skeptical that there should be the option to append flags. There's existing infrastructure for Release and Debug builds. That's where compiler flags should be tweaked (even in a User build type), not on the command line or environment.

You suggest I should add a ASAN build type if I want to use address-sanatizer (to stick with that example)? That is extremely time-consuming to use any tool. Also this would imply that we would need to add this ASAN build type to the official CMakeLists if we want to keep using it for Jenkins.

Libraries should be searched for from paths, not imposed by the user. Normal usage shouldn't need it; a user adding a tool with a new dependency needs to follow our (good) example for how to do that.

What problem does that solve? To just give one example. Currently if a library has already been found then setting GMX_PREFER_STATIC_LIB doesn't have any influence anymore, and one needs to change the library name (or clear the cache). Why would we disallow changing the library name if it doesn't solve any problem?

Instead, one should detect what flags are required for some feature, store them in an cache variable and use that only where required and do not cache any combined variable.

Yes. Not storing any combined variables is Teemu's suggestion I agree with. But that has nothing to do with all the other things. And it doesn't imply that we need to prohibit manual settings (see the two messages from me and Teemu before yours).

Now the problem of setting and re-setting reduces to managing the behaviour of a FindXXX module on a case-by-case basis. It's only when you append to variables and write the result to the cache that re-runs can keep spamming variables.

Yes. Like in Teemu and in my variation.

There are definitely GROMACS compiler settings that should be common across all source files (optimization flags, warning suppression, acceleration flags, probably not much else), and the CMake methodology for that is to use CMAKE_C_FLAGS or its relatives.

Well in theory other options exist (see above). But given that those are not language specific it is probably better to stick with CMAKE_C_FLAGS

If the user set something that's already cached. We can prepend or append to that to give the user the chance to specify things (but again I don't think we should plan for this to happen much). That happens at run time (and so is effective for managing the final generation of files by CMake), but it does not persist unless we make it so. The things we want to persist are the things we've set, not the combination of everything, and I think this has been a key design flaw until now.

OK. But this is the key point. How do you exactly want to persist? Do we need to persist at all (why not simply recompute the flags every time?) How do we append flags to user flags? How do we handle the default cmake flags?

Those things we've set can/should be modular for our own sanity, so the acceleration flags should go in a (cached) ACCELERATION_{FEATURE}_C_FLAGS (rather than a polyglot GROMACS_C_FLAGS or ACCELERATION_C_FLAGS), and get added to CMAKE_C_FLAGS and the result not cached.

That means that I can't overwrite flags. I think this is a bad idea. By itself, if ACCELERATION_C_FLAGS is still cached, it wouldn't solve the specific issue of this page which started this wider discussion. It also wouldn't solve #1040.

Currently it's not possible to change the acceleration flags in a subsequent run of CMake because it all goes in a polyglot CMAKE_C_FLAGS variable that has an internal cache variable to control whether it gets reset. Fixing that would need another variable to detect whether acceleration has changed, and then more infrastructure to try to re-construct the polyglot variable without the old acceleration settings, with the new ones, and preserving anything else that got thrown in the soup along the way.

Again, why persisting any of our flags at all?

#8 Updated by Szilárd Páll over 6 years ago

As I didn't have much time to think it through and consider all options and suggestion carefully, I don't have a strong opinion. However, I would like to emphasize a few point Roland also touches:

  • Adding and overriding flags seems to be quite difficult in the suggested scheme and not scriptable at all, making overriding from the command line nearly impossible (right?) -- although adding flags doesn't work with the current code either. Moreover, spreading compiler flags across a bunch of different cache variables makes quite difficult to review or change flags even using ccmake or gui.

#9 Updated by Mark Abraham over 6 years ago

RFC at https://gerrit.gromacs.org/#/c/1908

User can override anything before or after anything happens. Results of tests or user settings persist. Nothing else persists (or needs to, IMO). You can change GMX_CPU_ACCELERATION perfectly, and compiler detection only happens when it's needed.

#10 Updated by Mark Abraham over 6 years ago

Roland Schulz wrote:

Mark Abraham wrote:

I think the problem is that the design of CMakeLists.txt tries to be autotools and construct a monolithic set of persistent CFLAGS, and respond to environment variables. That's intrinsically broken for a multi-pass configuration system.

By monolithic you mean that the all stored in one variable?

The problem with monolithic variables is that they are all that could be cached in the old implementation, which built the previous mess of needing to manage when things were allowed to be set and problems dealing with subsequent user decisions.

If they are all added by the same logic to CMAKE_C_*_FLAGS how does storing them into independent variables change anything (besides organizing it a bit)?

The compiler flag variables that are dependent on only the result of a compiler test (or a user over-ride) can be cached. Anything can be constructed from them each iteration. Seems perfect.

How do we respond to environment variables?

In my new patch, we don't except inasmuch as CMake does by default. Previously we did respond to some environment variables (but in a buggy way - ENV{CFLAGS} was used in a predicate but not anywhere else I could see). What environment variable settings should we respond to that don't work now?

I think a major problem is that we try to reinvent the wheel and don't try to learn from other big projects using cmake. And your new approach seems to do the same mistake again.

I agree with trying to learn from others if we can identify people who have done it well. Our major mistake two years ago was switching to a build system where we had no experience and no example to follow (and there is probably still nothing published but the CMake book, which is based on 2.6 anyway, IIRC), and then trying to do it like autoconf.

I disagree that I'm reinventing new bad wheels. An iterative build system should cache invariants, i.e. results from user input and tests arising from them. The final configuration is constructed from the final set of invariants. The build system should not cache much else (anything?) because it doesn't need to, and doing so makes problems related to persistence of things that need to be responsive.

For example, the SSE4.1 compiler flags should be cached in a variable that relates only to that, and only get put into CMAKE_C_FLAGS in response to GMX_CPU_ACCELERATION=SSE4.1, and persist if the user changes to SSE2, and come back when SSE4.1 is chosen again and the final compiler command should have no history dragged into it, and make should work. This all happens smoothly and elegantly in the new commit, which is what this Redmine issue is really about :-)

IMO the design needs to be such that the user should not want to override the CFLAGS.

I STRONG disagree. That implies we foresee any possible build the user wants to do? What about extra tools (e.g. adress-sanatizer)? We clearly can't add a cmake option for every debug/profile/... tool out there and add the appropriate flags. Also this makes it impossible for the user to set the appropriate flags for compiler (versions) we haven't tested.

Sorry for bringing up a side issue. CMAKE_C_FLAGS=-Dspecial ccmake .. should work (and definitely does with the new approach). I think needing or wanting to write CMAKE_C_FLAGS="-mavx -mfancyshitforintel19" cmake .. is wrong. ACCELERATION_AVX_C_FLAGS="-mavx -mfancyshitforintel19" cmake .. to trigger shiny stuff for AVX in Intel 19 is much better, and this works now.

If someone wants to write a tool that needs GSL or something, and so needs to use CMAKE_C_FLAGS=-I/path/to/gsl.h cmake .. then they can do that. But I think we agree that have no business trying to do anything else for them.

As soon as we allow for overriding then the whole system is unworkable because nothing about any state of anything is known.

That's why we shouldn't imply state but test state. If we rerun most tests on every invocation the problem of how one option influence another is mostly gone. And we don't need all the logic of how they are related.

Rerunning dependent tests is fine (and necessary) - but I can't think of one for GROMACS offhand. Re-running invariant tests shouldn't need to happen ever. Storing only invariants solves exactly the problem you suggest - we shouldn't imply state, but only test it.

I'm skeptical that there should be the option to append flags. There's existing infrastructure for Release and Debug builds. That's where compiler flags should be tweaked (even in a User build type), not on the command line or environment.

You suggest I should add a ASAN build type if I want to use address-sanatizer (to stick with that example)? That is extremely time-consuming to use any tool. Also this would imply that we would need to add this ASAN build type to the official CMakeLists if we want to keep using it for Jenkins.

I have no idea what that tool is, so I can't comment directly. I'm not opposed to letting the user do things they need to do to do something special (that's their problem). But normal use of GROMACS should not need it at all. (Cue Szilard with some scary story about Intel C and -ip)

Libraries should be searched for from paths, not imposed by the user. Normal usage shouldn't need it; a user adding a tool with a new dependency needs to follow our (good) example for how to do that.

What problem does that solve? To just give one example. Currently if a library has already been found then setting GMX_PREFER_STATIC_LIB doesn't have any influence anymore, and one needs to change the library name (or clear the cache). Why would we disallow changing the library name if it doesn't solve any problem?

We're talking about different things. I don't think the user should be able to do something like FFTW_LIBRARY=/my/home/built/lib/libfftw3.so cmake .. - they should come in the front door with CMAKE_PREFIX_PATH=/my/home/built cmake .. like everybody else.

Triggering searches for only static libraries basically requires a cache refresh. Frankly, the best fix there is to make a fatal error and tell the user to refresh and go again with -DGMX_PREFER_STATIC_LIBS the first time (and move some logic accordingly).

Instead, one should detect what flags are required for some feature, store them in an cache variable and use that only where required and do not cache any combined variable.

Yes. Not storing any combined variables is Teemu's suggestion I agree with. But that has nothing to do with all the other things. And it doesn't imply that we need to prohibit manual settings (see the two messages from me and Teemu before yours).

Not storing combined variables has everything to do with avoiding the issue of this Redmine, and nothing to do with prohibiting manual settings. I do want to prohibit the user from being able to say "use my CFLAGS no matter what" because once we have that mechanism people will want to use it internally to avoid having to design stuff sensibly. The necessary corollary is that there has to be nothing going into the constructed CFLAGS that they cannot influence, either by replacement or appending. So far, this seems easy if we cache the things from which we construct CFLAGS.

Adding new variables to store information about the state of manual overrides is treating the symptom, not the cause of this Redmine issue. The cause is storing combined variables.

Now the problem of setting and re-setting reduces to managing the behaviour of a FindXXX module on a case-by-case basis. It's only when you append to variables and write the result to the cache that re-runs can keep spamming variables.

Yes. Like in Teemu and in my variation.

There are definitely GROMACS compiler settings that should be common across all source files (optimization flags, warning suppression, acceleration flags, probably not much else), and the CMake methodology for that is to use CMAKE_C_FLAGS or its relatives.

Well in theory other options exist (see above). But given that those are not language specific it is probably better to stick with CMAKE_C_FLAGS

If the user set something that's already cached. We can prepend or append to that to give the user the chance to specify things (but again I don't think we should plan for this to happen much). That happens at run time (and so is effective for managing the final generation of files by CMake), but it does not persist unless we make it so. The things we want to persist are the things we've set, not the combination of everything, and I think this has been a key design flaw until now.

OK. But this is the key point. How do you exactly want to persist? Do we need to persist at all (why not simply recompute the flags every time?) How do we append flags to user flags? How do we handle the default cmake flags?

See RFC.

Those things we've set can/should be modular for our own sanity, so the acceleration flags should go in a (cached) ACCELERATION_{FEATURE}_C_FLAGS (rather than a polyglot GROMACS_C_FLAGS or ACCELERATION_C_FLAGS), and get added to CMAKE_C_FLAGS and the result not cached.

That means that I can't overwrite flags. I think this is a bad idea. By itself, if ACCELERATION_C_FLAGS is still cached, it wouldn't solve the specific issue of this page which started this wider discussion. It also wouldn't solve #1040.

You can overwrite a cached variable once it's written, and if the test checks for its result before running, and knows whether it is an invariant test, we can avoid rerunning. We have to rerun any dependent tests - but that's inevitable.

In fact this approach does solve the issue here, and I suspect #1040 also.

Currently it's not possible to change the acceleration flags in a subsequent run of CMake because it all goes in a polyglot CMAKE_C_FLAGS variable that has an internal cache variable to control whether it gets reset. Fixing that would need another variable to detect whether acceleration has changed, and then more infrastructure to try to re-construct the polyglot variable without the old acceleration settings, with the new ones, and preserving anything else that got thrown in the soup along the way.

Again, why persisting any of our flags at all?

Because they take finite time to run and some of them we know don't change because the compiler shouldn't change. And no, I think it would be a terrible idea to support allowing a change of compiler after the initial call to cmake. There are basically no invariants under a change of compiler - a new cache is essential.

#11 Updated by Roland Schulz over 6 years ago

Mark Abraham wrote:

I agree with trying to learn from others if we can identify people who have done it well. Our major mistake two years ago was switching to a build system where we had no experience and no example to follow (and there is probably still nothing published but the CMake book, which is based on 2.6 anyway, IIRC), and then trying to do it like autoconf.

VTK, Paraview, KDE are good examples to study. There are several others.

For example, the SSE4.1 compiler flags should be cached in a variable that relates only to that, and only get put into CMAKE_C_FLAGS in response to GMX_CPU_ACCELERATION=SSE4.1, and persist if the user changes to SSE2, and come back when SSE4.1 is chosen again and the final compiler command should have no history dragged into it, and make should work. This all happens smoothly and elegantly in the new commit, which is what this Redmine issue is really about :-)

Yes that part does work nicely now. Not sure though whether it is the best solution and whether all the extra variables (besides acceleration) are a good idea.

IMO the design needs to be such that the user should not want to override the CFLAGS.

I STRONG disagree. That implies we foresee any possible build the user wants to do? What about extra tools (e.g. adress-sanatizer)? We clearly can't add a cmake option for every debug/profile/... tool out there and add the appropriate flags. Also this makes it impossible for the user to set the appropriate flags for compiler (versions) we haven't tested.

Sorry for bringing up a side issue. CMAKE_C_FLAGS=-Dspecial ccmake .. should work (and definitely does with the new approach). I think needing or wanting to write CMAKE_C_FLAGS="-mavx -mfancyshitforintel19" cmake .. is wrong.

But that is how the flags behave which cmake set by default. So for consistency (and #1040) that's how it should behave. Also it has the advantage that one can easily overwrite flags. One can get both the ease of use of adding and overwriting with the solution suggested for #1040.

ACCELERATION_AVX_C_FLAGS="-mavx -mfancyshitforintel19" cmake .. to trigger shiny stuff for AVX in Intel 19 is much better, and this works now.

but I don't think it is obvious to the user.

As soon as we allow for overriding then the whole system is unworkable because nothing about any state of anything is known.

That's why we shouldn't imply state but test state. If we rerun most tests on every invocation the problem of how one option influence another is mostly gone. And we don't need all the logic of how they are related.

Rerunning dependent tests is fine (and necessary) - but I can't think of one for GROMACS offhand. Re-running invariant tests shouldn't need to happen ever. Storing only invariants solves exactly the problem you suggest - we shouldn't imply state, but only test it.

We fully agree on that. That's what Teemu pointed out first.

We're talking about different things. I don't think the user should be able to do something like FFTW_LIBRARY=/my/home/built/lib/libfftw3.so cmake .. - they should come in the front door with CMAKE_PREFIX_PATH=/my/home/built cmake .. like everybody else.

Why. What does the simplify? And I also don't see how this is different from what I said.

Triggering searches for only static libraries basically requires a cache refresh. Frankly, the best fix there is to make a fatal error and tell the user to refresh and go again with -DGMX_PREFER_STATIC_LIBS the first time (and move some logic accordingly).

It was just one example. We won't solve all these for everyone.

I do want to prohibit the user from being able to say "use my CFLAGS no matter what" because once we have that mechanism people will want to use it internally to avoid having to design stuff sensibly.

Here I completely disagree. Your goal should be achieved by code review. A user interface (I think non-internal cmake variables constitutes as such) design shouldn't be influenced by internal coding goals, like encouraging good coding practices. And it breaks the consistency requested by #1040.

The necessary corollary is that there has to be nothing going into the constructed CFLAGS that they cannot influence, either by replacement or appending. So far, this seems easy if we cache the things from which we construct CFLAGS.

That is a solution, but causes the large number of variables.

Adding new variables to store information about the state of manual overrides is treating the symptom, not the cause of this Redmine issue. The cause is storing combined variables.

No. The problem was that the way the variables was written was broken. And (as we agree) it was broken because one cannot combine variables and cache the result. The whole problem is complicated by the fact that CMAKE_C_FLAGS is both the result of the combination of several variables (for our automatic setting) and the standard cmake variable for the user to set. And it can't be both at the same time. But it can be either one at different times, and doesn't have to be restricted to be the combination of variables (your solution). We can simply not touch CMAKE_C_FLAGS at all if it is already set by the user. This makes overwriting easy and is the only solution I can see for #1040 (to make it consistent with the way the default cmake flags work).

Currently it's not possible to change the acceleration flags in a subsequent run of CMake because it all goes in a polyglot CMAKE_C_FLAGS variable that has an internal cache variable to control whether it gets reset. Fixing that would need another variable to detect whether acceleration has changed, and then more infrastructure to try to re-construct the polyglot variable without the old acceleration settings, with the new ones, and preserving anything else that got thrown in the soup along the way.

Again, why persisting any of our flags at all?

Because they take finite time to run and some of them we know don't change because the compiler shouldn't change. And no, I think it would be a terrible idea to support allowing a change of compiler after the initial call to cmake. There are basically no invariants under a change of compiler - a new cache is essential.

I didn't mean to suggest not to persist the tests. I agree they should. But they are persisted twice with the RFC. E.g. both CFLAGS_WARN-ADVANCED:INTERNAL and CFLAGS_WARN:STRING. For the reasons you mention only CFLAGS_WARN-ADVANCED is needed not also CFLAGS_WARN. CFLAGS_WARN is only needed for the user to overwrite it. But with a better solution for that, it isn't needed at all.

#12 Updated by Teemu Murtola over 6 years ago

  • Assignee changed from Rossen Apostolov to Mark Abraham
  • Target version set to 4.6

Some comments, sorry for not putting these in the context by quoting previous answers (and so possibly missing some points):

  • It is trivial to implement an override option for the user using either
    • A separate variable (e.g., CMAKE_C_FLAGS_OVERRIDE) that, if set, is used instead of any Gromacs or CMake-defined default flags.
    • A separate flag (e.g., GMX_SKIP_DEFAULT_C_FLAGS) that, if set, causes Gromacs to use any value provided for CMAKE_C_FLAGS as it was given.
  • It is also possible to implement it such that setting CMAKE_C_FLAGS causes it to be used without changing anything (Roland's proposition in #1040), although this is slightly trickier (and some corner cases can be left that are difficult to treat (e.g., if the user happens to set the variable to the default used by CMake).
  • Both of the above can also be applied to subsets of the variables; Mark's approach is an extreme one where essentially every single flag has its own cache variable.
  • Neither of the above is in conflict with the suggestion to not cache dependent state; user override is also one input parameter.
  • If we want to have an easily viewable set of flags, we can then either 1) name the flag cache variables with a common prefix such that they show next to each other in a GUI, or 2) add a separate cache variable that in the end is set as set(FULL_C_FLAGS ${CMAKE_C_FLAGS} CACHE STRING "Complete set of compiler flags" FORCE), and not used for anything else (of course, the build type flags should be treated separately; there are at least two options). This, combined with a user override option, should be a relatively simple way for editing as well.
  • I agree with Roland that we can't possibly predict all situations where users would want to add or modify existing flags.
  • Does the RFC really add both of those CFLAGS_WARN variants, or is one a leftover from an earlier cmake run? Earlier, an internal variable was used, but the RFC only seems to use a string variable.

#13 Updated by Roland Schulz over 6 years ago

What is the plan for a solution to this problem? It has high priority so we should try to fix it for 4.6. I think 1908 has no chance to get approved. I know I would vote -1 and I assume (based on his comments) Teemu would too. And of course the commit it depends on already has -2 and -1. This means we need a different approach. Mark are you working on something new or do you want to change the assignee? If so should we implement the solution suggested by Teemu and me?

#14 Updated by Erik Lindahl over 6 years ago

  • Assignee changed from Mark Abraham to Roland Schulz

@Roland: Mark has a ton of things on his todo-list, so I think it would be great if you and/or Teemu could have a look here.

However, given the vivid discussions we tend to end up in when it comes to CMake architectures, it might be wisest to try and selectively address a couple of the worst problems we have, rather than completely redesigning the way all flags are handled. I'll trust your judgement there, though...

#15 Updated by Szilárd Páll over 6 years ago

Erik Lindahl wrote:

However, given the vivid discussions we tend to end up in when it comes to CMake architectures, it might be wisest to try and selectively address a couple of the worst problems we have, rather than completely redesigning the way all flags are handled. I'll trust your judgement there, though...

I agree with you suggestion. We don't seem to have the time and resources for a complete CMake overhaul so addressing some of the issues like this CFLAGS problem (and perhaps the configuration summary) would already improve the build system quite a bit.

#16 Updated by Roland Schulz over 6 years ago

  • Status changed from New to In Progress

#17 Updated by Roland Schulz over 6 years ago

  • Status changed from In Progress to Closed

Also available in: Atom PDF