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

Make it possible to build with c++20 and switch to new Key4hep build action #114

Merged
merged 3 commits into from
Feb 5, 2024

Conversation

tmadlener
Copy link
Contributor

@tmadlener tmadlener commented Feb 1, 2024

BEGINRELEASENOTES

  • Allow to build with c++20
  • Switch to new key4hep build action and remove old workflow

ENDRELEASENOTES

@tmadlener tmadlener changed the title Make it possible to build with c++20 Make it possible to build with c++20 and switch to new Key4hep build action Feb 1, 2024
@tmadlener tmadlener enabled auto-merge (squash) February 1, 2024 16:32
@@ -3,6 +3,7 @@ name: Key4hep build
on:
push:
workflow_dispatch:
pull_request:
Copy link
Contributor

@jmcarcell jmcarcell Feb 4, 2024

Choose a reason for hiding this comment

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

This will get overwritten if it's not changed in key4hep-actions. Why is this needed? With the push: they run whenever there is a push in a branch, in a PR or not, pull_request: will duplicate the jobs for a PR since they will run in the PR and after push meaning it takes longer to find workers and for all the jobs to run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the pull_request they will not report back to the PR. The push hook only runs them on the branch (which is in my fork). Maybe push: master and pull_request: is the way to go?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think that's it. The repos have to have the same name for the default branch though but in this case it's true for key4hep

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah no I just remembered why I didn't go for that. With that, the workflows don't run in your workflow when you push to a different branch other than the default one and some people don't like that. I couldn't find a way of having it

  • Run once in PRs
  • Run also when pushing to a fork
  • Report in the PR page
    The closest is the first two without the third one with only push:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. But the one that we definitely need is the reporting, right? Otherwise we cannot use auto-merge with required checks.

I also didn't find a way to restrict the push to only the main repository, which I guess would be nice, because then there would be no duplicate reporting of failures.

@jmcarcell
Copy link
Contributor

The spack recipe was missing having the standard specified so it was being built with C++17 instead of C++20 for the nightlies

@tmadlener tmadlener merged commit ff0bbc8 into key4hep:main Feb 5, 2024
18 checks passed
jmcarcell added a commit that referenced this pull request Feb 5, 2024
…ible to build with c++20 and switch to new Key4hep build action (#114)
@tmadlener tmadlener deleted the allow-c++20 branch February 5, 2024 14:14
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.

2 participants