Mu2e / REve

Eve-7 based event display
Apache License 2.0
0 stars 11 forks source link

Preparation for art v3_15_00 #151

Closed kutschke closed 3 months ago

kutschke commented 4 months ago

This PR is a companion to Offline PR 1257. The two can be merged independently.

Make changes to that are required for art suite v3_15_00 that upgrades root to v6_30_06. The submitted code builds with both the new art suite and the with the current art suite v3_14_03. I have not yet run-time tested the REve changes.

In most cases the problem was that fhicl code was exposed to dictionary makers because of people having includes that were either unneeded or were in the .hh file rather than the .cc file, where they belonged. I don't understand the details of what in fhicl is causing the problem but there is no reason for fhicl code to be exposed to dictionary makers.

The one special case was inc/CollectionFiller.hh. In this case I used #ifndef __ROOTCLING__ to hide the offending code from root.

There is one more commit coming about dealing with the movement of some classes from ROOT::Experimental to ROOt:: . I am still working on it and this will be in draft status until it is resolved.

kutschke commented 4 months ago

@sophiemiddleton when you get a chance, please to a test merge in your own area and verify that my use of #ifndef __ROOTCLING__ in inc/CollectionFiller.hh did not break anything. I am not sure how to test it. I expect that it will be OK.

sophiemiddleton commented 4 months ago

Yes I will do that before merging

kutschke commented 4 months ago

@sophiemiddleton The CollectionFiller class is included in classes_def.xml and classes.h. I am curious why it needs to be. Do you call it from the GUI or command line?

sophiemiddleton commented 4 months ago

Its used to pass in the arguments from fcl

kutschke commented 4 months ago

Its used to pass in the arguments from fcl

Perhaps I am missing something really simply. What functionality exists that requires that CollectionFiller be visible to genreflex? Is there a way to change values inside of the instance that lives in REveEventDisplay_module from the root command line or the GUI?

kutschke commented 4 months ago

There is the explanation from Kyle about why changes to fhiclcpp trigger the error on this release:

with art 3.15, we are using C++20 concepts to constrain the types of arguments allowed for function templates. ROOT's genreflex and rootcling functionalities do not yet understand C++20 concepts; hence the failures. I'm glad you were able to get around the issue–we've tried to avoid using C++20 concepts in places where dictionaries will be generated...but that does require some reworking of code.

SInce the code builds with my changes it appears that we have no other fhicl code exposed to dictionary builders.

kutschke commented 4 months ago

My updates to the PR are in. Modulo responding to comments and requests, this is done. The most recent changes are

  1. Revert the use of ROOTCLING and instead remove CollectionFiller from classes.h and classes_def.xml. I don't see why they would be needed there.
  2. I added code to src/SConstruct to define the C preprocessor variable NUMERIC_ROOT_VERION that holds the UPS root version transformed into a pure number. This is passed into make_mainlib and make_plugins. I don't think it's needed in make_dict_and_map.
  3. I added a test in REveEventDisplay_module.cc to choose the correct namespace for RWebWindowsManager depending on the the ROOT version encoded in the c preprocessor varaible.

You need to put the test of NUMERIC_ROOT_VERION after the #include to defines the class. I suggest putting it after all includes. A corollary is that we cannot make one file with all of the usings - we need to put them in files as needed. If it's only needed in a .cc file, put it there, not in the .hh. Of course there will be times when it is needed in the .hh

Sophie: please do a test merge and check if everything behaves as expected.

kutschke commented 4 months ago

@rlcee Please have a look at the machinery of NUMERIC_ROOT_VERION . Is this a viable solution moving forward? Can we do it better?

sophiemiddleton commented 3 months ago

@kutschke this is currently a draft, let me know when it moves to a real PR

kutschke commented 3 months ago

@kutschke this is currently a draft, let me know when it moves to a real PR

It's ready to do. I just un-drafted it.

kutschke commented 3 months ago

I see there are conflicts - I will look into it after the meeting

kutschke commented 3 months ago

I rebased on top of the new HEAD of main. This overwrote the commit history in the commit tab above.

To push to my fork I had to add the --force-with-lease option to my push command.

This PR builds with both art v3_14_03 and v3_15_00 on SL7.