Skip to content

CI: gh-actions - add gh at apt-x64 step #15527

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

Closed
wants to merge 1 commit into from

Conversation

Ayesh
Copy link
Member

@Ayesh Ayesh commented Aug 21, 2024

Related: GH-13296.
ASAN builds use a different base image on GitHub Actions, which does not have the gh binary installed. This adds gh to the apt install list to make sure it's available on all builds.

@cmb69
Copy link
Member

cmb69 commented Aug 21, 2024

Wow, that was fast!

I was just going to reply:

I'm not sure how important testing with Caddyserver is for ASan builds (it seems only a couple of tests would be affected). If it's not that important we could just exclude the setup for that job. @iluuu1994, thoughts?

Well, if the gh installation doesn't take long, I'm fine with that either.

But I would prefer to have a clear indication if the setup fails. In #13542, @bukka said:

Download often fails so we just try to download it.

If the failing downloads are about LINUX_X64_DEBUG_ZTS_ASAN builds, then this issue would be solved, and we could remove the continue-on-error: true again.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, thanks @Ayesh!

If the failing downloads are about LINUX_X64_DEBUG_ZTS_ASAN builds, then this issue would be solved, and we could remove the continue-on-error: true again.

Ack

@Ayesh
Copy link
Member Author

Ayesh commented Aug 21, 2024

continue-on-error: true again.

Downloading Caddy binaries using gh should only rely on GitHub itself, and if GitHub is down, it's likely that the build will fail either way. Counting on @TimWolla, he might have a more informed opinion.

@TimWolla
Copy link
Member

ASAN builds use a different base image on GitHub Actions, which does not have the gh binary installed.

Ah, this is because ASAN is running in a container (introduced in #12034). Given that an Ubuntu 24.04 image is now available it might make sense to switch the ASAN builds to not run the build in a Docker container instead? Especially since Ubuntu 23.04 is already EOL.

@TimWolla
Copy link
Member

Downloading Caddy binaries using gh should only rely on GitHub itself, and if GitHub is down, it's likely that the build will fail either way.

Technically release downloading or the API could be down even when the actions subsystem or the git service is not. But I agree that this is unlikely. I don't particularly care either way, though.

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

"Request changes" for visibility of my first comment (and to remove this from my review requests).

Related: phpGH-13296.
ASAN builds use a different base image on GitHub Actions, which does not have the `gh` binary installed.
This adds `gh` to the `apt install` list, to make sure it's available on all builds.
@Ayesh Ayesh force-pushed the caddy-asan-build-fixes branch from 92e6a73 to de45269 Compare September 25, 2024 10:09
@cmb69
Copy link
Member

cmb69 commented Sep 25, 2024

@iluuu1994, thoughts on #15527 (comment)?

@iluuu1994
Copy link
Member

I'll look into dropping docker for the asan build.

@TimWolla
Copy link
Member

TimWolla commented Sep 29, 2024

I believe this is now obsolete with 91c0679?

@Ayesh
Copy link
Member Author

Ayesh commented Sep 29, 2024

You are right, I think that commit indeed makes this obsolete. GitHub's own images should have gh installed, at least on amd64 images.

Thank you for the reminder, I will close this now.

@Ayesh Ayesh closed this Sep 29, 2024
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.

4 participants