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

only fetch acts and compile it if not found in image #1649

Draft
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

tomeichlersmith
Copy link
Member

I am updating ldmx-sw, here are the details.

What are the issues that this addresses?

This does not have a corresponding issue here. I am following up with LDMX-Software/dev-build-context#115 while preparing for a v5 release of the ldmx/dev image.

Check List

  • I successfully compiled ldmx-sw with my developments. I test compiled with the newer image that has Acts locally. The CI will check that these developments work with the v4.2.2 image that does not have Acts.
  • I read, understood and follow the coding rules.
  • I ran my developments and the following shows that they are successful.

@tvami
Copy link
Member

tvami commented Mar 11, 2025

Isn't the solution to move those virtual destructors to be non virtual (even tho they are supposed to be virtual but given that ACTS didnt do them virtual...)?

@tomeichlersmith
Copy link
Member Author

That would be the solution, but that would involve editing Acts and I do not want to do that. There are also other warnings (turned errors) from Acts that are seen by the compiler that we are ignoring with the same SYSTEM strategy. I'm just localizing the SYSTEM include to be more specific.

include_directories(SYSTEM Tracking/acts/Core/include)

@tvami
Copy link
Member

tvami commented Mar 11, 2025

Yeah I agree that editing ACTS is beyond what we should do, but the earlier warning I saw in your original commit were about the inherited class which we have control over. It's true that I didnt look at the full list, so maybe there is some stuff that we have no control over... in that case going back to the SYSTEM approach is fine

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.

2 participants