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.

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

Copy cafe to build directory so we don't have to run mrbslp #110

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

jedori0228
Copy link
Contributor

@jedori0228 jedori0228 commented Jun 20, 2024

cafe script is not seen from the build directory, but only copied to localProducts during the install step. mrbsetenv appends the build directory to ${PATH}, so users have to run mrbslp to use localProducts instead. (and then had to run mrbsetenv to compile the code, and then mrbslp to run cafe...)
This fix copies the script to build directory and then copies it again to localProducts during install, so can run cafe without running mrbslp.

@jedori0228 jedori0228 marked this pull request as draft June 20, 2024 15:50
@jedori0228 jedori0228 marked this pull request as ready for review June 20, 2024 17:26
Copy link
Member

@PetrilloAtWork PetrilloAtWork left a comment

Choose a reason for hiding this comment

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

The PR clarifies well the need for the first change, but I miss the explanation for the second one.

@@ -98,7 +98,7 @@ void load_cafana_libs()
gROOT->Macro("${SBNANA_FQ_DIR}/bin/rootlogon.C");
}
else{
gROOT->Macro("${MRB_BUILDDIR}/sbnana/bin/rootlogon.C");
gROOT->Macro("${SBNANA_DIR}/sbnana/CAFAna/rootlogon.C");
Copy link
Member

Choose a reason for hiding this comment

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

This new version appears to mirror the setup strategy in cafe script.
Nevertheless I do not understand the motivation of this change. Can you explain?

Copy link
Contributor Author

@jedori0228 jedori0228 Jul 3, 2024

Choose a reason for hiding this comment

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

Without this PR, users ran mrbslp before they run cafe. ${SBNANA_FQ_DIR} is only set after mrbslp, and for this case it looks for "${SBNANA_FQ_DIR}/bin/rootlogon.C" which exists. This PR helps user to run cafe without running mrbslp, and there we have empty ${SBNANA_FQ_DIR}. I then found we don't have "${MRB_BUILDDIR}/sbnana/bin/rootlogon.C", but it exists under "${SBNANA_DIR}/sbnana/CAFAna/rootlogon.C"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While checking more on this, it looks like just using tagged sbnana with setup sbnana v09_75_03 -qe20:prof does not suffer from this issue, where most of the users (I guess) check out the product and use local products

Copy link
Member

Choose a reason for hiding this comment

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

If you setup sbnana (either explicitly or indirectly with mrbslp), SBNANA_FQ_DIR is set so this part of the code would be skipped. That could explain your last statement.

@ibsafa
Copy link
Contributor

ibsafa commented Oct 18, 2024

@jedori0228 @PetrilloAtWork What's the status of this PR? I'm preparing a release and would like to include all open PRs that can be pushed. let me know!

@ibsafa
Copy link
Contributor

ibsafa commented Jan 23, 2025

is this PR still needed? @jedori0228 ?

@jedori0228
Copy link
Contributor Author

It is not necessary, but at leaset helping me a lot.. I don't have to keep doing mrbslp and mrbsetenv between running the script and compiling.

@PetrilloAtWork
Copy link
Member

@jedori0228, have you considered using ALWAYS_COPY for rootlogon.C too? from the pieces I gather here, it looks like it would make that file available in exactly the place where the existing load_cafana_libs.C expects it.

@PetrilloAtWork
Copy link
Member

I am not sure which is the better solution, actually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants