Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update MC packages and FairRoot versions: #2114

Merged
merged 13 commits into from
May 3, 2020

Conversation

ihrivnac
Copy link
Contributor

@ihrivnac ihrivnac commented Feb 28, 2020

GEANT3: v3-3
GEANT4: v10.5.1
GEANT4_VMC: v5-0-p2
VGM: v4-7
FairRoot: 4ab2d2b1f0392 (includes a fix in FindROOT.cmake)

  • Keep previous versions for builds with ROOT 5:
    GEANT3: v2-7-p1
    VGM v4-4
    GEANT4_VMC v3-6-p6-inclxx-biasing-p2
    GEANT4 v10.4.2
    This should hopefully fix the JIRA issues O2-1029, O2-1044.

@ihrivnac ihrivnac requested a review from a team as a code owner February 28, 2020 09:54
@ghost ghost requested a review from MichaelLettrich February 28, 2020 09:54
@ihrivnac
Copy link
Contributor Author

In Geant4 10.5 the URL for data sets is changed from http:// to https:// which causes the error:
'Protocol "https" not supported or disabled in libcurl'
Could someone of the @alisw/infrastructure-admins check this? Thanks.

@ihrivnac
Copy link
Contributor Author

ihrivnac commented Mar 3, 2020

@ktf, could you, please, take a look why curl on the server does not support https protocol? See the error extracted from the log above. Thank you.

@ihrivnac
Copy link
Contributor Author

ihrivnac commented Mar 9, 2020

@ktf,
this PR needs an administrator's intervention. Can you check?

@ihrivnac ihrivnac force-pushed the mc-versions-update branch from d83734e to ae9c28d Compare March 11, 2020 10:31
@ihrivnac
Copy link
Contributor Author

Hi,
After the commit ae9c28d, the problem with downloading Geant4 data is gone.
The PR is now blocked due to aliDoctor error; is anyone (@ktf, @davidrohr , @pzhristov ) able to fix these ? We need to keep the old versions for all builds with ROOT 5.
Thank you,

@ihrivnac
Copy link
Contributor Author

In build/O2/alidist, we get a list of failing macros, where we get errors about incomplete types, eg:

In file included from input_line_9:1:
In file included from /mnt/mesos/sandbox/sandbox/O2/macro/o2sim.C:11:
/mnt/mesos/sandbox/sandbox/O2/macro/build_geometry.C:48:24: error: unknown type name 'FairRunSim'
void finalize_geometry(FairRunSim* run);

To compare these with a successful build, we need a log from a recently merged PR. At the last WP3 meeting, @ktf mentioned that this is possible with the current system. @ktf, could you, please, point me at such a log from a recently merged PR?

@ktf
Copy link
Member

ktf commented Mar 25, 2020

Just click on a commit and then on the details of a given test. If it's still there, you will get the log, otherwise you will get a "404" error.

@ihrivnac
Copy link
Contributor Author

Thanks. I tried https://github.com/alisw/alidist/pull/2154/commits, then after clicking on the x for the 3rd commit, I get the logs. However here, we have "2 errored and 3 successful checks" but the PR was merged anyway.

I was able to find a successful build and its log for this machine in the PR
#2153. Here, all tests pass ok while in our case, 24 tests, all with just one exception are tests run from a macro. For all these tests, the MC change is irrelevant, so the only cause possible is the FairRoot upgrade.

@sawenzel , @aphecetche , would you have an idea what could be common to these failing macros (as the log is very long, I put them also below):
93 - Detectors/PHOS/testsimulation/PutPhosInTop.C (Failed)
95 - Detectors/PHOS/testsimulation/drawPHOSgeometry.C (Failed)
103 - Detectors/CPV/testsimulation/drawCPVgeometry.C (Failed)
137 - Detectors/EMCAL/testsimulation/PutEmcalInTop.C (Failed)
139 - Detectors/EMCAL/testsimulation/drawEMCALgeometry.C (Failed)
141 - Detectors/EMCAL/testsimulation/run_sim_emcal.C (Failed)
159 - Detectors/TOF/prototyping/drawTOFgeometry.C (Failed)
241 - Detectors/MUON/MID/Simulation/macros/drawMIDGeometry.C (Failed)
280 - Detectors/TPC/simulation/macro/laserTrackGenerator.C (Failed)
287 - Detectors/TPC/monitor/macro/RunCompareMode3.C (Failed)
317 - Generators/share/external/extgen.C (Failed)
318 - Generators/share/external/extgen.C_compiled (Failed)
319 - Generators/share/external/pythia6.C (Failed)
321 - Generators/share/external/QEDLoader.C (Failed)
322 - Generators/share/external/tgenerator.C (Failed)
350 - macro/build_geometry.C (Failed)
356 - macro/eventDisplay.C (Failed)
357 - macro/eventDisplay.C_compiled (Failed)
362 - macro/o2sim.C (Failed)
374 - macro/run_clus_itsSA.C (Failed)
376 - macro/run_clus_mftSA.C (Failed)
378 - macro/run_clus_tof.C (Failed)
380 - macro/run_clus_tpc.C (Failed)
410 - macro/run_trac_mft.C (Failed)

