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

Fixes for s132 (Don't include GeometryService in rootcling) #1329

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

eflumerf
Copy link
Contributor

Update CMakeLists.txt for new files.

Make it so check_cmake.sh can be run as ./bin/check_cmake.sh (no copy to base directory)

@FNALbuild
Copy link
Collaborator

Hi @eflumerf,
You have proposed changes to files in these packages:

  • MCDataProducts
  • Analyses
  • bin
  • TrkReco
  • STMMC
  • GeometryService

which require these tests: build.

@Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main.

⌛ The following tests have been triggered for ebf8786: build (Build queue is empty)

About FNALbuild. Code review on Mu2e/Offline.

@FNALbuild
Copy link
Collaborator

☀️ The build tests passed at ebf8786.

Test Result Details
test with Command did not list any other PRs to include
merge Merged ebf8786 at 25d66e0
build (prof) Log file. Build time: 04 min 12 sec
ceSimReco Log file.
g4test_03MT Log file.
transportOnly Log file.
POT Log file.
g4study Log file.
cosmicSimReco Log file.
cosmicOffSpill Log file.
ceSteps Log file.
ceDigi Log file.
muDauSteps Log file.
ceMix Log file.
rootOverlaps Log file.
g4surfaceCheck Log file.
FIXME, TODO TODO (0) FIXME (0) in 1 files
clang-tidy 0 errors 0 warnings
whitespace check no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at ebf8786 after being merged into the base branch at 25d66e0.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

@kutschke
Copy link
Collaborator

@eflumerf is the need to exclude the section of Surface steps drive by the new root in art 3.15 or is it driven by something intrinsic to cmake? My own opinion is that data products should not use services but we have never been able to get agreement on that.

@eflumerf
Copy link
Contributor Author

Basically, in s132, fhicl-cpp 4.19.00 includes c++20 code. ROOT cling does not understand c++20, so you cannot have a #include "fhiclcpp/ParameterSet.h" visible to any files which are processed by rootcling (i.e. included in a dictionary).

The change I made hides the GeometryService methods from rootcling, as they shouldn't be needed for object persistence in ROOT.

@eflumerf
Copy link
Contributor Author

I think the result of this change to fhiclcpp is that you cannot include services in data products until ROOT is updated to support c++20 in rootcling, which I think has an indefinite time-scale.

@kutschke
Copy link
Collaborator

I think the result of this change to fhiclcpp is that you cannot include services in data products until ROOT is updated to support c++20 in rootcling, which I think has an indefinite time-scale.

Thanks Eric. I will approve and merge the PR and create an issue to discuss getting services out of data products, which I think is the preferred solution.

Copy link
Collaborator

@kutschke kutschke left a comment

Choose a reason for hiding this comment

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

See the discussion in the Conversation tab about the use of services in data products. Eric's solution is good enough for now. I create issue #1330 to follow up.

I will merge now.

@kutschke kutschke merged commit b80ba8e into Mu2e:main Aug 22, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants