Tidies up CMake, enables core libraries to build with -Wall -Wextra -… #190

luketpickering · 2024-10-28T16:21:04Z

…Werror

updates to yaml 0.8.0
updates to CPM 0.40.2
removes dependency on NuHepMC/CMakeModules
sets CXX standard to 17, can probably roll it back, but there is C++17 code in here
separates warning/error flags from other compiler flags
- warnings/errors become a separate interface target that we link privately to library code
all core libraries updated (hopefully minimally) to build under stringent compiler checks. Most of this is:
- myvect.size() -> int(myvect.size())
- (int)mylong -> int(mylong)
- (TObj*)SomeROOTFunc() -> static_cast<TObj*>(SomeROOTFunc())
Also changes macro types to using declarations

Pull request description

Changes or fixes

Examples

KSkwarczynski

Thanks a lot for tidyng all of this mess.
I have some minor comments which bots might have not found

KSkwarczynski · 2024-10-28T17:15:26Z

CMakeLists.txt

-  set(CMAKE_CXX_STANDARD 14)
-endif()
-
-# KS: If C++ standard is lower than C++ standard used for ROOT compilation things will go terribly wrong
-if(DEFINED ROOT_CXX_STANDARD AND ROOT_CXX_STANDARD GREATER CMAKE_CXX_STANDARD)
-  set(CMAKE_CXX_STANDARD ${ROOT_CXX_STANDARD})
-endif()
-cmessage(STATUS "CMAKE CXX Standard: ${CMAKE_CXX_STANDARD}")
-
-# KS: ROOT changed cmake in 6.32, we should move avay from using Luke's hack, keep it for due to compatibility
-if(ROOT_CXX_STANDARD LESS 14 AND ROOT_VERSION VERSION_LESS 6.32.00)
-  cmessage(WARNING "ROOT CXX STANDARD: ${ROOT_CXX_STANDARD}")
+  set(CMAKE_CXX_STANDARD 17)


can you try ussing ROOT_CXX_STANDARD?
It is in root cofnig
https://github.com/root-project/root/blob/8d5764957e4bcb9cdc0bca44f83fb2fb4fe1815d/cmake/scripts/RootUseFile.cmake.in#L12

oh, no its not, if we 'use' the RootUseFile then it sets the standard for the whole compilation, but it doesn't expose directly what standard it uses. I'll just grab the flag-stripping code from NuHepMC

I've put something a bit hacky in for the moment. NuOscillator uses the nuhepmc/cmakemodules still and it ends up setting up root, so I'd need to PR it out of that to properly just let ROOT set itself up. But this builds for the moment

splines/SplineCommon.h

cmake/Modules/MaCh3Dependencies.cmake

CMakeLists.txt

KSkwarczynski · 2024-10-28T19:46:56Z

According to CI Minuit2 flag is no longer on

MaCh3/cmake/Modules/MaCh3Dependencies.cmake

Line 36 in 43e4b14

    
           if(ROOT_VERSION GREATER_EQUAL 6.32.00 OR ROOT_CXX_FLAGS MATCHES "-DMINUIT2_ENABLED")

we were reyling on ROOT_CXX_FLAGS set via NuHEP Tools
https://github.com/mach3-software/MaCh3/actions/runs/11558478688/job/32170957073?pr=190

CMakeLists.txt

…Werror - updates to yaml 0.8.0 - updates to CPM 0.40.2 - removes dependency on NuHepMC/CMakeModules - sets CXX standard to 17, can probably roll it back, but there is C++17 code in here - separates warning/error flags from other compiler flags - warnings/errors become a separate interface target that we link privately to library code - all core libraries updated (hopefully minimally) to build under stringent compiler checks. Most of this is: * myvect.size() -> int(myvect.size()) * (int)mylong -> int(mylong) * (TObj*)SomeROOTFunc() -> static_cast<TObj*>(SomeROOTFunc()) - Also changes macro types to using declarations

- Updates note about float types for low memory structs - adds not about diagnostic disabling

- points NuOscillator at luketpickering fork momentarily to test letting that set up ROOT directly. - builds with DEBUG enabled now as well which was highlighting other issues during CI.

…lator

`abs` is a C-library function which operates on ints, `fabs` is the float version. We should use std::abs which is the C++ version that has overloads (which don't exist in C).

- also catches a few more uses of `abs` rather than `std::abs`.

- also fix bug in ROOT c++ standard constraint

- make Catch2 find_package quite - fix bug in GetPentaltyTerm where dial value was an int - lots of explicit type cats - ignore useless-cast in lots of places to allow M3::float_t = double/float compilation

KSkwarczynski

Soma comments

cmake/Modules/NuOscillatorSetup.cmake

covariance/covarianceBase.h

tests/manager_tests.cpp

luketpickering requested review from ewanwm, EdAtkin, dbarrow257, KSkwarczynski and henry-wallace-phys as code owners October 28, 2024 16:21

github-actions bot added Plotting plotting related MCMC MCMC related Nu Osc/Xsec Related with neutrino interactions or oscialtions Samples Manager Cmake labels Oct 28, 2024

luketpickering force-pushed the feature/picker24/wall-fix branch from e3d9978 to 30cb019 Compare October 28, 2024 16:35

KSkwarczynski reviewed Oct 28, 2024

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

luketpickering force-pushed the feature/picker24/wall-fix branch from 13226a4 to 31d7b1b Compare October 29, 2024 07:38

github-actions bot added the CI/CD CI/CD label Oct 29, 2024

luketpickering force-pushed the feature/picker24/wall-fix branch 3 times, most recently from b5d4fbf to 4d68790 Compare October 30, 2024 10:41

Luke Pickering added 10 commits October 30, 2024 11:18

Adds cmake option for python

ac8dae1

adds some more options to the yaml build to get it to build less

dbb70e1

fixes python bindings after tidy up

0d34241

NULL -> nullptr

98e430d

un-un-fix the samplePDF python wrapper

f39268e

roll back define -> constexpr inline to coddle old nvcc

afef265

respond to minor cmake comments on PR

2a6366c

use find_package for root dependency in MaCh3Config.cmake.in

fbb6ada

adds MaCh3CompileDefinitions target to house definitions

3adcb90

Luke Pickering added 16 commits October 30, 2024 11:22

Update Contributing

939b3e0

- Updates note about float types for low memory structs - adds not about diagnostic disabling

roll back type changes to covariance/ and splines/ best to change less.

62a3e12

Everything now compiles with all errors enabled.

dfbb2f6

- points NuOscillator at luketpickering fork momentarily to test letting that set up ROOT directly. - builds with DEBUG enabled now as well which was highlighting other issues during CI.

Pass CMAKE_CXX_STANDARD (determined with ROOT's help) back to NuOscil…

7fbb58a

…lator

fix markdown linter failures

e2109b1

adds cmath and explicitly uses std::abs to avoid float-conversion to int

5dc208d

`abs` is a C-library function which operates on ints, `fabs` is the float version. We should use std::abs which is the C++ version that has overloads (which don't exist in C).

fix errors on debian build

2b80c58

removes all c++17 features

03e95a8

be more descriptive about why you set the standard you did

3973f31

adds some pragma guards around root interface function that changes type

b0eeca3

typo

5dcd494

further improves CONTRIBUTING.md notes about heightened compiler pedancy

81cce32

fixes a few more conversions that GCC10 on ubuntu22 had issues with

004aa8f

- also catches a few more uses of `abs` rather than `std::abs`.

Provide MACH3_CXX_STANDARD as a global property for use by dependents

30fa3df

- also fix bug in ROOT c++ standard constraint

ignore all build-like directories

d442469

Adds a unit test of one of the important YamlHelper functions

5ab2064

luketpickering force-pushed the feature/picker24/wall-fix branch from afc9d27 to 5ab2064 Compare October 30, 2024 12:14

Luke Pickering added 2 commits October 30, 2024 14:10

various tweaks to compile with lots of warnings/errors

fdcc7ba

- make Catch2 find_package quite - fix bug in GetPentaltyTerm where dial value was an int - lots of explicit type cats - ignore useless-cast in lots of places to allow M3::float_t = double/float compilation

float conversions were a red herring.. roll back the roll back

9d795a2

luketpickering force-pushed the feature/picker24/wall-fix branch from a5e9318 to 9d795a2 Compare October 30, 2024 16:24

Luke Pickering added 2 commits October 31, 2024 10:12

bleeding edge compiler detects some possible nullptr dereferencing

e68be27

fix incorrect float cast

a31735c

KSkwarczynski reviewed Oct 31, 2024

View reviewed changes

cmake/Modules/NuOscillatorSetup.cmake Outdated Show resolved Hide resolved

covariance/covarianceBase.h Outdated Show resolved Hide resolved

covariance/covarianceBase.h Outdated Show resolved Hide resolved

tests/manager_tests.cpp Outdated Show resolved Hide resolved

Luke Pickering added 4 commits October 31, 2024 11:46

undo re-add of deprecated code. oops

1840c76

remove tests, put them in tutorial

3a6f1fb

track official tag of NuOscillator

da31506

adds NuOscillatorInterface validations to CI

596af29

KSkwarczynski approved these changes Oct 31, 2024

View reviewed changes

KSkwarczynski merged commit aae06b6 into develop Oct 31, 2024
11 of 14 checks passed

KSkwarczynski deleted the feature/picker24/wall-fix branch October 31, 2024 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tidies up CMake, enables core libraries to build with -Wall -Wextra -… #190

Tidies up CMake, enables core libraries to build with -Wall -Wextra -… #190

luketpickering commented Oct 28, 2024

KSkwarczynski left a comment

KSkwarczynski Oct 28, 2024

luketpickering Oct 29, 2024

luketpickering Oct 29, 2024

luketpickering Oct 29, 2024

KSkwarczynski commented Oct 28, 2024 •

edited

Loading

KSkwarczynski left a comment

Tidies up CMake, enables core libraries to build with -Wall -Wextra -… #190

Tidies up CMake, enables core libraries to build with -Wall -Wextra -… #190

Conversation

luketpickering commented Oct 28, 2024

Pull request description

Changes or fixes

Examples

KSkwarczynski left a comment

Choose a reason for hiding this comment

KSkwarczynski Oct 28, 2024

Choose a reason for hiding this comment

luketpickering Oct 29, 2024

Choose a reason for hiding this comment

luketpickering Oct 29, 2024

Choose a reason for hiding this comment

luketpickering Oct 29, 2024

Choose a reason for hiding this comment

KSkwarczynski commented Oct 28, 2024 • edited Loading

KSkwarczynski left a comment

Choose a reason for hiding this comment

KSkwarczynski commented Oct 28, 2024 •

edited

Loading