@ktf
Copy link
Member

ktf commented Mar 25, 2020

It was merged because the error was unrelated.

@ihrivnac
Copy link
Contributor Author

Finally, I was able to reproduce the error on CC7. In the LD_LIBRARY_PATH, there is:
/root/alice/sw/slc7_x86-64/FairRoot/4ab2d2b1f0-1/lib
while the FairRoot installs libraries in lib64:
/root/alice/sw/slc7_x86-64/FairRoot/4ab2d2b1f0-1/lib64

After linking by hand lib64 to lib, the failing tests pass.
So we need to tell somewhere in dependencies, the correct lib path.
@aphecetche , would you know where to put this fix?

@ihrivnac ihrivnac force-pushed the mc-versions-update branch from 7df3914 to 266c1d0 Compare April 17, 2020 15:59
@ihrivnac
Copy link
Contributor Author

@ktf , the build/O2/alidist fails on some python issues, while there is nothing related python in this update. Then, the build/AliRoot/alidist log is systematically empty with each branch update. Could you take a look?

@ihrivnac ihrivnac force-pushed the mc-versions-update branch from 266c1d0 to dc70166 Compare April 20, 2020 08:29
@ihrivnac
Copy link
Contributor Author

@aphecetche ,
The build on Mac is now broken while it was passing fine before your CMake upgrade: AliceO2Group/AliceO2@7a6b188

Adding

# FIXME: this should be properly done by Geant4 itself instead 
find_package(VGM)
set_property(TARGET geant4 APPEND PROPERTY INTERFACE_LINK_DIRECTORIES $<TARGET_FILE_DIR:XmlVGM>)

in FindGeant4.cmake is wrong as Geant4 does not use VGM at all; VGM is used only by Geant4 VMC. After removing this code, the build goes fine on my MacOS, as well as o2sim_G4 test.

Can we remove this code? No other modifications will be needed for building with the new versions proposed in this PR.

@aphecetche
Copy link
Contributor

@ihrivnac will check further later on, as it may indeed be the case that those lines would be better in some FindGeant4_VMC.cmake but the point is that I did need them to have the o2sim_G4 tests running fine on Mac. And what I call running fine is in all of those situations :

  • with or without SIP disabled
  • within an env ALIBUILD_O2_TESTS=1 aliBuild --defaults build O2 run
  • from a BUILD/O2-latest/O2 directory and just doing ctest -R o2sim_G4 without setting any specific environment

@ihrivnac
Copy link
Contributor Author

@aphecetche , on my side (on Mac with SIP disabled, I believe) the ctest -R o2sim_G4 runs ok in the build directory without setting environment. Could you try out your tests with the change proposed above with this PR branch and let me know if you see any issues?
Note that in the new VMC packages versions included in this PR have CMake files improved.

@aphecetche
Copy link
Contributor

@ihrivnac will do. Might take some time as it means quite a few packages to rebuild ;-)

@aphecetche
Copy link
Contributor

@ihrivnac. I tested w/ SIP enabled and no env. and the o2sim_G4 tests indeed work well. So yes, you can remove those lines from FindGeant4.cmake

# FIXME: this should be properly done by Geant4 itself instead 
find_package(VGM)
set_property(TARGET geant4 APPEND PROPERTY INTERFACE_LINK_DIRECTORIES $<TARGET_FILE_DIR:XmlVGM>)

@ihrivnac
Copy link
Contributor Author

@aphecetche , thanks for checking. The PR is now open: AliceO2Group/AliceO2#3395

Copy link
Member

@ktf ktf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Genuine error.

@aphecetche
Copy link
Contributor

@ihrivnac indeed you are right. But then the test could be a little DRYier, using something like (not tested ) :

if(TARGET XmlVGM)
set(targetXml XmlVGM)
else if (TARGET VGM::XmlVGM)
set(targetXml VGM::XmlVGM)
endif()

if (targetXml)
set_target_properties(${targetXml}
PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
"${VGM_INCLUDE_DIRS}"
INTERFACE_LINK_DIRECTORIES
$&lt;TARGET_FILE_DIR:${targetXml}>)
endif()

