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.

Sign up for GitHub

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

Jump to bottom

Delete the object held by gROOT after we reset the globals. #13463

Merged
merged 1 commit into from
Aug 16, 2023
Merged

Delete the object held by gROOT after we reset the globals. #13463

merged 1 commit into from
Aug 16, 2023

Conversation

pcanal
Copy link
Member

@pcanal pcanal commented Aug 16, 2023 •
edited
Loading

For example this allows unique_ptr to be deleted first and thus inform TROOT if need be

Fix #13462

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/default
How to customize builds

@github-actions
Copy link

github-actions bot commented Aug 16, 2023 •
edited
Loading

Test Results

       3 files         3 suites   13h 35m 28s ⏱️
2 435 tests 2 435 ✔️ 0 💤 0 ❌
7 239 runs  7 239 ✔️ 0 💤 0 ❌

Results for commit 84c704b.

♻️ This comment has been updated with latest results.

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

jalopezg-git
jalopezg-git reviewed Aug 16, 2023
View reviewed changes
Copy link
Collaborator

@jalopezg-git jalopezg-git left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The fix makes a lot of sense 👍! I guess it actually fixes #13462, right? Also, this should probably be backported to v6-28-00-patches.

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/default
How to customize builds

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/default
How to customize builds

jalopezg-git
jalopezg-git approved these changes Aug 16, 2023
View reviewed changes
Copy link
Collaborator

@jalopezg-git jalopezg-git left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, just some minor fixes on the comments! I think we want this in v6-28-00-patches too.

(I think this shouldn't break anything, but let's also wait for the CI.)

core/base/src/TROOT.cxx Outdated Show resolved Hide resolved
core/base/src/TROOT.cxx Outdated Show resolved Hide resolved
@hahnjo
Copy link
Member

hahnjo commented Aug 16, 2023

We might want to task CMS to test this one, just to check if it fixes #13429...

@phsft-bot
Copy link
Collaborator

Build failed on mac11/noimt.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Collaborator

Build failed on windows10/default.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

@aandvalenzuela
Copy link

We might want to task CMS to test this one, just to check if it fixes #13429...

@hahnjo let's wait until we merge the latest ROOT update in cms-sw/cmsdist#8650 (hopefully in a couple of hours) and I will start the testing. Thanks!

@hahnjo
Copy link
Member

hahnjo commented Aug 16, 2023

@pcanal can we have a test for this? This sounds pretty terrible to get wrong...

@aandvalenzuela aandvalenzuela mentioned this pull request Aug 16, 2023
Closed
@pcanal
Delete the object held by gROOT after we reset the globals.
bcc37fb 
For example this allows unique_ptr to be deleted first and thus inform TROOT if need be.

Fix root-project#13462
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/default
How to customize builds

@pcanal pcanal merged commit 2f78d2a into root-project:master Aug 16, 2023
7 of 12 checks passed
@hahnjo
Copy link
Member

hahnjo commented Aug 17, 2023

@pcanal can we have a test for this? This sounds pretty terrible to get wrong...

ping? Unless I'm blind, I don't think we have a test for this...

@pcanal
Copy link
Member Author

pcanal commented Aug 17, 2023

Not yet :)

@pcanal
Copy link
Member Author

pcanal commented Aug 17, 2023

It is at root-project/roottest#1002

@pcanal pcanal mentioned this pull request Aug 18, 2023
Open
@pcanal
Copy link
Member Author

pcanal commented Aug 18, 2023

@hahnjo let's wait until we merge the latest ROOT update in cms-sw/cmsdist#8650 (hopefully in a couple of hours) and I will start the testing. Thanks!

@aandvalenzuela Did you get a chance to test?

@aandvalenzuela
Copy link

@hahnjo let's wait until we merge the latest ROOT update in cms-sw/cmsdist#8650 (hopefully in a couple of hours) and I will start the testing. Thanks!

@aandvalenzuela Did you get a chance to test?

Yes! I tested it at cms-sw#186, but unit test failures are still present

@pcanal
Copy link
Member Author

pcanal commented Aug 18, 2023

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jalopezg-git jalopezg-git jalopezg-git approved these changes

@Axel-Naumann Axel-Naumann Awaiting requested review from Axel-Naumann

Assignees

@pcanal pcanal

Labels
in:Core Libraries
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Segmentation violation for a trivial std::unique_ptr
5 participants
@pcanal @phsft-bot @hahnjo @aandvalenzuela @jalopezg-git