-
Notifications
You must be signed in to change notification settings - Fork 37
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
Switch to templated links and remove explicit declarations from yaml file #373
Conversation
f5827f0
to
aa795e8
Compare
7c3d437
to
bdc2a68
Compare
bdc2a68
to
eb971fb
Compare
The Key4hep release workflows are failing because they don't come with a recent enough version of podio. |
The downstream-build workflow seems broken and it looks like it stumbles during parsing of the PR message? I will try to build this locally to check if there are any leftover Associations still in the stack. Otherwise I would like to get this merged rather soon. |
Nothing breaks for me locally, so I think this can be merged. Waiting another day to give everyone a chance to get a comment or question in. |
The downstream build should be fixed, parsing was almost removed but not quite because of some I was not sure about schema evolution being necessary here but 0.99 was already in July so it's all the files produced in the last 6 months with the nightlies or in the last months with the latest release 🤔 |
edm4hep/CMakeLists.txt
Outdated
target_include_directories(edm4hep_v2 PRIVATE | ||
$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/edm4hep/schema_evolution/include> | ||
) | ||
target_link_libraries(edm4hep_v2 PUBLIC podio::podio EDM4HEP::edm4hep) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not entirely sure about the naming of this library here. Maybe v2
gives the wrong impression? Ideally this would simply be merged into edm4hep
but the problem for that is that we still need a dedicated dictionary at the moment, because the selection.xml
for EDM4hep is generated and we cannot simply inject the few datatypes that would be necessary into that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe edm4hepOLE
(OldLinkEvolution)? or edm4hepOLD
(OldLinkData)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:D that would be some nice backronym in this case. Would be OK for me, but I am not sure we can find one for all future changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed it to edm4hepOldSchemas
which should hopefully be a somewhat future proof catch-all.
Thanks for the fix. Schema evolution is currently here to keep backwards compatibility, since we kind of advertised that this will be a transparent switch (and also to show that it is technically possible). I can have another look to see if it can squeeze it a bit more. In principle it is also possible to just drop that part some time in the future, when everyone is definitely on >= 1.0. |
BEGINRELEASENOTES
podio::Link
s for theLink
datatypesAssociation
headers that were put in place for a smooth transition toLink
sENDRELEASENOTES
This PR introduces all the existing
Link
datatypes as typedefs (keeping the same header file names for the collections as before) ofpodio::LinkCollection<ToT, FromT>
and removes their declarations from the yaml file. It also includes the necessary schema evolution code for podio to be able to read back data files that have been written with previous versions of EDM4hep (or at least they will once suitable files have been uploaded for testing).include/edm4hep
edm4hep/schema_evolution
Both of these can obviously be changed to something more appropriate if necessary.
links
category for the YAML definitions AIDASoft/podio#691