@ihrivnac
Copy link
Contributor Author

ihrivnac commented Apr 23, 2020

If VGM is found the target should exist, and if it does not exist it is better to issue an error than do silently nothing. So I'd prefer the test with the version; but the code can be improved, eg.:

# VGM uses namespace since 4.7
if(${VGM_VERSION} VERSION_LESS "4.7")
  set(targetVGM XmlVGM)
else()
  set(targetVGM VGM::XmlVGM)
endif()

set_target_properties(${targetVGM}
                      PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
                                "${VGM_INCLUDE_DIRS}"
                      INTERFACE_LINK_DIRECTORIES
                                $<TARGET_FILE_DIR:${targetVGM}>)

ihrivnac added 11 commits April 26, 2020 10:57
  GEANT3: v3-0
  GEANT4: v10.5.1
  GEANT4_VMC: v5-0
  VGM: v4-7
  FairRoot: 4ab2d2b1f0392 (includes a fix in FindROOT.cmake)
- Keep previous Geant4 version for AliRoot (to use the same as in production):
  VGM v4-4
  GEANT4_VMC v3-6-p6-inclxx-biasing-p2
  GEANT4 v10.4.2
This should hopefully fix the JIRA issues O2-1029, O2-1044.
Withdraw dummy modification in order to trigger FairRoot revuild in CI
- Update VMC packages versions for patches:
  - geant3 version v2-7-p1 for Root 5 and v3-3 for Root 6
  - geant4_vmc v5-0-p2
- Avoid installing FairRoot in lib64 (what causes test failures on CC7)
  - added CMAKE_INSTALL_LIBDIR
@ihrivnac ihrivnac force-pushed the mc-versions-update branch from a2a2a74 to d0ffb25 Compare April 26, 2020 08:58
@ihrivnac
Copy link
Contributor Author

The fix in O2 CMake is now available AliceO2Group/AliceO2#3437. Running new test round after rebasing.

@ihrivnac
Copy link
Contributor Author

Now the only test failing is O2Suite/alidist_macOS, there is one test failing which however fails also in other PRs, so it's not related to the changes in this PR. The change requested by @MichaelLettrich is applied, also the issue pointed by @aphecetche is solved (in O2).
@ktf, can you merge?

cmake.sh Outdated
@@ -25,7 +25,7 @@ SET(Java_JAVAC_EXECUTABLE FALSE CACHE BOOL "" FORCE)
SET(BUILD_CursesDialog FALSE CACHE BOOL "" FORCE)

# OpenSSL is problematic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need to comment this out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added to fix the problem commented on 28 Feb.

@ihrivnac
Copy link
Contributor Author

@ktf, @michael,
The changes requested by @MichaelLettrich are applied, the change requested by @ktf is marked as resolved Is it now ok to merge or do you still need any action from me?

@ihrivnac
Copy link
Contributor Author

ihrivnac commented May 1, 2020

@ktf , the change you requested is marked as resolved. Can you approve?

@ktf
Copy link
Member

ktf commented May 2, 2020

All tests passed before my minor cosmetic cleanups. Merging.

@ktf ktf merged commit f909b5b into alisw:master May 3, 2020
@ktf
Copy link
Member

ktf commented May 3, 2020

Please, next time if you need to do such a large PR, make sure when you are happy you squash the commits and edit the commit message to the actual content of the PR. This time I did it, please check I did not miss anything.

@ihrivnac
Copy link
Contributor Author

ihrivnac commented May 4, 2020

@ktf, thanks for squashing. I use to do it just before merge to keep the list of commits available all time before merging. The commit message seems to me complete.

preghenella pushed a commit to alicetof/alidist that referenced this pull request Jul 13, 2020
* Update MC packages and FairRoot versions:
  GEANT3: v3-3
  GEANT4: v10.5.1
  GEANT4_VMC: v5-0-p2
  VGM: v4-7
  FairRoot: 4ab2d2b1f0392 (includes a fix in FindROOT.cmake)
* Keep previous Geant4 version for AliRoot (to use the same as in production):
  VGM v4-4
  GEANT4_VMC v3-6-p6-inclxx-biasing-p2
  GEANT4 v10.4.2
This should hopefully fix the JIRA issues O2-1029, O2-1044.
* Disable switching off OpenSSL
* Set old VMC packages versions in all defaults using ROOT 5
* Avoid installing FairRoot in lib64 (what causes test failures on CC7)
  - added CMAKE_INSTALL_LIBDIR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants