-
Notifications
You must be signed in to change notification settings - Fork 697
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
Migrate release CI back to github #9437
base: master
Are you sure you want to change the base?
Conversation
001d0e1
to
bb01a96
Compare
Also includes:
|
5c37718
to
34a7c9c
Compare
3187a1a
to
8781d59
Compare
Workflow:
|
81397df
to
0800602
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my perspective this is quite appealing and I would like to give it a try for the next release.
Ironically, the GitLab pipeline took forever because the Darwin job had to queue |
That would mean we just increased complexity, which defeats the purpose of this PR. If anything goes extremely wrong during the next release, you can restore the gitlab code from the git history. |
If it wasn't clear, I meant to keep it around for the next release and then dropping it. I hope that doesn't defeat the purpose. This change is bigger than just reimplementing some code ,and keeping two systems running in parallel is good practice. And basically free in this case. |
Regarding this one point, I'd make it two releases and I'd keep the gitlab CI running all the time to ensure we spot problems in it ASAP, just as we do today. While I'm talking, @hasufell, thank you for the PR. I remember the big appeal of your proposal long ago, now materialised, was that we'd test what we release. Does this PR bring us closer to the point where we test daily the same binaries that will be released? In particular, are our normal GHA tests run on these artifacts, and when not applicable, on the exact commits from which the artifacts are created? What's the roadmap to unify CI and release CI, to a reasonable extent? |
I don't agree with this at all. There's no indication this is necessary. In HLS we also didn't need it and it has a much more complex CI. |
To avoid a deadlock I've created https://github.com/chreekat/cabal/tree/b/add-github-release-ci, which is an edited version of this branch that keeps GitLab CI. I hope it won't be necessary. |
I can split the PR in two commits, so that Gitlab removal is easier to revert in case it's necessary. But I won't subscribe to "let's keep code in master, just in case". That's bad engineering practice. |
I don't think anybody is proposing that. Gitlab CI would be running continuously so that we can spot any problems and fix them ASAP and during a release, if needed, the release manager would be able to compare the results of gitlab and github, test behaviour of the artifacts from both systems, etc. |
I have decided to build bindists myself for the next cabal release, so cabal's CI isn't relevant for GHCup directly anymore. You can use this PR and the contained patch as you please. |
I have reopened this to prevent us losing valuable work. |
And thank you @hasufell for working on this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just leaving some comments as I had the time to read this properly. It looks quite comprehensive! good stuff.
PS: @hasufell you have been clear that you don't intend to continue working on this so my comments are not to be taken as request for more work from you :-) They more like notes to my future self.
|
||
# https://github.com/haskell/cabal/issues/7313#issuecomment-811851884 | ||
if [ "$(getconf LONG_BIT)" == "32" ] || [ "${DISTRO}" == "CentOS" ] ; then | ||
echo 'constraints: lukko -ofd-locking' >> cabal.project.release.local |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should just be included in cabal.project.release
under a if arch(I386)
conditional. I am not sure if there is something specific to CentOS, it does not seem to be mentioned in the thread above
args=( | ||
-w "ghc-$GHC_VERSION" | ||
--disable-profiling | ||
--enable-executable-stripping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other things that should be in cabal.project.release
timeout_in: 120m | ||
only_if: $CIRRUS_TAG != '' | ||
env: | ||
ADD_CABAL_ARGS: "--enable-split-sections" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be implemented in cabal.project.release under a conditional if os(FreeBSD)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this be conditional on platform? AFAIK it's only Windows that doesn't support it, and it's a win everywhere else. (Macs ignore the option since they always do dead code stripping, although GHC needed a lot of hackery to make use of it safely, but that's not our problem.)
uses: actions/checkout@v3 | ||
|
||
- if: matrix.ARCH == 'ARM' | ||
uses: docker://hasufell/arm32v7-ubuntu-haskell:focal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there officially maintaned images we can use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect that there are official Ubuntu images, but they don't have any Haskell stuff preinstalled and if they did it'd be Ubuntu's ancient versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The images are autorebuilt: https://github.com/haskell/ghcup-hs/actions/workflows/docker.yaml
However, you should be using docker://hasufell/arm32v7-debian-haskell:10
.
See my latest action: https://github.com/stable-haskell/cabal/blob/cabal-install-v3.12.1.0/.github/workflows/release.yaml
ARTIFACT: ${{ matrix.ARTIFACT }} | ||
|
||
- if: matrix.ARCH == 'ARM64' | ||
uses: docker://hasufell/arm64v8-ubuntu-haskell:focal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
|
||
- name: git config | ||
run: | | ||
git config --global --get-all safe.directory | grep '^\*$' || git config --global --add safe.directory "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the current ci we just do
git config --global protocol.file.allow always
and we have a comment point to https://bugs.launchpad.net/ubuntu/+source/git/+bug/1993586 (cabal PR #8546)
I'd be willing to take this on after we get 3.12.1.0 out the door. |
Happy to help too. |
fbf1f85
to
8c78a11
Compare
I have temporarily reinstated the Gitlab CI stuff until we come to an agreement about it. (I'm on the side that it's always possible to arrange things so it can be reverted back in if we need it. I mean, that's what git is for, isn't it?) (And part of the reason I did that is so I can take it out in a distinct commit that can easily be reverted.) |
90334dd
to
3f2515c
Compare
Manually squashed, added |
Gitlab CI removed again, revert a12c6be to bring it back if needed. |
Authored-by: Julian Ospald (@hasufell)
Revert this commit at need.
f232a4a
to
a12c6be
Compare
No description provided.