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

Compiler in base-spack #62

Merged
merged 2 commits into from
Aug 22, 2023
Merged

Compiler in base-spack #62

merged 2 commits into from
Aug 22, 2023

Conversation

CodeGat
Copy link
Member

@CodeGat CodeGat commented Aug 21, 2023

Pushing the compiler down into base-spack as it is not updated as frequently in the pipeline and can therefore help compilation times. This PR changes the workflows somewhat, in which the matrix is created relatively early in the pipeline, and is fed into the new workflow (dependency-image-pipeline.yml)

Should close #50 !

@CodeGat CodeGat self-assigned this Aug 21, 2023
@CodeGat CodeGat force-pushed the 50-compiler-base-spack branch from 67e5126 to f3b1bb8 Compare August 21, 2023 22:59
Copy link
Member

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

I have some comments!

I can't say I have checked it exhaustively, but I can do another pass if required.

Copy link
Contributor

@Quartzer2 Quartzer2 left a comment

Choose a reason for hiding this comment

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

All good. Though just a cursory run through before I have to head off.

Only point in relation to something Aidan mentioned (and this my opinion only) is to use traditional GNU stuff (sed/awk/etc). Jq is great and I've used it often - just that it is 3rd party. Again just my opinion, I'm an old timer (pre-JSON standard) so probably a bit conservative in what I use for parsing :) Otherwise happy to use new stuff - just depends if it is critical or not. Depending on definition of citical!

@CodeGat CodeGat requested a review from aidanheerdegen August 22, 2023 04:52
@CodeGat
Copy link
Member Author

CodeGat commented Aug 22, 2023

@Quartzer2 thats a fair opinion too! Looking through the build-ci code again now and there seems to be precedent for jq but I'll wait to change anything for the moment. Interestingly, the runner comes with jq installed(?!)

@aidanheerdegen
Copy link
Member

@Quartzer2 thats a fair opinion too!

Indeed it is. But as you say @CodeGat we're already using it and it is very nice and fast and actually parses the JSON, so has more chance of picking up problems, and working with a wider range of possible inputs.

I like jq because it is obvious what it is doing. We could arguably make it even more obvious using the full command line option --compact-output.

I admit I couldn't think of a way the current method breaks, but I only spent 5 minutes thinking, and I may not be very imaginative. I did think of a way: DOS line-endings, but that seems a bit far-fetched, so I'm not counting it.

Copy link
Member

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

Looks great!

Thanks for the good work.

@CodeGat CodeGat force-pushed the 50-compiler-base-spack branch from ec24506 to 8735953 Compare August 22, 2023 23:07
@CodeGat CodeGat merged commit bc914d7 into main Aug 22, 2023
@CodeGat CodeGat deleted the 50-compiler-base-spack branch August 22, 2023 23:08
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.

Push install of compilers lower down in the Dockerfile pipeline
3 participants