Skip to content

Base spack.includes for build-ci - #486

Merged
CodeGat merged 11 commits into
api-v2from
484-top-level-includes
Sep 14, 2026
Merged

CodeGat merged 11 commits into
api-v2from
484-top-level-includes

Conversation

@CodeGat

@CodeGat CodeGat commented Aug 27, 2026

Copy link
Copy Markdown
Member

Closes #484

Background

We are looking to remove common configuration from individual CR manifests via layered configuration via a spack.include section.

The configuration in this PR is the top level of included configuration, with common configurations for compilers, concretizers, packages and repos. Any of the config in this repository is easily overridable at the CR level.

Note

build-ci@v4-specific CI updates for ASP will be done in #485, this PR is specifically for CRs using build-ci@v4 that consume these base includes, not for ASP to work with build-ci@v4

Structure

Currently, we are looking at having a base level of configuration, which contains information on compilers, packages and repos.

We will also have a pr level of configuration (which inherits base), that is special, specifically for the case where we have an ASP PR that calls CRs, which then include ASP configuration - we want it to use the ASP PRs version of access-spack-packages, not the default api-v2.

As a common entrypoint for both CR and ASP PR, we have a ci level of config, which makes the decision on inheriting base or pr (which then inherits base).

The PR

  • Add .github/build-ci/includes/base for config common to all compilers
  • Add .github/build-ci/includes/pr for config to be used when access-spack-packages is creating PRs
  • Add .github/build-ci/includes/ci as a common entrypoint

Testing

Testing includes in ACCESS-NRI/MOM6#74

@CodeGat CodeGat added the build-ci-tests Relating to build-ci's manifest tests label Aug 27, 2026
@CodeGat CodeGat self-assigned this Aug 27, 2026
@CodeGat CodeGat changed the title Base spack.includes for build-ci Base spack.includes for build-ci, updates for build-ci@v4 Aug 27, 2026
@CodeGat
CodeGat force-pushed the 484-top-level-includes branch from 322b6bf to 09ea280 Compare August 27, 2026 00:42
@CodeGat

CodeGat commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

Before merging, update target to x86_64_v3. Also consider putting it under the base scope rather than the compiler scope

EDIT: Not required, as it is set in https://github.com/ACCESS-NRI/spack-config/blob/main/v1.1/packages.yaml#L12-L13

@CodeGat
CodeGat marked this pull request as ready for review August 28, 2026 06:53
Comment thread .github/build-ci/include/base/repos.yaml Outdated
@CodeGat
CodeGat force-pushed the 484-top-level-includes branch from 824e10c to 7506633 Compare August 31, 2026 00:37
@CodeGat
CodeGat force-pushed the 484-top-level-includes branch from 7506633 to 0b027b0 Compare August 31, 2026 00:40
@CodeGat CodeGat changed the title Base spack.includes for build-ci, updates for build-ci@v4 Base spack.includes for build-ci Aug 31, 2026
@CodeGat

CodeGat commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Okay, this is ready for review. I've dropped the CI commit from this PR as it will be done in #485

@harshula

Copy link
Copy Markdown
Collaborator

Hi @CodeGat , We use include in https://github.com/ACCESS-NRI/spack-config/tree/main/v1.1/include . Perhaps for consistency, it would better to replace includes with include?

@CodeGat
CodeGat marked this pull request as draft August 31, 2026 05:07
@CodeGat

CodeGat commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Drafting this to test out ACCESS-NRI/MOM6#74 (comment)

@CodeGat

CodeGat commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

I've got this working again, see ACCESS-NRI/MOM6#74 (comment).

However, we need to double-check that we can fold all the compiler info in a single directory so we can be sure this works. Whether that be by putting compiler directives in the spec line, I dunno. But this is looking good! Might need to rename the ASP_ci folder too. It's essentially config used for PRs from this repo.

@CodeGat
CodeGat marked this pull request as ready for review September 3, 2026 00:16
@CodeGat

CodeGat commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

Looks like we can fold compiler info into a single directory by using propagating toolchains in the spec line, eg. %%access_oneapi, so that bit is solved. Example manifest and successful run with concretization.

Now for some naming - for the specific configuration that is used when we are running an access-spack-packages PR - the directory is called .github/build-ci/include/ASP-ci currently, but that seems a bit clunky. Since it is to do with ASP PRs, maybe it should just be .github/build-ci/include/pr, instead. What do people think?

@aidanheerdegen

Copy link
Copy Markdown
Member

What do people think?

I like pr

@CodeGat
CodeGat force-pushed the 484-top-level-includes branch 5 times, most recently from 7472442 to 88f4fd1 Compare September 9, 2026 03:58
@CodeGat

CodeGat commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

Okay this is finally ready for review!

Added additional context to the comments in repos.yaml.
@harshula

Copy link
Copy Markdown
Collaborator

Hi @CodeGat , Can you please explain the choice of sub-directory names under .github/build-ci/include/ and what they mean?

@CodeGat

CodeGat commented Sep 11, 2026

Copy link
Copy Markdown
Member Author
  • .github/build-ci/include/ci is the common entrypoint for all build-ci builds - it chooses between including pr (and base) or just base depending on whether it is a PR from ASP or a CR respectively
  • .github/build-ci/include/pr is for config to be used when the PR is from this repository. It also includes base.
  • .github/build-ci/include/base is a common base for config common to all builds

@harshula

Copy link
Copy Markdown
Collaborator

Hi @CodeGat , Thanks!

  • Can .github/build-ci/include/ci be changed to reflect it is the common entrypoint?
  • Can .github/build-ci/include/pr be changed to reflect that it contains config for local CI?

For example, Have a look at spack-config (https://github.com/ACCESS-NRI/spack-config/tree/main/v1.1). Could something like this work?:

  • .github/build-ci/include/base is .github/build-ci/config/base
  • .github/build-ci/include/ci is .github/build-ci/config/include
  • .github/build-ci/include/pr is .github/build-ci/config/local

After this discussion, I think I should add a base directory to spack-config!

@CodeGat

CodeGat commented Sep 11, 2026

Copy link
Copy Markdown
Member Author

I think that looks good, too. But, should this .github/build-ci/config convention for includes carry over to CRs? Currently they are of the form .github/build-ci/include, see https://github.com/ACCESS-NRI/MOM6/pull/74/changes#diff-f03f8a56c63fecd0bf7d473a48384c6808b0c5b960acc88a217d10db83f0f81dR4

@harshula

Copy link
Copy Markdown
Collaborator

Hi @CodeGat , Yes, that's good idea! Let's make all the .github/build-ci/ directories as similar as possible.

@CodeGat

CodeGat commented Sep 11, 2026

Copy link
Copy Markdown
Member Author

Comment thread .github/build-ci/config/base/packages.yaml
aidanheerdegen
aidanheerdegen previously approved these changes Sep 11, 2026
harshula
harshula previously approved these changes Sep 14, 2026

@aidanheerdegen aidanheerdegen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm approving again, but it seems the same as last time I approved, which is fine, just checking I am not going insane.

@CodeGat

CodeGat commented Sep 14, 2026

Copy link
Copy Markdown
Member Author

I've validated that the new v3 image works with these changes, see https://github.com/ACCESS-NRI/MOM6/actions/runs/34799693502?pr=74

Thanks everyone!

@CodeGat
CodeGat merged commit 85d18da into api-v2 Sep 14, 2026
@CodeGat
CodeGat deleted the 484-top-level-includes branch September 14, 2026 04:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build-ci-tests Relating to build-ci's manifest tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add top-level spack.includes default

3 participants