Fix misc. build warnings with gcc/clang in nightlies

/ci-test --merge

[2020-06-09 14:28] Validation started with lhcb-master-mr#910

About:

CMake Warning at Gaudi/InstallArea/x86_64-centos7-gcc9-opt/cmake/GaudiBuildFlags.cmake:45 (message):
375	
  CMAKE_BUILD_TYPE set to RelWithDebInfo, but BINARY_TAG build type opt
376	
  implies Release

What is the logic in Gaudi for the build type? Is BINARY_TAG a cmake option or an environment variable? @clemenci . We should follow the same logic when compiling Gaudi-Allen.

About the last warning, python3 is needed to generate Allen configurations. It would be interesting to test it with the py3 slot and see if the warning goes away.

@clemenci should comment but I believe it can be either. In my local builds here I have it as an env. var.

cl034 ~/LHCbCMake/Feature/Allen > echo $BINARY_TAG
x86_64-centos7-clang8-opt

Its also an env. var. in the nightlies. See the environment and configure logs at

https://lhcb-nightlies.web.cern.ch/logs/build/nightly/lhcb-master/1121/x86_64-centos7-clang8-opt/Allen/

About the last warning, python3 is needed to generate Allen configurations. It would be interesting to test it with the py3 slot and see if the warning goes away.

Would it then be OK to make this an INFO, not a WARNING ? As its 'expected' in the current builds, and things anyway work, a WARNING seems too strong.

Yes, I agree it should be an INFO.

Relying on an environment variable is rather fragile. Is there also a cmake variable that could be passed to Allen in the configure process?

Otherwise, for now we can just implement the same logic with that env var in the Allen-Gaudi section in the main Allen CMakeLists.txt.

I believe you can pass an argument if you want, but the env. var is how the nightlies does it so I think you need to support it regardless.

so use an argument if passed to CMake, otherwise fallback to an env. var. If that fails ??? guess..

Actually the CMake option already exists and is CMAKE_BUILD_TYPE. So yeah, if the env var exists, overseed that I guess.

BINARY_TAG is both and environment variable and a CMake variable, and it's used to drive the CMAKE_BUILD_TYPE. In principle I would like to use it as a pure CMake variable, but some tests, scripts, etc. rely on that environment variables (and in some cases on CMTCONFIG), so the environment variable is used to initialize the CMake variable, and the CMake variable is used to set the environment variable in the runtime.

Things will change with the new Gaudi CMake configuration.

As a matter of fact, you should not set CMAKE_BUILD_TYPE in the CMakeLists.txt, or at least not unconditionally: set it to whatever you like most if it was not already set, but, please, do not override my decision to build in Release, Debug or whatever else mode.

Currently, CMAKE_BUILD_TYPE is presented in Allen's CMakeLists.txt as an option with a default value of RelWithDebInfo, and it is never set (other than the default value). This is why it was colliding with Gaudi's value I presume, as it was set by using some other variable / option (BINARY_TAG). With @jonrob 's fix the warning should now be gone.

Looking forward to the Gaudi CMake configuration, at which point we will probably revisit the Allen CMake configuration as well (which honestly could use a revision).

@dcampora Looking at the build commands in the nightlies I think there is a problem in that Allen is overriding some of the settings coming from Gaudi, when it shouldn't. i.e. I see that

CMakeLists.txt:set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "-O3 -g -DNDEBUG")

