fix: limit external-eic-shell error reports to downstream use only#4907
fix: limit external-eic-shell error reports to downstream use only#4907paulgessinger merged 3 commits intoacts-project:mainfrom
Conversation
github-actions
Bot
added
the
Infrastructure
|
@tmadlener Maybe you want something like this for key4hep builds? |
|
RFR @paulgessinger Does this seem like something you'd be ok with? It opens a risk in that the ACTS build itself can build in all ACTS testing systems but fail inside the EIC environment. That's currently not a hard failure, but would become one. |
|
|
hi @wdconinc we never merged this, do you want to rebase and we try again? |
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
|
Rebased (a few times until I got it right). Let's see in CI if made any remaining mistakes. |
There was a problem hiding this comment.
Pull request overview
Adjusts the external_eic-shell CI job behavior so that ACTS build failures are treated as primary pipeline failures (and don’t generate downstream noise), while downstream (EICrecon) failures remain reported via the existing comment/artifact mechanism.
Changes:
- Split the prior combined external build step into separate “Build ACTS” (hard-fail) and “Build EICrecon” (soft-fail) steps.
- Update the PR comment artifact generation/upload conditions to trigger only on EICrecon failures.
- Change the intended install prefix used between the two steps to a workspace-mounted path (
/work/prefix) so it can be reused across steps.
Comments suppressed due to low confidence (1)
.github/workflows/builds.yml:463
export CMAKE_INSTALL_PREFIX=/work/prefixdoes not set CMake’s install prefix by itself. Since the configure call doesn’t pass-DCMAKE_INSTALL_PREFIX=...(and the install call doesn’t use--prefix), Acts will likely still install to the default prefix (e.g./usr/local), while the EICrecon step pointsActs_ROOTat/work/prefix. Please set the install prefix explicitly during the Acts configure (or duringcmake --install) so the installed package ends up under/work/prefixas intended.
ccache -z && \
export CMAKE_INSTALL_PREFIX=/work/prefix
export CMAKE_CXX_COMPILER_LAUNCHER=ccache
echo "::group::Acts configure phase"
cmake -B build -S . \
-GNinja \
-DACTS_BUILD_EXAMPLES=ON \
-DACTS_BUILD_FATRAS=ON \
-DACTS_BUILD_PLUGIN_DD4HEP=ON
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
/ci-bridge-run |
|
Some copilot related job fails, which blocks merge sentinel. I'll merge this manually after #5277 |
Limit the types of errors that external-eic-shell reports in the summary to build errors after ACTS has been successfully built, and treat failure to build ACTS itself as a pipeline error.
--- END COMMIT MESSAGE ---
With the notifications of failures in the external-eic-shell stack, we now also receive notifications of code in PR that is not yet ready where ACTS fails to compile (rather than this being a change in interface that EIC needs to be aware of). This is not actionable for EIC, so the notifications are noise. Example is https://github.com/acts-project/acts/actions/runs/20168575581/job/57898016435
This PR modifies the strategy to treat build failures of ACTS within the EIC environment as a primary error that will fail a pipeline (instead of only reporting a failure in the comment). This is unlikely to suppress true EIC-specific issues, which would be cases where ACTS builds fine, but somehow the EIC environment can't build it.