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

Remove the depracated warning when using gconfig.C #1011

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from
Draft

Remove the depracated warning when using gconfig.C #1011

wants to merge 3 commits into from

Conversation

karabowi
Copy link
Collaborator

Use the FairVMCConfig in the simulation macros.

Addresses issue #1010.

Checklist:

[ X ] Rebased against dev branch
[ X ] My name is in the resp. CONTRIBUTORS/AUTHORS file
[ X ] Followed the seven rules of great commit messages

@karabowi karabowi force-pushed the gconfig_fix branch 8 times, most recently from 5986b68 to b9c0dad Compare September 23, 2020 09:04
@dennisklein dennisklein added this to the v19.0 milestone Mar 8, 2021
@dennisklein
Copy link
Member

Please rebase on current dev, thx!

@dennisklein dennisklein marked this pull request as draft March 8, 2021 16:16
@dennisklein dennisklein linked an issue Mar 8, 2021 that may be closed by this pull request
[WARN] FairGenericVMCConfig::Setup() Using gConfig.C macro DEPRACATED. #1010
Open
@karabowi karabowi force-pushed the gconfig_fix branch 2 times, most recently from 3715d06 to 453ab6c Compare April 22, 2021 10:07
@dennisklein dennisklein marked this pull request as ready for review July 23, 2021 13:43
dennisklein
dennisklein previously approved these changes Jul 23, 2021
View reviewed changes
Copy link
Member

@dennisklein dennisklein 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.

I took the liberty and removed the commits that added and removed stuff (with the netto result of doing nothing). Also rebased to dev and renamed one commit from Add missing library to Tune MQ pixelDetector example output.

@dennisklein dennisklein force-pushed the gconfig_fix branch 3 times, most recently from 7c189ff to 453ab6c Compare July 24, 2021 23:07
dennisklein
dennisklein reviewed Jul 24, 2021
View reviewed changes
Copy link
Member

@dennisklein dennisklein 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.

I reverted my changes, because the PR turned out not quite ready for merging.

  • See inline comments
  • Please clean the commits from changes that are introduced and again removed within this PR.
  • Two following commits have the same commit message, but one of them does not match the changes, please reword more appropriately.

(once done, press the button "Ready for review" at the bottom of this page right above the CI checks list)

examples/MQ/pixelDetector/macros/run_sim.C Outdated
@@ -49,6 +49,7 @@ void run_sim(Int_t nEvents = 10, TString mcEngine = "TGeant3", Int_t fileId = 0,
// ----- Create simulation run ----------------------------------------
FairRunSim* run = new FairRunSim();
run->SetName(mcEngine); // Transport engine
run->SetSimulationConfig(new FairVMCConfig());
Copy link
Member

Choose a reason for hiding this comment

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

The FairVMCConfig object appears to be leaking (not going to repeat this comment for the other places in this PR).

examples/MQ/pixelDetector/src/CMakeLists.txt Show resolved Hide resolved
@dennisklein dennisklein marked this pull request as draft July 24, 2021 23:45
@ChristianTackeGSI
Copy link
Member

The centos 7 CI tests fail, because they don't have yaml-cpp installed.

As Dennis already pointed out, we should get clear on how this should be handled?

@ChristianTackeGSI
Copy link
Member

Next step: Memory management.

If SetSimulationConfig gets an owning pointer (as all the proposed examples do), FairRunSim should clean up that memory.

My proposal would be to perform the same steps as for Sink/Source:
Use a unique_ptr inside FairRunSim, provide a new (additional) SetSimulationConfig API that also takes a unique_ptr, and rewrite the examples accordingly.
See for examples on how this could look like: #1194, #1200

@karabowi
Remove the depracated warning about using gconfig.C
db5050e 
Use the FairVMCConfig in the simulation macros.

Addresses issue FairRootGroup#1010.
@karabowi
Apply fixes after rebasing
61fb012 
Added yaml-cpp requirement to main CMakeLists.
Changed include order in Tutorial1 macro.
Removed endl from LOG(info) in FairYamlVMCConfig.
@karabowi
Use unique pointers for the simulation config
2c0dd25 
Changes the FairGenericVMCConfig and FairRunSim,
as well as macros/files using the config.
@dennisklein dennisklein modified the milestones: v19.0, v19.2 Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristianTackeGSI ChristianTackeGSI ChristianTackeGSI left review comments

@dennisklein dennisklein dennisklein left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
next
Development

Successfully merging this pull request may close these issues.

[WARN] FairGenericVMCConfig::Setup() Using gConfig.C macro DEPRACATED.
3 participants
@karabowi @dennisklein @ChristianTackeGSI