Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: explicitly use SELECTION_XML in OUTPUT_FOLDER #318

Merged
merged 4 commits into from
Aug 5, 2022

Conversation

wdconinc
Copy link
Contributor

@wdconinc wdconinc commented Aug 3, 2022

After checking that ${ARG_OUTPUT_FOLDER}/${SELECTION_XML} exists, we should not simply pass ${SELECTION_XML} to PODIO_GENERATE_DICTIONARY, but pass the full path for when ${ARG_OUTPUT_FOLDER} is not ${CMAKE_CURRENT_SOURCE_DIR}.

BEGINRELEASENOTES

  • CMake: PODIO_ADD_ROOT_IO_DICT: Bugfix for data models in OUTPUT_FOLDER not equal to source dir in root dictionary generation cmake macro.
    • Now SELECTION_XML can be passed either as absolute path or relative to OUTPUT_FOLDER.

ENDRELEASENOTES

@tmadlener
Copy link
Collaborator

Good catch. Thank you.
Tow minor things (I can also fix those, just let me know. I should be able to push directly to the PR branch usually)

  • Could you also update the docstring of the macro to include that SELECTION_XML is relative to OUTPUT_FOLDER?
  • The podio internal tests of the extenstion datamodel also do not seem to get this correct here:

    podio/tests/CMakeLists.txt

    Lines 199 to 200 in d0d4f67

    PODIO_ADD_ROOT_IO_DICT(ExtensionDataModelDict ExtensionDataModel "${ext_headers}" ${CMAKE_CURRENT_SOURCE_DIR}/extension_model/src/selection.xml
    OUTPUT_FOLDER ${CMAKE_CURRENT_SOURCE_DIR}/extension_model)

@wdconinc
Copy link
Contributor Author

wdconinc commented Aug 4, 2022

Could you also update the docstring of the macro to include that SELECTION_XML is relative to OUTPUT_FOLDER?

We could also allow both relative and absolute paths to selection file and use cmake to figure which it is, similar to https://github.com/AIDASoft/podio/blob/master/cmake/podioMacros.cmake#L29-L34.

The other approach could be to change the test for existence of the selection.xml in https://github.com/AIDASoft/podio/blob/master/cmake/podioMacros.cmake#L273-L276 to something like

  IF (NOT EXISTS ${ARG_OUTPUT_FOLDER}/${SELECTION_XML}
  AND NOT EXISTS ${SELECTION_XML})
    MESSAGE(STATUS "Not adding the ROOT dictionary to the targets because the selection.xml has not been generated")
    RETURN()
  ENDIF()

I'll let you fix this in whichever way you think best.

wdconinc and others added 2 commits August 5, 2022 08:47
After checking that `${ARG_OUTPUT_FOLDER}/${SELECTION_XML}` exists, we should not simply pass `${SELECTION_XML}` to `PODIO_GENERATE_DICTIONARY`, but pass the full path for when `${ARG_OUTPUT_FOLDER}` is not `${CMAKE_CURRENT_SOURCE_DIR}`.
@tmadlener tmadlener force-pushed the selection-xml-in-output-folder branch from 1835614 to 86d03cb Compare August 5, 2022 07:06
Not generating the selection.xml is be a valid usecase
@tmadlener
Copy link
Collaborator

Thanks for the IS_ABSOLUTE hint. Now SELECTION_XML can either be absolute or relative to OUTPUT_FOLDER. Also updated the documentation and made the status message a bit more informative.

@tmadlener
Copy link
Collaborator

dev4/x86_64-centos7-gcc11-opt seems to be broken entirely, we pick up the default cmake from CentOS7.

@tmadlener tmadlener merged commit 548aa47 into AIDASoft:master Aug 5, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants