-
Notifications
You must be signed in to change notification settings - Fork 47
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
Use EESSI module in EESSI-install-software.sh #790
base: 2023.06-software.eessi.io
Are you sure you want to change the base?
Use EESSI module in EESSI-install-software.sh #790
Conversation
Instance
|
Instance
|
Instance
|
bot: build repo:eessi.io-2023.06-software arch:zen2 |
Updates by the bot instance
|
Updates by the bot instance
|
Updates by the bot instance
|
New job on instance
|
Ghehe, is that a polite way of saying you would like a review? ;-) I don't think it will outright affect the use in |
bot: build repo:eessi.io-2023.06-software arch:zen2 |
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:zen2 |
Updates by the bot instance
|
Updates by the bot instance
|
Updates by the bot instance
|
New job on instance
|
…tware-layer into use_module_in_eessi_install
bot: build repo:eessi.io-2023.06-software arch:zen2 |
Updates by the bot instance
|
Updates by the bot instance
|
… we want to install in the CVMFS prefix
bot: build repo:eessi.io-2023.06-software arch:zen2 |
Updates by the bot instance
|
Updates by the bot instance
|
Updates by the bot instance
|
New job on instance
|
… not suitable as a test package
Hmm...
That's not the artefacts list I expected :D Yes, this script should be reinstalled. But it should at the very least also install the
Ah, that explains! Someone already installed my favorite dummy package that I like to try ;-) |
bot: build repo:eessi.io-2023.06-software arch:zen2 |
Updates by the bot instance
|
Updates by the bot instance
|
Updates by the bot instance
|
Unable to download or merge changes between the source branch and the destination branch. |
bot: build repo:eessi.io-2023.06-software arch:zen2 |
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:zen2 |
Updates by the bot instance
|
Updates by the bot instance
|
Updates by the bot instance
|
New job on 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.
Looks good, I think I see an issue in a particular scenario for dev.eessi.io
|
||
# If in dev.eessi.io, allow building on top of softw | ||
if [[ "${EESSI_CVMFS_REPO}" == /cvmfs/dev.eessi.io ]]; then | ||
module use /cvmfs/software.eessi.io/versions/$EESSI_VERSION/software/${EESSI_OS_TYPE}/${EESSI_SOFTWARE_SUBDIR_OVERRIDE}/modules/all |
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.
Would be good to make a comment that explains the expected precedence in MODULEPATH
:
/path/to/dev.eessi.io/modules:/path/to/accel/modules:/path/to/software.eessi.io/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.
I'm not sure you have caught the case though that would define this:
- I want to install in dev.eessi.io
- I want to install for an accelerator (and require accelerator dependencies)
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 wonder is this what we want to support:
/path/to/dev.eessi.io/accel/modules:/path/to/dev.eessi.io/modules:/path/to/software.eessi.io/accel/modules:/path/to/software.eessi.io/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.
While my pr moved this section of code, I'm pretty sure it was already there, wasn't it? I'm on my phone currently, so can't easily check
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 one is on me, these changes come from #771, see also @boegel's comment about handling accel cases #771 (comment)
In a meeting we talked about fixing this in a separate PR, but if it's better to address this on this PR I can give it a go when I'm back at my desk this week
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'd be in favour of doing that in a separate PR, if @ocaisa agrees. This PR currently is mostly a reshuffle: some parts that can only be done after EESSI is initialized got moved down until after the EESSI module is loaded. While the pr replaced sourcing the various init scripts with loading the module, it should not modify behaviour in any way. I think it's more clear of we do a big fix like that in a separate PR
Co-authored-by: ocaisa <[email protected]>
No description provided.