Sync branch ci rollout - #414
Open
mcocdawc wants to merge 30 commits into
Open
Conversation
Migrates ecflow onto the manifest-driven CI in ecmwf/ci-infrastructure. The legacy pipeline keeps running unchanged, moved to old_CI.yml (name: old CI) so the new one can take the plain `CI` name that the generated downstream orchestrators match their `workflow_run` trigger against. ecflow does not depend on eckit: nothing in its CMake references it, so there is no eckit edge in either direction. It does depend on stack-deps, for pybind11 — ENABLE_PYTHON is on by default and cmake/Dependencies.cmake does a find_package(pybind11 2.10.3 REQUIRED) under it. Boost and OpenSSL come from the container: ENABLE_SSL is on by default, and Boost is required outright. The build leg therefore uses the new ubuntu24.04-gfortran13-boost image, while keeping platform = ubuntu-24.04 so the ecbuild and stack-deps artifacts published under that slug are reused rather than rebuilt — container is scheduling, not artifact identity. Three configure flags are carried over from the repo's own ci-config.yml, each needed because the toolchain here is the distro's: BOOST_ROOT=/usr (Dependencies.cmake sets Boost_NO_SYSTEM_PATHS unconditionally, hiding the apt-installed Boost), ENABLE_CONFIG_MODE_BOOST=OFF (ubuntu 24.04 ships CMake 3.28, below the 3.30 config mode targets), and ENABLE_STATIC_BOOST_LIBS=OFF. ctest runs against the build tree, not the install tree: ecflow's tests link against it and against static libraries that never reach the install prefix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the generated cross-repo-trigger.yml, and trigger-downstream.yml for the three repos that have consumers. Regenerate with the ci-infrastructure-generate CLI; never edit by hand. [[trigger-downstream]].ref stays at develop. That ref is what the orchestrator pins its `uses:` to — GHA forbids an expression there — and it selects only which workflow DEFINITION runs, never the code. Branch coordination is entirely a runtime affair and needs no help from it: the orchestrator passes branch: workflow_run.head_branch, pick-ref checks out the same-named branch in each consumer when one exists, and resolve_deps sees a current-branch matching ^sync-branch- and resolves every upstream's manifest and artifacts from it. One consequence worth knowing: `on: workflow_run` only fires for a workflow file present on the default branch, and runs that copy. So the fan-out cannot be exercised from sync-branch-ci-rollout at all — it goes live only once these files reach develop. Nothing to flip later; this is the steady-state form. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ci-infrastructure removed its reusable .github/workflows/pre-commit.yml (ecmwf/ci-infrastructure#14), so the comment here pointed at a workflow that no longer exists. Each repo now carries the job itself; stack-dependencies' ci.yml has the copyable version. Comment only — no job runs either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The expensive work — the actual compile — is already on arc-runner-very-large via the matrix legs. resolve was not: it sat on arc-runner-normal inside the base container, for a job that is one batched GraphQL walk of the manifest graph with no compiler and nothing to build. ubuntu-slim is the cheap 1-CPU ARC runner and needs no container, which is also what ci-infrastructure's generator picks for the identical resolve job it emits into cross-repo-trigger.yml. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
These are the first expensive compiles in the new system, and artifact reuse does not help them: it avoids a rebuild only when nothing changed, so a PR touching one source file still recompiles the whole tree in a fresh container. That is exactly what sccache is for. ecbuild and stack-dependencies did without it for good reason — one compiles nothing, the other builds once and is then reused from its artifact. The steps live in the build action rather than the workflow so both entry points get them: the generated cross-repo-trigger.yml calls this action directly and never sees the workflow's own steps. It already exports SCCACHE_BUCKET; ci.yml now does too, alongside the artifact bucket. They share the object store's endpoint and credentials and differ only in bucket. Opt-in by presence of that bucket. sccache has no default bucket and a daemon that cannot reach one refuses to start, which would fail the build outright, so with SCCACHE_BUCKET unset both steps no-op, the launcher flags come out empty and the compile just runs uncached. A repo without the secret still builds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reverts the ubuntu-slim change: it saved runner size and lost far more to a per-job venv build. ci-infrastructure is baked into the base image and advertised via CI_INFRASTRUCTURE_PYTHON, which makes ensure-infrastructure-present a true no-op — no build, no PyPI/GitHub egress. ubuntu-slim is a runner label, not an image, so with no container there is no bake and every run installs the package from scratch. It cannot even be cached between runs: the venv lives under RUNNER_TOOL_CACHE inside _work, and ARC's Kubernetes mode overlays _work with an ephemeral per-job volume, so the cache is discarded each time. resolve is short, so the container dominates its cost, not the runner size. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ecflow built and tested inside a SLURM job, linking the atos-hpc-gnu ecbuild and stack-deps (pybind11) artifacts. Two toolchains, matching the two the legacy ci-hpc-config.yml exercised most recently — gnu-15.2.0 and intel-2025.3.1 — each with its own recipe and its own platform slug, since an intel-built tree is a different ABI from a gnu one and the two must never share an artifact name. Boost, Python and Qt come from cluster modules rather than artifacts: ecflow links Boost.Python against a specific interpreter and the cluster ships the matched pair, which is also why the module versions are pinned rather than floating. pybind11 still comes from stack-deps. The configure flags and the ctest selection (-L nightly -E s_http) are carried over from the legacy config. `needs` names HPC kinds only — the lanes are separate artifact universes. The job runs in the internal-tools container for its cluster ssh identity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #414 +/- ##
========================================
Coverage 52.31% 52.32%
========================================
Files 1251 1250 -1
Lines 102291 102288 -3
Branches 15202 15202
========================================
+ Hits 53515 53523 +8
+ Misses 48776 48765 -11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The runner-lane build failed at configure with "CRITICAL - Qt5/6 not found -
this is required for ecFlowUI": ENABLE_UI is ON by default in CMakeLists.txt and
the build action never turns it off, but the pinned container carried only
gfortran-13 and Boost.
The runner lane was the odd one out, not the UI default. The HPC lane already
builds ecFlowUI via `module load qt/6.6.1` in .ci/hpc/build-{gnu,intel}.sh, and
the legacy downstream-ci pipeline built it too — .github/ci-config.yml never
passes -DENABLE_UI=OFF, so its image must have shipped Qt. Adding
-DENABLE_UI=OFF here would have unblocked the fan-out by silently narrowing the
runner lane to less than both pipelines it replaces, leaving ecFlowUI compiled
on HPC only.
So the image grows Qt6 instead: ecmwf/ci-container-images now publishes
ubuntu24.04-gfortran13-boost-qt6 (renamed from -boost, since that repo's
convention is that a variant's name says exactly what it adds over the base),
and this pins it. Nothing else moves — `container` is scheduling, not artifact
identity, and it is resolved from this manifest at runtime rather than baked
into the generated workflows, so no regeneration is needed.
The build action gains a comment recording why ENABLE_UI is left at its default
and where Qt comes from: that file is the first thing anyone opens when this
error recurs, and the answer lives two repos away.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1jc9nyMBC8eQaxkF4vhoq
ci.yml has run ctest here all along, but the generated cross-repo-trigger.yml job only built and published. So when an upstream fanned out to us, a green downstream/runner proved only that ecflow still LINKED against the new ecbuild and stack-deps -- never that it still worked. Set `ctest = true` on [matrix.build] and regenerate. The step lands after the publish step, matching ci.yml: whoever consumes our artifact picks it up as soon as publish runs and never waits on these tests. Plain `ctest`, exactly what ci.yml runs. Also add .github/actionlint.yaml so the ARC scale-set labels stop reading as unknown runners. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014ZXmNdJJYqZkpBNTycoPdP
Publish ran before Test, so a failing suite still put its output in the store — and nothing downstream can tell the difference afterwards. resolve_deps and fetch_deps both decide with a bare object_exists, and check_artifact stops asking about the producing run the moment the object is found. The orchestrator's `conclusion == 'success'` gate does not cover this: it stops the fan-out from starting, not a rebuild-request waiter (whose poll returns as soon as Publish completes, before ctest has begun) or a later run that finds the object already present. Swapping the two steps closes it — a failing step ends the job, so nothing is uploaded. The cost is that consumers now wait for our tests. `Print dependency table` moves above `Test` in the same pass. It does not depend on the publish step, and having it run on a red suite is the point: it names the upstream shas the failing build resolved against. The generated cross-repo-trigger.yml carries the same swap, from the regenerated ci-infrastructure. HPC is unaffected: those recipes already ctest before `cmake --install`, and build-on-hpc publishes only after the job succeeds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014ZXmNdJJYqZkpBNTycoPdP
That case cannot pass. libs/server/test/TestAuthentication.cpp is a single
BOOST_CHECK_MESSAGE(false, "Test not implemented");
added on develop by d44e163 ("Refactor Access control", 2025-09-02) as a
placeholder. It is registered with LABELS unit, so the HPC recipes never execute
it (`ctest -L nightly -E s_http`) and only the runner lane trips over it — which
is why it surfaced now rather than months ago: nothing in the previous setup ran
the unit label on a path that gated anything.
Excluded in CI rather than fixed in the test source, because the stub belongs to
ecflow proper and the fix — marking the case
`*boost::unit_test::disabled()`, or implementing it — is an upstream change to
develop, not part of the CI rollout. Both the ci.yml step and the manifest's
ctest-args carry the exclusion so the push/PR build and the generated fan-out run
the identical suite; a comment in each names the commit and says to drop it once
the test exists, since from then on the exclusion, not the stub, is what would
hide a real regression.
This leaves the runner lane green: the other 52 failures in this job were
"/usr/bin/python3: No module named pytest" and are already gone now that the base
image ships python3-pytest.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ZXmNdJJYqZkpBNTycoPdP
libs/server/test/TestAuthentication.cpp was a single
BOOST_CHECK_MESSAGE(false, "Test not implemented");
added by d44e163 ("Refactor Access control", 2025-09-02). It cannot pass, so
every run of the `unit` label that reaches it is red by construction. It went
unnoticed because the HPC recipes run `ctest -L nightly -E s_http` and never
execute it; the new runner-lane ctest does, which is what surfaced it.
Deleted rather than skipped. The previous commit excluded it with
`ctest -E u_server_authentication` in both ci.yml and the manifest's ctest-args,
which is carrying CI machinery — in two places, kept in step by hand — to hide a
test that asserts its own absence. An empty placeholder is not a test, and it
should not sit on develop in a state where implementing it is the only way to
make the suite honest.
Removes both sources (the case and its Boost entry point, neither shared with
another target) and the ecbuild_add_test/target_clangformat pair that registered
it, and reverts the CI exclusion in the same pass, so nothing is left referencing
the name. Authentication coverage, when it is written, comes back as a real test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ZXmNdJJYqZkpBNTycoPdP
The runner lane ran a bare `ctest`, on the reasoning that it should match
ci.yml. Wrong reference point: what this lane replaces is
.github/ci-config.yml, the legacy downstream-ci runner config, and that gates on
ctest_options: >-
-L nightly
-E s_test|s_zombies
`-L nightly` is the set the old pipeline actually required to be green;
s_test and s_zombies (libs/test/overall) are long-running system suites it
deliberately skips. Running everything unfiltered meant this lane demanded more
than the pipeline it is meant to reproduce, so failures here did not mean a
regression — they meant we had opted into suites nobody gated on.
Also sets -DENABLE_ALL_TESTS=ON in the build composite. The legacy config passes
it, and so do our own .ci/hpc/build-{gnu,intel}.sh, which left the runner lane as
the one place in the rollout configuring a smaller suite than everywhere else —
the opposite error to the ctest filter, and easy to miss because the two cancel
out in the totals.
Both ci.yml and the manifest's ctest-args carry the same filter so the push/PR
build and the generated fan-out gate identically. Quoted, or the shell reads the
`|` as a pipe. The HPC recipes keep their own `-E s_http`, which is a separate
cluster-specific narrowing from .github/ci-hpc-config.yml, not this one.
Note this also means the u_server_authentication placeholder removed in the
previous commit would never have run here: it carried LABELS unit, not nightly.
It was still right to delete — a test that asserts its own absence has no
business on develop — but the failure that surfaced it was ours, not ecflow's.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ZXmNdJJYqZkpBNTycoPdP
Two bugs, both from loading UNVERSIONED prgenv modules while pinning versioned
boost/python/qt around them.
gnu: CMake was left to its default compiler search and picked /usr/bin/g++ —
the compute node's system gcc 8.5.0 — while this recipe hardcodes
Boost_ROOT=.../boost/1.90.0/GNU/15.2. The link failed with
libboost_program_options.so.1.90.0: undefined reference to
`std::ios_base_library_init()@GLIBCXX_3.4.32'
because that symbol is GCC 13+ libstdc++. So the HPC gnu leg was never a test
failure at all: ecflow never linked, and no artifact was ever produced. Pinned to
gcc/15.2.0 with explicit CMAKE_C/CXX_COMPILER, matching build.hpc.sh's
load_gcc15_2() and the legacy `gnu-15.2.0` platform. The intel leg escaped this
only because it passed CMAKE_CXX_COMPILER=icpx.
intel: `module load prgenv/intel` resolved to IntelLLVM 2021.4.0, not the
2025.3.1 this recipe's header claimed, while hardcoding the INTEL/2025.3 Boost
prefix. It linked, so the mismatch was invisible — but 2021.4.0 is a different
row of the legacy test table (`-E '(py3_|s_http)'`, Python tests disabled too),
so we were running a suite that toolchain is not expected to pass. Pinned to
prgenv/intel-llvm + intel/2025.3.1, which is what build.hpc.sh's
load_intel2025_3() uses — note prgenv/intel-llvm, not prgenv/intel.
Also adds aocc-4.0.0 and nvidia-24.11 legs. These demonstrate omitting ctest
rather than disabling it: the legacy ci-hpc-config.yml has to write
`ctest_options: --version` — invoking ctest with a flag that makes it print its
version and exit — because that config has nowhere to say "no tests". Our
recipes own the test invocation, so not testing is the absence of a ctest line:
nothing to disable, no sentinel flag, and the reader sees build then install with
nothing between them. On the runner lane the same thing is said by omitting
`ctest = true` from [matrix.build].
nvidia-24.11 rather than the config's nvidia-22.11: build.hpc.sh only knows
nvidia/24.1, 24.11 and 25.11, so 22.11 looks retired and `module load
nvidia/22.11` would fail at job start. 24.11 is in both the legacy config and
build.hpc.sh, and is equally tests-disabled, so it demonstrates the same point on
a toolchain that provably exists.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ZXmNdJJYqZkpBNTycoPdP
mcocdawc
force-pushed
the
sync-branch-ci-rollout
branch
from
August 24, 2026 11:49
eaad471 to
2cf4fe9
Compare
ecflow's suite ran one test at a time: 71 tests, 500.34 sec*proc, 500.38 sec
real — a ratio of 1.00, the whole machine idle but for one process. The build
step was already parallel, so only the test phase wasted the runner.
-j 8 rather than the $(nproc) the other repos take. ecflow's tests are not
independent the way ecbuild's nested cmake configures are: 460 of those 500
seconds are 7 `integration` tests that start real ecFlow servers, and parallel
execution risks collisions on ports and on-disk state. 8 is not a guess — it is
the width ecflow's own .github/cd-config.yml already uses for every HPC stage,
test_command: "ctest --output-on-failure -j 8 -L nightly"
so it is a concurrency this suite is already run at in this project.
Applied to ci.yml, the manifest's ctest-args (so the fan-out matches) and both
testing HPC recipes. The aocc and nvidia recipes are untouched: they run no tests
at all.
On its own commit, separate from the other three repos', so that if this does
surface a flake it is unambiguous which change caused it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ZXmNdJJYqZkpBNTycoPdP
mcocdawc
force-pushed
the
sync-branch-ci-rollout
branch
from
August 24, 2026 12:07
26e40c3 to
1e6b8fe
Compare
It is the sole failure on that lane, and it TIMES OUT rather than asserting:
99% tests passed, 1 tests failed out of 79
The following tests FAILED:
74 - py3_s_TestClientApi (Timeout)
Worth recording where it came from: it is one of the `integration python nightly
slow` cases that start a real ecFlow server, registered only under
`if (ENABLE_ALL_TESTS AND ENABLE_SERVER)`. It appears on this lane because we
turned -DENABLE_ALL_TESTS=ON to match the legacy runner config — 79 tests here,
up from 72 — so this is coverage newly gained, not newly broken.
The exact target name matters. ctest -E is a REGEX matched against the test name,
so a near-miss such as `py3_s_Test_Client` matches nothing and silently leaves the
test running — a filter that looks applied and is not. The three py3_s_* targets
this repo builds are TestClientApi, TestPythonChildApi and TestSslSetup; only the
first fails.
Applied to ci.yml and to the manifest's ctest-args, which are kept identical so
push/PR and the generated fan-out gate on the same set. The HPC recipes keep
their own `-E s_http` and are untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ZXmNdJJYqZkpBNTycoPdP
Reverts the -DENABLE_ALL_TESTS=ON I added to match the legacy runner config. Turning it on registered the `integration python nightly slow` py3_s_* group (72 tests -> 79), and py3_s_TestClientApi in that group times out. Tuning those timeouts belongs to ecflow's maintainers, not to the CI rollout, so this lane stays on the default set until they get to it. That also drops the `-E py3_s_TestClientApi` exclusion added in the previous commit. With ENABLE_ALL_TESTS off, that target is not registered at all — the group is behind `if (ENABLE_ALL_TESTS AND ENABLE_SERVER)` — so the filter would match nothing. An exclusion that looks applied and does nothing is exactly the trap flagged when it went in, so it goes rather than sitting there inert. The .ci/hpc recipes keep ENABLE_ALL_TESTS=ON: they are a separate lane with its own failures still under investigation, and narrowing them here would confuse that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014ZXmNdJJYqZkpBNTycoPdP
Repoints every container reference at the renamed Harbor projects:
eccr.ecmwf.int/public-playground-ci/... -> public-ci-images/...
eccr.ecmwf.int/private-playground-ci/... -> private-ci-images/...
in .ci/manifest.toml (the `container` field on each matrix leg) and in ci.yml's
hardcoded resolve-job image. The "playground" name has outlived the playground.
Stage 3 of 3, and deliberately last: nothing may point at a project before the
images exist in it. ci-infrastructure's rename published all six public images
first (its own run is green and the repositories are visible in the registry),
then ci-container-images published internal-tools to private-ci-images. Every
image named here has been confirmed present before this commit was made.
No artifact churn: `container` is scheduling only and never enters artifact
identity, so nothing published under the old image names is invalidated and no
rebuild cascades from this.
Regenerating changes nothing, which is the expected result rather than a missed
step — the generated cross-repo-trigger workflows carry `image: ${{
matrix.container || '' }}` and resolve the image from the matrix at run time, so
the names live in the manifest alone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ZXmNdJJYqZkpBNTycoPdP
Both remaining HPC failures were this branch diverging from what the legacy
CI actually does, so both fixes are carried over from it rather than invented.
Resources. build-package-hpc's atos template (templates/macros.jinja,
sbatch_atos) emits
#SBATCH --gres=ssdtmp:30G
#SBATCH --mem=64GB
#SBATCH --cpus-per-task=<parallel // ntasks>
#SBATCH --ntasks=<ntasks>
and .github/ci-hpc-config.yml sets `parallel: 64` for every ecflow platform,
against that tool's `ntasks` default of 1 -- one task, 64 CPUs, a flat 64 GB.
These recipes asked for 8 tasks and named no --mem at all, so they ran on
SLURM's default memory. That is what killed nvidia: ECMWF's watch_cgroup
stopped it (EC_MEMKILL, job 37369545) at 8785 MiB against an 8000 MiB cgroup
SOFT limit -- the hard limit was 78 GB, so the watchdog, not the OOM killer.
nvc++/nvcpfe carry ~1100 MiB per translation unit, several times what g++,
icpx or clang need, which is why only that leg died. Building 4-way would
have hidden it while diverging further; the legacy CI compiles this same tree
64-way without trouble because it asks for the memory. So do we now, and
`cmake --build --parallel` follows $SLURM_CPUS_PER_TASK rather than
$SLURM_NTASKS.
gnu. It built and tested, but all 55 py3_* tests failed identically before
collecting a case:
ImportError: /lib64/libstdc++.so.6: version `GLIBCXX_3.4.32' not found
(required by .../libs/pyext/python3/ecflow.so)
The python tests dlopen ecflow.so into the cluster's python3, whose loader
resolves libstdc++.so.6 to the system /lib64 copy -- gcc 8.5.0's, which stops
at GLIBCXX_3.4.25 -- while ecflow.so is built by gcc 15.2. The C++ tests are
unaffected, which is why the run came back 25/80 green. The legacy config's
`gnu-15.2.0` block already carries the cure and this recipe had dropped it:
-DECFLOW_PYEXT_TEST_LD_LIBRARY_PATH=/usr/local/apps/gcc/15.2.0/lib64, which
ecflow reads in libs/pyext/python3/CMakeLists.txt and prepends to
LD_LIBRARY_PATH in the python tests' ctest ENVIRONMENT. Restored verbatim.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ZXmNdJJYqZkpBNTycoPdP
The nvidia leg got past the memory limit and died at link instead, on every
executable that pulls Boost.Context:
libboost_context.so: undefined reference to `jump_fcontext'
`make_fcontext'
`ontop_fcontext'
Those are Boost.Context's assembly entry points, and the NVIDIA/24.11 build of
the shared library does not define them. The static libboost_context.a carries
those objects, which is why the legacy CI compiles this same tree against this
same module without trouble -- it links Boost statically.
ENABLE_STATIC_BOOST_LIBS and ENABLE_CONFIG_MODE_BOOST both default ON in
CMakeLists.txt, and .github/ci-hpc-config.yml names neither in any platform
block, so the legacy HPC CI simply takes those defaults. These recipes forced
both OFF. That pair is correct for the RUNNER lane, where
.github/actions/build-ecflow documents why -- ubuntu 24.04 ships CMake 3.28,
below config mode's 3.30 floor, and the distro packages only shared Boost --
but it was copied onto the HPC recipes, where the cluster modules provide both
and cmake/new is 4.2.4. Neither reason survives the move, so both overrides go
and the defaults stand, as they do in the legacy config.
Dropped from all four legs rather than just nvidia: gnu, intel and aocc were
passing only because their shared Boost.Context happens to be intact, and the
legacy CI builds all four against static Boost.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014ZXmNdJJYqZkpBNTycoPdP
Closed
marcosbento
self-requested a review
August 26, 2026 12:27
pull_request_target runs the BASE branch copy, so the trigger cannot be flipped before this is on develop -- neither event would fire and the open rollout PRs would get no CI at all. Flip the one word after the merge; the merge-ref checkouts and the PR-keyed concurrency group are already correct for both events. Add workflow_dispatch, which old_CI.yml had and the rewrite dropped: after the flip it is the only way to exercise a change to this file, since pull_request_target never runs a PR own copy of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RZEn4Nuyg9jTRB3steGTgi
marcosbento
requested changes
Aug 27, 2026
marcosbento
left a comment
marcosbento
left a comment
Collaborator
There was a problem hiding this comment.
Some adjustments needed...
| - 'develop' | ||
| tags-ignore: | ||
| - '**' | ||
| branches: [develop] |
Collaborator
There was a problem hiding this comment.
CI is also expected to run on changes in the main branch, not only in develop.
| branches: | ||
| - 'main' | ||
| - 'develop' | ||
| tags-ignore: |
Collaborator
There was a problem hiding this comment.
The existing CI is not run when tags are pushed. Should this behaviour continue?
| @@ -0,0 +1,59 @@ | |||
| name: old CI | |||
Collaborator
There was a problem hiding this comment.
We could do a better job at renaming the existing CI... I am pretty sure that the "new" CI will eventually become "old" as well.
Maybe a better name would be 'legacy CI' or 'deprecated CI', to clearly indicate that the CI is on its way out. Or 'monolithic CI', to indicate the nature of its implementation.
| paths-ignore: | ||
| - ".cd/**" | ||
| - ".github/cd-config.yml" | ||
| pull_request: |
Collaborator
There was a problem hiding this comment.
Does labeled/unlabeled cover the functionality that prevents external contributor PRs from being executed, until they are labelled with approved-for-cd?
| ecflow: ecmwf/ecflow@${{ github.event.pull_request.head.sha || github.sha }} | ||
| skip_matrix_jobs: | | ||
| clang@macos-13-arm | ||
| codecov_upload: true |
Collaborator
There was a problem hiding this comment.
Is codecov_upload covered in the newly proposed CI?
| # platform stays ubuntu-24.04 — the same binary-compatibility class as the other | ||
| # repos — so the ecbuild and stack-deps artifacts built under that slug are | ||
| # reused rather than rebuilt. The boost image differs only in the tools it | ||
| # reused rather than rebuilt. The boost-qt6 image differs only in the tools it |
Collaborator
There was a problem hiding this comment.
Most of these comments (generated by AI?) seem unnecessary drivel...!
Consider removing the comments.
|
|
||
| target_clangformat(u_server CONDITION ENABLE_TESTS) | ||
|
|
||
| ecbuild_add_test( |
Collaborator
There was a problem hiding this comment.
Do not remove this test -- revert this change.
| # -DCMAKE_PREFIX_PATH=/path/to/jasper:/path/to/any/package/out/of/place | ||
| # -DBUILD_SHARED_LIBS=OFF | ||
| ############################################################################## | ||
|
|
Collaborator
There was a problem hiding this comment.
Change unrelated to new CI adoption -- revert this change.
| # are long-running system suites it skips. The HPC lane keeps its own | ||
| # `-E s_http` instead, per .github/ci-hpc-config.yml. | ||
| - name: Test | ||
| run: ctest --test-dir "${{ steps.build.outputs.build-dir }}" --output-on-failure -L nightly -E 's_test|s_zombies' -j 8 |
Collaborator
There was a problem hiding this comment.
It seems that ctest -L nightly -E 's_test|s_zombies' -j 8 appears in three different places:
.ci/manifest.toml
.github/workflows/ci.yml
.github/workflows/cross-repo-trigger.yml
Is this duplication necessary/unavoidable?
| @@ -0,0 +1,93 @@ | |||
| #!/bin/bash | |||
| # ecflow's HPC build recipe for the gnu toolchain, submitted as a SLURM job by | |||
Collaborator
There was a problem hiding this comment.
This header (and overall comments) in these files see duplicate and mostly unnecessary.
If these are deemed really necessary, consider moving them to a single .ci/hpc/README.md; and reference the README instead.
marcosbento
requested changes
Aug 27, 2026
| build-type = "Release" | ||
| runs-on = "arc-runner-very-large" | ||
| container = "eccr.ecmwf.int/public-playground-ci/ubuntu24.04-gfortran13-boost:latest" | ||
| container = "eccr.ecmwf.int/public-ci-images/ubuntu24.04-gfortran13-boost-qt6:latest" |
Collaborator
There was a problem hiding this comment.
macOS is a major target for ecflow, specially for the UI.
Apart from Linux runners, for ecflow we definitely need to consider (at least one) macOS runner -- to ensure full coverage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Contributor Declaration
By opening this pull request, I affirm the following:
🌦️ >> Documentation << 🌦️
https://sites.ecmwf.int/docs/dev-section/ecflow/pull-requests/PR-414