Address differences in ROOT5 and ROOT6 interfaces

[plexoos]

The main idea behind the proposed changes is to have the code that can seamlessly work with ROOT5 and ROOT6

[plexoos]

Sorry for adding another commit to this thread but I think it belongs here. I believe TVirtualMC is your domain @klendathu2k and @perevbnlgov

[plexoos]

Thanks very much for the feedback Jason ✌️

I note that TH1::Rebin is virtual in ROOT6, so does this solve a real problem? (I guess there is a compiler warning or other motivation here?)

Yes, the introduction of the new TH2* TH2::Rebin in ROOT6 causes a real error about not covariant return type of a virtual function.

#16 3360.1 In file included from .sl88_gcc789/OBJ/StRoot/StUtilities/StMultiH1F.cxx:13:0:
#16 3360.1 .sl88_gcc789/OBJ/StRoot/StUtilities/StMultiH1F.h:38:23: error: invalid covariant return type for 'virtual TH1* StMultiH1F::Rebin(Int_t, const char*, const Double_t*)'
#16 3360.1    virtual        TH1* Rebin(Int_t ngroup, const char* newname, const Double_t* xbins)

The old and new overriding chains are like this: TH1 TH1::Rebin -> TH1 StMultiH1F::Rebin :white_check_mark: vs TH1 TH1::Rebin -> TH2 TH2::Rebin -> TH1* StMultiH1F::Rebin :x: TH1 TH1::Rebin -> TH2 TH2::Rebin -> TH2* StMultiH1F::Rebin :white_check_mark:

[genevb]

I see....They added a Rebin() to TH2 in ROOT6 with a (slightly) different return type than the previously-inherited Rebin() from TH1. They didn't add one in TH3, so no impact on StMultiH2F. I don't think I (or anyone else) ever used the returned pointer, so I approve the change to StMultiH1F.h.

[genevb]

Hopefully understood, but worth re-iterating: merging any changes like these that have the potential to affect FastOffline should wait until at least a couple weeks after the data-taking ends.

[plexoos]

Hopefully understood, but worth re-iterating: merging any changes like these that have the potential to affect FastOffline should wait until at least a couple weeks after the data-taking ends.

Is there anything that would prevent us from creating a stable branch for fast offline and other online tools that would include only hand picked changes during the run? Sharing a common branch for bleeding edge changes is very common and in fact should be encouraged. On the other hand, preventing incremental development of new features during the run because the online chooses to use an unstable branch does not sound very productive.

[genevb]

Hi, Dmitri

If the S&C management and others in the Collaboration (e.g. subsystem software coordinators) want DEV to be built from some other branch than "main" during Run operations, we can have a switch for AutoBuild.

-Gene

[plexoos]

Hi Gene: When you refer to the 'main' branch do you think of it as a stable or development branch? Picking the definition based on a specific time of year is at least confusing.

[genevb]

Hi, Dmitri

If you're asking my perspective, I personally do not apply these terms to the code repository, but I do to the builds. DEV is a development build that we have leveraged for FastOffline during Run operations because we wanted the rapid code deployment scheme of DEV when necessary to support the Run operations, but that brings the requirement of being careful about what gets inserted by developers into the place from which DEV pulls its code. It is not a perfect scheme, and if you wish to change this established scheme, you need to have a clear proposal for how Run operations will be supported.

-Gene

On Jul 6, 2021, at 11:30 AM, Dmitri Smirnov @.**@.>> wrote:

Hi Gene: When you refer to the 'main' branch do you think of it as a stable or development branch? Picking the definition based on a specific time of year is at least confusing.

[plexoos]

Gene, the data taking is over. Are you ok with merging these changes now?

[plexoos]

If there are no further comments I would like to merge this PR. We need to move on with ROOT6

[genevb]

I suggest getting SL21c out the door first. -Gene

On Jul 13, 2021, at 10:52 AM, Dmitri Smirnov @.**@.>> wrote:

If there are no further comments I would like to merge this PR. We need to move on with ROOT6

[plexoos]

Good point. Agreed

[starsdong]

Gene, can you approve this PR (as you already indicated in the comments)? I think merging will happen automatically later.

[genevb]

Gene, can you approve this PR (as you already indicated in the comments)? I think merging will happen automatically later.

Aren't there already 2 approvals? I'd be interested to understand why additional approvals are needed if anyone knows why.

-Gene

[plexoos]

Aren't there already 2 approvals? I'd be interested to understand why additional approvals are needed if anyone knows why.

Yes, but to enable the merge button 2 approvals from code owers is required. The code owners are indicated by this shield lock icon

We have the corresponding flag set in the settings:

[genevb]

to enable the merge button 2 approvals from code owers is required.

That implies that the approvals from Jason and Dmitry K. were meaningless. Correct?

[plexoos]

That implies that the approvals from Jason and Dmitry K. were meaningless. Correct?

Can't speak for others but for me it is important to see the reaction from other contributors especially when I know that they spent time looking over the changes. So, definitely not meaningless.

[genevb]

Can't speak for others but for me it is important to see the reaction from other contributors especially when I know that they spent time looking over the changes. So, definitely not meaningless.

OK, reasonable point. I agree that I would've withheld my approval if there was lesser support from such others.

[plexoos]

Perhaps, I should have mentioned in the closing comment that tagging or branching can be done from any past commit in the existing history. So, any changes on the main branch after the tag or branching point can be ignored

[genevb]

Perhaps you could’ve waited just a few more hours (was it that urgent?) as we had agreed. Please let me know how to get the version hash from before today’s merges. Thanks,

-Gene

On Jul 19, 2021, at 11:54 AM, Dmitri Smirnov @.***> wrote:



Perhaps, I should have mentioned in the closing comment that tagging or branching can be done from any past commit in the existing history. So, any changes on the main branch after the tag or branching point can be ignored

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHubhttps://github.com/star-bnl/star-sw/pull/47#issuecomment-882661135, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AUK2OBPLDNPVVMNC6CFCYRDTYRDCVANCNFSM47TC5DPQ.

[plexoos]

There is no point in waiting if all we want to include in the release is already in the repo Hashes can be viewed on this web site or with git log Amol just asked me to help with the tagging. I am going to do it this time. The hash for SL21c tag will be 0d53a1a98fb3cadf755744f8d1e08cab089837dc

[veprbl]

We could make what @plexoos suggests to be more formal by designating a stabilization branch that could receive backports for only the release-critical fixes