Skip to content

Do not execute the test case that assumes geometry is there if there is no geometry #18639

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

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

Conversation

dpiparo
Copy link
Member

@dpiparo dpiparo commented May 7, 2025

Noticed while doing unrelated work.

@ferdymercury
Copy link
Collaborator

Thanks!!

There is this other file using unguarded geom:

main/src/g2root.f

Maybe the easiest way could be to change:

gSystem->Load("libGeom");

with

R_ASSERT(gSystem->Load("libGeom") >= 0); // g2root needs ROOT built with geom active

Copy link

github-actions bot commented May 7, 2025

Test Results

    18 files      18 suites   3d 20h 29m 39s ⏱️
 2 733 tests  2 733 ✅ 0 💤 0 ❌
48 031 runs  48 031 ✅ 0 💤 0 ❌

Results for commit 025e5ac.

@dpiparo
Copy link
Member Author

dpiparo commented May 7, 2025

Hi @ferdymercury : thanks a lot. I would maybe build the executable only if the geom is active, would that maybe be a solution too?

@ferdymercury
Copy link
Collaborator

Hi @ferdymercury : thanks a lot. I would maybe build the executable only if the geom is active, would that maybe be a solution too?

Thanks to you. Mmm not sure, according to https://linux.die.net/man/1/g2root
g2root is only creating a text C++ macro, not really running ROOT. So even if geom is not installed, g2root should be able to work without problems. Now if someone gets that macro, sends it over SSH to another computer, and that other computer has no geom, it should get the error. That's why I was suggesting to edit the created C++ test macro with a more sensible error message. (Otherwise you would get gGeoManager error not defined.)

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants