-
Notifications
You must be signed in to change notification settings - Fork 46
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 github CLI to global-workflow-env #1270
base: develop
Are you sure you want to change the base?
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.
I noticed CI is failing, can you check please?
Also, I am torn. We should check the original issue, and if the users requesting gh cli want it for a specific application (e.g. global workflow), then it should either go in that virtual package or higher up in the tree if more applications want it.
The goal is to be able to load a single module (after the stack-* metamodules) such as module load global-workflow-env
and having access to everything that is needed. Users shouldn't have to know all dependencies themselves, that's our job.
It seems like the permissions in the extracted gh cli source code are causing other PR's github actions to fail: https://github.com/JCSDA/spack-stack/actions/runs/10526384237/job/29167119508?pr=1257 I'll remove the build caches manually on the systems for now so that #1257 can proceed, please fix this before pushing to the branch again:
|
Looks like all the offending files are missing the write permission:
|
A change in #1270 uncovered a behavior where go mod derived package install modules cannot be removed from our scratch directory due to undesirable folder permissions. This has been observed before and golang maintainers marked the behavior as WAI suggesting that people maintain these directory trees with go-language tools. In our case this is not highly feasible due to spack's intervention in the project structure meaning we must find a workaround. The workaround here uses find to update directory permissions to ensure that write/execute is set on all directories in the tree. This has been tested locally but due to environment carryover we cannot ensure it has the desired effect for go packages in our CI system until it is submitted. (Ie; testing would require re-running an affected workflow and all other pull requests would still be subject to the issue and would break).
Thanks for the review - I've updated this per your suggestion; I'm still learning the various enviornment modules we manage. I'll watch the workflow results here to see if that CI fix we merged works as expected. |
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 to me, thanks for making the updates. I'll go ahead and approve. Auto-merge will take care of the rest, assuming that the tests all pass.
I see that the ubuntu intel CI test fails because of go. We should stay away from go. That's the reason why git-lfs is always an external package. go is difficult to build, if we can avoid it then let's do it. If not, we need to look into building go with GNU (along the lines of #1257 |
Withdrawing my approval because I didn't realize that gh cli depends on "go" - need to take a closer look at the package
Add the github-cli to global-workflow-env
Testing
gh
) in order to include this CLI toolIssue(s) addressed
Fixes: #1105
Checklist