-
Notifications
You must be signed in to change notification settings - Fork 14
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
Accommodate separate lmod, tcl modules.yaml's #329
Accommodate separate lmod, tcl modules.yaml's #329
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look okay to me, but I haven't tested. I'm relying on the CI testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks. Any possibilities to include it in the unit tests here and also in the spack-stack unit tests?
@climbfuji I don't see the jcsda-emc unit tests getting run anywhere, am I missing something? I added a line to unit_tests.yaml just now that I think will run them. EDIT: nevermind, got it :) |
4be24a2
to
b6a9748
Compare
This looks great, I am testing this now on my macOS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw the same thing in my testing, but it didn't seem to negatively impact anything. Is it breaking the test environment/keeping you from installing? |
I haven't tried that, but I know we added some logic for the containers previously (I might have been able to remove that in the last container PR, don't remember) that replaced the double colons. I can give that a try, but it would be cleaner if we didn't have these malformatted yaml files. |
Okay. I'd prefer to leave as-is if we can, but if needed we can always run a search and replace on the output file to get rid of the apostrophes. |
I found it, it’s still there. Check https://github.com/JCSDA/spack/blob/8296fcf898cb2997ad309273ab8480114c134bca/lib/jcsda-emc/spack-stack/stack/container_env.py#L52-L57 <https://github.com/JCSDA/spack/blob/8296fcf898cb2997ad309273ab8480114c134bca/lib/jcsda-emc/spack-stack/stack/container_env.py#L52-L57> it’s actually quite simple. Any chance you can add that?
… On Oct 10, 2023, at 4:53 PM, Alex Richert ***@***.***> wrote:
Okay. I'd prefer to leave as-is if we can, but if needed we can always run a search and replace on the output file to get rid of the apostrophes.
—
Reply to this email directly, view it on GitHub <#329 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5C2RJ6FKOVP6R6TBIX6SDX6XGYPAVCNFSM6AAAAAA5MZVQUCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJWGM4TOMRVGE>.
You are receiving this because you were mentioned.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks very much for this. Works like a charm on my macOS laptop.
Description
This PR updates the 'create env' command in order to support the separation of common/modules.yaml into lmod and tcl ones.
Issue(s) addressed
Addresses JCSDA/spack-stack#703, JCSDA/spack-stack#50
Dependencies
none
Checklist