Skip to content

CMake migration: fix a few glitches - #2183

Merged
ktf merged 2 commits into
AliceO2Group:devfrom
ktf:fix-cmake-migration
Jul 10, 2019
Merged

CMake migration: fix a few glitches#2183
ktf merged 2 commits into
AliceO2Group:devfrom
ktf:fix-cmake-migration

Conversation

@ktf

@ktf ktf commented Jul 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Member
  • Clang requires LLVM on some platforms, so if LLVM is not there,
    this will fail.
  • We need to protect against usage of simulation packages when
    Geant4 is not there.
  • ROOT needs to be at least v6.16.00

@ktf
cmake migration: fix a few glitches
1a02a69 
* Clang requires LLVM on some platforms, so if LLVM is not there,
  this will fail.
* We need to protect against usage of simulation packages when
  Geant4 is not there.
@ktf ktf mentioned this pull request Jul 10, 2019
Merged
@ktf
ktf merged commit 154b764 into AliceO2Group:dev Jul 10, 2019
@ktf
ktf deleted the fix-cmake-migration branch July 10, 2019 13:09
@ktf

ktf commented Jul 10, 2019

Copy link
Copy Markdown
Member Author

@aphecetche the o2/cuda issue seems unrelated, however I have the impression the CMake migration reintroduced the duplicated include paths. Could you confirm? If yes, could you add a list(... UNIQUE) wherever it is needed?

@aphecetche

aphecetche commented Jul 10, 2019

Copy link
Copy Markdown
Collaborator

@ktf where exactly do you see duplicated include paths ? In principle wherever possible I've used list(REMOVE_DUPLICATES ...) (but for instance the equivalent for generator expression will only be available in CMake 3.15) ?

@ktf

ktf commented Jul 10, 2019

Copy link
Copy Markdown
Member Author

Look at the log at https://ali-ci.cern.ch/repo/logs/AliceO2Group/AliceO2/2183/825eb00167844b4658bde2cccf2bf6e6a6c2dcff/build_o2_cuda/fullLog.txt

@aphecetche

aphecetche commented Jul 10, 2019

Copy link
Copy Markdown
Collaborator

I must be blind : what exactly am I supposed to look at in the log ?

@ktf

ktf commented Jul 10, 2019

Copy link
Copy Markdown
Member Author

Look for === Detectors/TPC/monitor/macro/RunSimpleEventDisplay.C_compiled - Argument: /build/ci_checks/o2_checker_linux_o2_ci_0/sw/slc7_x86-64/pythia/latest/include:/build/ci_checks/o2_checker_linux_o2_ci_0/. There is a long line which has /build/ci_checks/o2_checker_linux_o2_ci_0/sw/slc7_x86-64/ROOT/latest/include repeated a few (dozens) of times.

@aphecetche

aphecetche commented Jul 10, 2019

Copy link
Copy Markdown
Collaborator

ok : that falls into that comment

AliceO2/cmake/O2AddTestRootMacro.cmake

Line 109 in c3a5896

# FIXME: once CMake 3.15 is out, use $<REMOVE_DUPLICATES:list> to dedupe the

unless this is a critical issue, I'd just wait for CMake 3.15 to solve the problem for us (should not be long as it's already rc3)

@ktf

ktf commented Jul 10, 2019

Copy link
Copy Markdown
Member Author

Can we really move to 3.15 as it comes out? In general we said we should wait for devtoolset on centos to provide them, before we actually do the upgrade.

@ktf

ktf commented Jul 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

Ok, but why not list(REMOVE_DUPLICATES incdir)?

@aphecetche

aphecetche commented Jul 10, 2019

Copy link
Copy Markdown
Collaborator

Because the expansion of the $<TARGET_PROPERTY:${t},INTERFACE_INCLUDE_DIRECTORIES> only happens at generation time, not a configure time, so list(REMOVE_DUPLICATES incdir) has no duplicate to work with in fact. Hence the need for the generator expression version of it.

MichaelLettrich pushed a commit that referenced this pull request Jul 24, 2019
@ktf @MichaelLettrich
CMake migration: fix a few glitches (#2183)
59f599d 
* Clang requires LLVM on some platforms, so if LLVM is not there,
  this will fail.
* We need to protect against usage of simulation packages when
  Geant4 is not there.
* Update ROOT requirements to v6-16-00
knopers8 pushed a commit to knopers8/AliceO2 that referenced this pull request Oct 23, 2019
@ktf @knopers8
CMake migration: fix a few glitches (AliceO2Group#2183)
6717f25 
* Clang requires LLVM on some platforms, so if LLVM is not there,
  this will fail.
* We need to protect against usage of simulation packages when
  Geant4 is not there.
* Update ROOT requirements to v6-16-00
carlos-soncco pushed a commit to carlos-soncco/AliceO2 that referenced this pull request Oct 28, 2019
@ktf @carlos-soncco
CMake migration: fix a few glitches (AliceO2Group#2183)
ff08fc9 
* Clang requires LLVM on some platforms, so if LLVM is not there,
  this will fail.
* We need to protect against usage of simulation packages when
  Geant4 is not there.
* Update ROOT requirements to v6-16-00
EmilGorm pushed a commit to EmilGorm/AliceO2 that referenced this pull request Apr 15, 2023
@mhemmer-cern
[EMCAL-689] EMCal TM: Add missing fill histo (AliceO2Group#2183)
8b91a58 
- Add missing fill of new A-/C-Side histograms for electron E/p in case pion rejection is not active
- Add info for TPC NSgima configurables to be more clear
- Changed if statements for TPC PID NSigma to be of format: lower boundary <= NSigma && NSigma <= upper boundary to be more clear
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ktf @aphecetche