is effectively being applied in both the optimised and debug builds in the nightlies, over riding what Gaudi sets. This means, for instance, we are getting -g applied in the optimised builds and -O3 used in the debug builds. Neither of these are what Gaudi sets (we prefer to not have -g in the opt builds, and use -Og in debug builds.

So please, could you look to just not setting this at all yourself in Gaudi based builds and use whatever Gaudi sets.

i.e. see

https://gitlab.cern.ch/lhcb/Allen/-/blob/master/CMakeLists.txt#L9

a number settings there should only be set if not previously set, in Gaudi.

Could you please open an issue? The repercusions of this may be as little as just having to modify those flags you indicated - or as big as having to rename several variables and hide definitions if not in standalone mode.

Probably this hints at requiring to rethink how the cmake structure is going to support in a scalable manner both standalone and non-standalone builds.

see #150

Just one more comment here, then I jump on #150.

The settings we share between projects are managed via GaudiBuildFlags.cmake, included by the function gaudi_project(). Allen does not use them, so it's not picking up the mapping from BINARY_TAG to CMAKE_BUILD_TYPE, and build flags. With the new configuration (see LBCOMP-23) we took a step back and keep the configuration as focused on the build as possible (no tunings of warnings or very special build flags), moving the LHCb specific settings to the toolchain files. To help developers we added one setting to enable a bunch of options helping the developers (a bit like the default CMAKE_BUILD_TYPE in Allen).

Anther thing, do not invent your flag to enable/disable tests, use BUILD_TESTING from https://cmake.org/cmake/help/latest/module/CTest.html

added 1 deleted label

mentioned in issue #149 (closed)

added 1 commit

d372d652 - Fix misc. build warnings with gcc/clang in nightlies

Compare with previous version

@dcampora Allen is currently not listed in the projects built in the lhcb-gaudi-head-py3 slot.

https://lhcb-nightlies.web.cern.ch/nightly/lhcb-gaudi-head-py3/build/379/

It needs explicitly adding.

mentioned in merge request lhcb-core/LHCbNightlyConf!388 (merged)

added 1 commit

88ae2f0a - Convert python3 WARNING message to STATUS

Compare with previous version

/ci-test --merge

[2020-06-09 18:24] Validation started with lhcb-master-mr#913
[2020-06-10 00:10] Validation started with lhcb-sanitizers#590
[2020-06-10 00:11] Validation started with lhcb-gaudi-head-py3#380
[2020-06-10 00:11] Validation started with lhcb-lcg-test#58
[2020-06-10 00:11] Validation started with lhcb-lcg-dev3#1294
[2020-06-10 00:14] Validation started with lhcb-test-efficiency#96
[2020-06-10 00:17] Validation started with lhcb-lcg-dev4#1308
[2020-06-10 00:18] Validation started with lhcb-head#2617
[2020-06-10 00:25] Validation started with lhcb-gaudi-head#2627
[2020-06-10 04:04] Validation started with lhcb-sanitizers#591
[2020-06-11 00:10] Validation started with lhcb-test-efficiency#97
[2020-06-11 00:11] Validation started with lhcb-sanitizers#592
[2020-06-11 00:14] Validation started with lhcb-gaudi-head#2628
[2020-06-11 00:14] Validation started with lhcb-lcg-dev4#1309
[2020-06-11 00:14] Validation started with lhcb-lcg-dev3#1295
[2020-06-11 00:18] Validation started with lhcb-gaudi-head-py3#381
[2020-06-11 00:18] Validation started with lhcb-lcg-test#59
[2020-06-11 00:19] Validation started with lhcb-head#2618
[2020-06-12 00:09] Validation started with lhcb-lcg-dev4#1310
[2020-06-12 00:10] Validation started with lhcb-lcg-dev3#1296
[2020-06-12 00:11] Validation started with lhcb-gaudi-head-py3#382
[2020-06-12 00:11] Validation started with lhcb-sanitizers#593
[2020-06-12 00:12] Validation started with lhcb-lcg-test#60
[2020-06-12 00:14] Validation started with lhcb-test-efficiency#98
[2020-06-12 00:17] Validation started with lhcb-gaudi-head#2629
[2020-06-12 00:25] Validation started with lhcb-head#2619

resolved all threads

@dcampora I am getting warnings about the pipeline tests. Is there anything to worry about ? I know from the ci-test my changes are fine. Could it be in some way related to the fact I used my own fork to host the branch here. Do I need to enable something or other in it to allow these pipelines to work ?

mentioned in issue #150

mentioned in merge request !398 (merged)

mentioned in merge request gaudi/Gaudi!1046 (merged)

@dcampora so what happens next with this ? Are you waiting for something else from me ? If would be good to merge this asap, firstly to fix the builds in lhcb-master but also as a precursor to fixing gaudi/Gaudi!1046 (merged)

Indeed. Could you please create a branch in Allen, merge your changes into that one, and open a MR from that one? Then we can see the result of the Allen CI and merge accordingly.

We should at that point close this MR, and for bookkeeping reasons cite this MR in that new one.

Done. Will close this now.

mentioned in merge request !399 (merged)

Fix misc. build warnings with gcc/clang in nightlies

Merge request reports

Activity