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

Use 'declarationFile' instead of #include in CovMatrix component #308

Merged
merged 5 commits into from
Jun 3, 2024

Conversation

m-fila
Copy link
Contributor

@m-fila m-fila commented May 17, 2024

BEGINRELEASENOTES

  • Use declarationFile instead of include in CovMatrix to fix generated documentation for covariance matrix components. Fixes #296

ENDRELEASENOTES

Closes #296

Declarations added with #include were not appearing in a documentation generated with doxygen. New podio direcative 'declarationFile' (AIDASoft/podio#601) includes these declaration during file generation without relaying on pre-processor and make them visible to doxygen

Old New
Screenshot_2024-05-01_14-27-21 covmatrix_fix

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.

As discussed in the podio PR, we should probably make the CovMatrixCommon.ipp retrigger CMake by adjusting one of the CMakeLists.txt

@tmadlener
Copy link
Contributor

I think we also need to install the extra_code directory into <prefix>/share (or wherever we put the yaml file exactly). Otherwise downstream models cannot use EDM4hep as an upstream. See, e.g. https://github.com/key4hep/EDM4hep/actions/runs/9205295127/job/25320679012?pr=308#step:4:1220

@m-fila
Copy link
Contributor Author

m-fila commented May 23, 2024

The extra_code is installed in <prefix>/share/edm4hep/edm4hep/extra_code to mimic the path in source tree. Or in the source tree it could be move from edm4hep/extra_code to the top level and then installed in <prefix>/share/edm4hep/extra_code

@m-fila m-fila marked this pull request as ready for review May 23, 2024 11:49
@tmadlener tmadlener changed the title Use 'declarationFile' instead of #include in CovMatrix Use 'declarationFile' instead of #include in CovMatrix component Jun 3, 2024
@tmadlener tmadlener merged commit c4160ee into key4hep:main Jun 3, 2024
8 of 11 checks passed
@m-fila m-fila deleted the extracode branch June 5, 2024 09:25
@hegner
Copy link
Contributor

hegner commented Jun 13, 2024

@m-fila this PR destroyed all automatic testing because the keyword DEPENDS is interpreted as value of PODIO_IO_HANDLERS. Can you prepare a fix?
ping @tmadlener

@tmadlener
Copy link
Contributor

As far as I can tell only those workflows which build on an old(ish) version of podio are currently broken. However, there are multiple small changes in EDM4hep that depend on features of podio that came after v00.99

@m-fila
Copy link
Contributor Author

m-fila commented Jun 13, 2024

this should break only the Key4hep build / build (release, ...) checks. Do you mean there are some other checks failing or should I fix the build (release, ...) checks? If the second is the case I don't really see any solutions other than reverting this, but then these test will fail due to some other changes (failing since the introduction of interfaces)

@tmadlener
Copy link
Contributor

I don't think it's worth fixing the release builds. They effectively just have a podio version that is no longer sufficient. We could put a version requirement into the top level CMakeLists.txt if we wanted to, to make the tests fail earlier. However, since podio v1.0 is very close, I think we just wait with that until we have that.

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.

Missing methods in doxygen documentation for CovMatrix classes
3 participants