-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add new init scripts for new initialization module #668
Add new init scripts for new initialization module #668
Conversation
- zsh - fish
variable setting and calling
- ksh
- csh
Instance
|
Instance
|
Instance
|
init/bash
Outdated
export MODULEPATH=/cvmfs/software.eessi.io/versions/"$EESSI_VERSION"/init/modules | ||
. /cvmfs/software.eessi.io/versions/"$EESSI_VERSION"/compat/linux/$(uname -m)/usr/share/Lmod/init/bash | ||
|
||
module load "$LMOD_SYSTEM_DEFAULT_MODULES" |
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.
The module load should not be necessary, they should be loaded when Lmod is initialised
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.
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.
The module load should not be necessary, they should be loaded when Lmod is initialised
In my test cases the EESSI/2023.06
module was not loaded automatically, that why I put it in.
Just setting StdEnv.lua
doesn't do the trick also.
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.
Was there already an Lmod loaded when you source
d it?
module load $LMOD_SYSTEM_DEFAULT_MODULES
is a little brittle as this won't work if there is more than module in the variable (I think). It's probably best to take the official route as per the Lmod docs (shell dependent):
if [ -z "$__Init_Default_Modules" ]; then
export __Init_Default_Modules=1;
## ability to predefine elsewhere the default list
LMOD_SYSTEM_DEFAULT_MODULES=${LMOD_SYSTEM_DEFAULT_MODULES:-"StdEnv"}
export LMOD_SYSTEM_DEFAULT_MODULES
module --initial_load --no_redirect restore
else
module refresh
fi
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.
Was there already an Lmod loaded when you
source
d it?
No, Lmod was not loaded at that moment. Only loaded with the script.
Ok it expanded the PS1 inside the git commit message 🥲 |
Implemented in last commit. Works as expected, with exclusion for csh. |
- add test script - add github actions
not sure what the output for generic would be. But different arches should be already tested on module
- Fix Format
Pushed tests are expected to fail for now (getting interesting after merge of #667) |
Pull Last merge from Upstream
init/bash
Outdated
export __Init_Default_Modules=1; | ||
|
||
## ability to predefine elsewhere the default list | ||
LMOD_SYSTEM_DEFAULT_MODULES=${LMOD_SYSTEM_DEFAULT_MODULES:-"StdEnv"} |
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.
LMOD_SYSTEM_DEFAULT_MODULES=${LMOD_SYSTEM_DEFAULT_MODULES:-"StdEnv"} | |
LMOD_SYSTEM_DEFAULT_MODULES=${LMOD_SYSTEM_DEFAULT_MODULES:-"EESSI"} |
You'll need this everywhere
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.
Some sites also force full name matching so you probably can't leave it versionless
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 now implemented with Version number. It doesn't matter to me but like to understand the difference why StdEnv.lua
is not the correct solution.
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.
There is no existing StdEnv.lua
when Lmod is configured (we don't ship one)
We never included PS1 in #667 so your tests will fail for that currently. If you are keen to have that, it could be part of this PR or a follow-up after this is merged (this PR has higher priority in my opinion) |
- move new scripts - copy bash
csh is not as feature rich as other shells I have implemented. Maybe someday I learn how to approach it!
before we run any tests we check if the shell provided is actually testable. If a shell is not tested yet only means we don't know how to test the shell or the shell is not added to the TEST_SHELLS Array.
We actually don't know how to csh for now. But if we figure out we don't need to update here.
remove leftover comment Co-authored-by: ocaisa <[email protected]>
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.
LGTM
bot: build repo:eessi.io-2023.06-software arch:x86_64/generic |
Updates by the bot instance
|
Updates by the bot instance
|
New job on instance
|
Updates by the bot instance
|
You need something similar to https://github.com/EESSI/software-layer/blob/2023.06-software.eessi.io/install_scripts.sh#L94-L97 so that the new scripts are actually deployed |
Add copy of init lmod scripts
bot: build repo:eessi.io-2023.06-software arch:x86_64/generic |
Updates by the bot instance
|
Updates by the bot instance
|
Updates by the bot instance
|
New job on instance
|
bot: build repo:eessi.io-2023.06-software arch:x86_64/generic |
Updates by the bot instance
|
Updates by the bot instance
|
Updates by the bot instance
|
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.
LGTM, thanks for steering this over the line!
Label |
Staging PR merged, this is now in the wild! |
This is a followup PR for #667
After successful merge of #667 I have some test cases prepared, to test against the different shell.
For the moment, I struggle with an implementation for csh. It only seems to happen when I try to load the module inside csh. Error response of csh is not the brightest I've seen.