Skip to content

[CMake] Avoid installing unneeded files with gnuinstall=OFF #19473

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

guitargeek
Copy link
Contributor

More detail in the commit descriptions.

@guitargeek guitargeek self-assigned this Jul 29, 2025
@guitargeek guitargeek requested a review from bellenot as a code owner July 29, 2025 15:01
@guitargeek guitargeek added in:Build System clean build Ask CI to do non-incremental build on PR labels Jul 29, 2025
Comment on lines -18 to -23
if(NOT gnuinstall)
install(DIRECTORY gdml/ DESTINATION geom/gdml
FILES_MATCHING PATTERN "*.py"
PATTERN "inc" EXCLUDE
PATTERN "src" EXCLUDE)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

It looks good in principle, but do we know where these scripts are used, if at all? What purpose do they serve being only available in the build directory? I guess it would only be fine if they were used in some tests alone and they're not supposed to be exposed to users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. Maybe @agheata has an idea?

Copy link
Member

Choose a reason for hiding this comment

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

What purpose do they serve being only available in the build directory?

This is not how I read the install invocation. I read it as installing this file 'only' in the install directory (and not in the build directory) but .... only if not using the gnuinstall layout.

but do we know where these scripts are used, if at all?

We don't know if they are used. However according to the doc geom/gdml/doc/index.md those are used to convert TGeo objects to gdml from python.

So if anything it is surprising that they are not installed in all cases. In a related observation they are not tested by ctest at the moment ... Maybe Witek has some left over tests?

Copy link

github-actions bot commented Jul 29, 2025

Test Results

    21 files      21 suites   3d 15h 56m 22s ⏱️
 3 375 tests  3 375 ✅ 0 💤 0 ❌
69 164 runs  69 164 ✅ 0 💤 0 ❌

Results for commit 6985d73.

♻️ This comment has been updated with latest results.

The geometry packages put some almost 20 year old scripts into the
install prefix:
```txt
geom/
└── gdml
    ├── doc
    ├── ROOTwriter.py
    └── writer.py
```
It's not clear why they should be there, and they are already excluded
when building ROOT with `gnuinstall=ON`. For more consistent builds
between the different options, I think it's better if this is never
installed.
This was still relevant for the external `roottest` builds, but since
that project was absorbed into ROOT itself, there is not need for these
files anymore.
@guitargeek guitargeek force-pushed the gnuinstall_makefile branch from 7fe8b57 to 6985d73 Compare August 17, 2025 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean build Ask CI to do non-incremental build on PR in:Build System
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants