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

Restore compatibility with EDM4hep 0.10 #1380

Merged
merged 11 commits into from
Jan 16, 2025

Conversation

jmcarcell
Copy link
Member

@jmcarcell jmcarcell commented Jan 7, 2025

BEGINRELEASENOTES

  • Revert some of the changes of Fix a few compiler warnings #1375 to build a vector of floats instead of doubles since the necessary constructor doesn't exist in 0.10.X.
  • Use edm4hep::labels::CellIDEncoding only when they are available (starting at version 0.99)

ENDRELEASENOTES

See #1333 (comment)

Copy link

github-actions bot commented Jan 7, 2025

Test Results

   16 files     16 suites   5h 56m 5s ⏱️
  369 tests   366 ✅ 0 💤 3 ❌
2 904 runs  2 895 ✅ 0 💤 9 ❌

For more details on these failures, see this check.

Results for commit 1ef0a0b.

♻️ This comment has been updated with latest results.

DDDigi/io/DigiIO.cpp Outdated Show resolved Hide resolved
DDG4/edm4hep/Geant4Output2EDM4hep.cpp Outdated Show resolved Hide resolved
@tmadlener
Copy link
Contributor

Given the discussion in #1379. How hard would it be to setup a workflow that builds an older version of EDM4hep on the fly to make sure things still actually work?

@andresailer
Copy link
Member

related to the deprecation of a constructor

Can you add why these are reverted please!

@andresailer
Copy link
Member

Given the discussion in #1379. How hard would it be to setup a workflow that builds an older version of EDM4hep on the fly to make sure things still actually work?

Which old version of EDM4hep should we test against? Maybe it exists in one of the LCG releases.

@tmadlener
Copy link
Contributor

Which old version of EDM4hep should we test against?

Letting @wdconinc confirm, but I think 0.10.5 is what we should be aiming for.

@wdconinc
Copy link
Contributor

wdconinc commented Jan 7, 2025

Which old version of EDM4hep should we test against?

Letting @wdconinc confirm, but I think 0.10.5 is what we should be aiming for.

We are using 0.10.5 in EIC (because we cannot upgrade to acts-38 yet, which is the only version that supports 0.99). However, it should probably be 0.10, and not rely on a patch release.

@andresailer
Copy link
Member

@jmcarcell Could you cherry-pick #1381 please!

@tmadlener
Copy link
Contributor

Can we also put a the 0.10.5 (or whatever we settle on for CI) into the find_package(EDM4HEP) call in the appropriate CMakeLists.txt?

@wdconinc
Copy link
Contributor

wdconinc commented Jan 7, 2025

Can we also put a the 0.10.5 (or whatever we settle on for CI) into the find_package(EDM4HEP) call in the appropriate CMakeLists.txt?

Or, in the interest of forward compatibility, the same structure that's used for podio to allow both 0.99 and 1.0?

@jmcarcell jmcarcell changed the title Restore compatibility with EDM4hep 0.10.5 Restore compatibility with EDM4hep 0.10 Jan 8, 2025
.github/workflows/linux.yml Show resolved Hide resolved
DDDigi/io/DigiIO.cpp Outdated Show resolved Hide resolved
@tmadlener
Copy link
Contributor

It looks like DDDigi is also built with LCG_102 and LCG_101, but that doesn't yet have a recent enough version of EDM4hep.

@jmcarcell
Copy link
Member Author

jmcarcell commented Jan 10, 2025

This should be ready now.

Can we also put a the 0.10.5 (or whatever we settle on for CI) into the find_package(EDM4HEP) call in the appropriate CMakeLists.txt?

Or, in the interest of forward compatibility, the same structure that's used for podio to allow both 0.99 and 1.0?

I do not mind this, but maybe we can do it once we get closer to EDM4hep 1.0 since I'm not sure when it's going to happen, and probably more changes will be needed in DD4hep by then?

@tmadlener
Copy link
Contributor

probably more changes will be needed in DD4hep by then?

I think there will be potentially some more changes, similar to #1374 for the MCParticle. And there is still #1373. So at least from my point of view, I think we have still some PRs ahead of us where we can add the compatibility with 1.0.

@andresailer
Copy link
Member

Something in ROOT master broke

   	 62 - t_ClientTests_Check_Shape_PseudoTrap (Subprocess aborted)
  	 63 - t_ClientTests_Check_Shape_PseudoTrap1 (Subprocess aborted)
  	 64 - t_ClientTests_Check_Shape_PseudoTrap2 (Subprocess aborted)

@andresailer andresailer requested a review from tmadlener January 13, 2025 12:52
@andresailer andresailer self-requested a review January 13, 2025 12:52
Copy link
Member

@andresailer andresailer left a comment

Choose a reason for hiding this comment

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

Since test failures are unrelated

CMakeLists.txt Outdated Show resolved Hide resolved
@andresailer andresailer requested a review from tmadlener January 16, 2025 15:03
Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Minor nitpick from my side. We can also leave it in, it doesn't really hurt at this point.

DDG4/edm4hep/Geant4Output2EDM4hep.cpp Show resolved Hide resolved
@andresailer
Copy link
Member

I would like to merge this after it is done on LCG_104, so that I can test the reader on LCG_104 as well.

@andresailer andresailer merged commit 467b011 into AIDASoft:master Jan 16, 2025
12 of 16 checks passed
@tmadlener tmadlener mentioned this pull request Jan 17, 2025
2 tasks
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.

4 participants