root [RF] Avoid using transient `std::unique

Harmless change to circumvent errors like these when building RooFit standalone:

Error in <TProtoClass::FindDataMember>: data member with index 0 is not found in class tuple<TreeReadBuffer*,default_delete<TreeReadBuffer> >
Error in <CreateRealData>: Cannot find data member # 0 of class tuple<TreeReadBuffer*,default_delete<TreeReadBuffer> > for parent RooAddPdf!
Error in <TProtoClass::FindDataMember>: data member with index 1 is not found in class tuple<TreeReadBuffer*,default_delete<TreeReadBuffer> >
Error in <CreateRealData>: Cannot find data member # 1 of class tuple<TreeReadBuffer*,default_delete<TreeReadBuffer> > for parent RooAddPdf!
Error in <TProtoClass::FindDataMember>: data member with index 0 is not found in class tuple<TreeReadBuffer*,default_delete<TreeReadBuffer> >
Error in <CreateRealData>: Cannot find data member # 0 of class tuple<TreeReadBuffer*,default_delete<TreeReadBuffer> > for parent RooPolynomial!

Jun 04 '24 09:06 guitargeek

Test Results

13 files 13 suites 3d 5h 2m 16s :stopwatch: 3 030 tests 3 030 :white_check_mark: 0 :zzz: 0 :x: 33 869 runs 33 869 :white_check_mark: 0 :zzz: 0 :x:

Results for commit ed22e9b9.

:recycle: This comment has been updated with latest results.

Jun 04 '24 11:06 github-actions[bot]

Wait, this should have been fixed by https://github.com/root-project/root/pull/13900. If that's not the case, we need to understand why. FYI @pcanal .

Jun 04 '24 13:06 vepadulano

From a practical point of view for RooFit, the fact that the problem is not understood should not block this PR to enter the next patch release, unless the underlying problem is fixed in the meantime.

I'll try to create an easy reproducer and open a GitHub issue!

Jun 04 '24 16:06 guitargeek

The original issue (which was "fixed" by also removing the transient unique_ptr because it was blocking the transition to C++17) is at https://github.com/root-project/root/issues/13361

I couldn't create a standalone reproducer back then, also we should probably reintroduce the transient unique_ptr in the RBrowserData class and see what happens.

All of this to say, I agree with you, it's outside of scope for this PR

Jun 04 '24 16:06 vepadulano

Harmless change to circumvent errors like these when building RooFit standalone:

Can you remind me how to reproduce this (as you saw it, no need to reduce)?

Jun 04 '24 16:06 pcanal

You need to compile this code with the 3rd commit in the history reverted and run the tests (just like the CI does): https://github.com/guitargeek/roofit/commits/main/

But you need a ROOT build without RooFit for that (roofit=OFF)

Jun 04 '24 16:06 guitargeek

Thanks. I am able reproduce that error. I am investigating.

Jun 05 '24 14:06 pcanal

@vepadulano, @pcanal, this PR has already missed the 6.32.02 patch release. Can we merge it so at least this fix is sure to be in the next patch release? The investigation of the problem is independent of this PR I think.

Jun 27 '24 07:06 guitargeek

This PR is a 'downgrade' is code simplicity that should not be necessary. However the underlying fix is not ready (and won't until August), so we could temporarily merge this if need be.

Jun 27 '24 10:06 pcanal

Ok, thanks for the comment! Let's see when the next patch release will be scheduled

Jun 27 '24 13:06 guitargeek

The 6.32.04 release is planned in about 10 days, according to @dpiparo.

@pcanal, will there be a fix for the underlying issue by then, or should be just merge this PR?

Aug 02 '24 13:08 guitargeek

Checking ...

Aug 02 '24 13:08 pcanal

@guitargeek Potential good news. I am currently unable to reproduce the problem with the master nor with the tip of v6-32-00-patches. Can you please verify? Thanks.

Aug 07 '24 18:08 pcanal

@guitargeek Nevermind. I just realized that I can still see the problem when running some of the test :(

Aug 07 '24 20:08 pcanal

https://github.com/root-project/root/pull/16202 should fix this problem. Can you please check?

Aug 08 '24 23:08 pcanal

I see the CI in that PR is still red. Should I wait for the PR to be updated such that it's green before I test? Or does it make sense to test already now?

Thanks a lot already for fixing it :+1:

Aug 09 '24 11:08 guitargeek

It makes sense to test it now to know at the very least if my independent tests are sufficient or not.

Aug 09 '24 12:08 pcanal

Perfect! It works!

Aug 10 '24 13:08 guitargeek

Superseded by a PR that fixes the underlying problem:

https://github.com/root-project/root/pull/16202

Aug 12 '24 11:08 guitargeek

Re-opening, because this should be merged in case #16202 doesn't get ready for the 6.34 release.

Sep 14 '24 15:09 guitargeek

[RF] Avoid using transient `std::unique_ptr`

Test Results