Skip to content

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