Enable ccache for Alpine builds on Travis CI #927
Conversation
|
I'm off for the holiday, just happened to take a look and see the oddity
and figured I'd point it out.
I can take a look Monday, but the security release is Tuesday isn't it?
That's not a lot of time to get fixes here and in official images before it
drops, so I think the revert would be simpler and safer so that can be
revisited following the release.
|
|
Unfortunately I think he's right. We should revert the two alpine/ccache commits and revisit in a new PR. I'm not sure we have enough time to try and get #850 fixed and merged for next week, but that's probably more doable and something we want to happen before the security release. We also need to make the upstream PR for #921. I'm not sure we want to have a security release include a switch from jessie to stretch, but at this point we have no choice I guess. I'm not going to be available much today or tomorrow, still sick 🤧 |
|
@tianon @chorrell @PeterDaveHello do you think this is sufficient for now? |
|
I think it should be a full revert, not just commenting out the code in the Dockerfiles |
|
And it will have to be a brand new PR. I was not able to use the revert --> PR option in Github for both commits |
|
@chorrell full revert of just the Dockerfile? I think we can keep the Travis CI part no? |
|
I guess if it's the easier thing to do and it doesn't affect the images or build times, sure. |
PeterDaveHello
force-pushed
the
enable-ccache-for-TravisCI-alpine-builds
branch
from
ee2d430 to
97e6127
Compare
PeterDaveHello
changed the title
|
@tianon @chorrell @LaurentGoderre @nodejs/docker please help review this PR, thanks. |
PeterDaveHello
force-pushed
the
enable-ccache-for-TravisCI-alpine-builds
branch
from
97e6127 to
1e7b77e
Compare
|
I feel that this travis-only injection means that you won't even be testing the same thing that is submitted to official-images. |
PeterDaveHello
force-pushed
the
enable-ccache-for-TravisCI-alpine-builds
branch
from
1e7b77e to
1bf8dbb
Compare
|
@yosifkit you are right, I hope if those are only for ccache, would be okay? |
|
I think what he's getting at is essentially, what's the point then? What
problem is it solving if only done for Travis here?
|
|
It's helping speed up the build time on Travis CI ;) |
|
Sure, but to what end? For what purpose? What problem does that solve?
Isn't the point of Travis to pre-verify changes here before submitting
them? If that's not the point, then IMO you might as well just disable
Travis because I don't see any other reason to use it in this way?
|
|
For a long time and still now, we spend so much time on Travis CI, especially when we have multiple PRs or branches triggered the CI pipeline, which slows down how fast we can deliver the updates, that's a frequently complained issue from users. Travis CI still very every changes, just with additional ccache to help speed it up. It saves time, reduces builds pending, and helps release new versions and verify changes more quickly. The ccache discussion started in #693, I didn't see any down vote there, #703 is also trying to use ccache another way, so I'm being confused here, is the problem here about ccache or the injection implementation? |
|
@tianon I think we ideally would want to solve it in your builds as well but that would be far more complicated. Reducing the time it takes to build those image is very worthwhile IMHO. |
|
This should not be needed anymore with the new unofficial build, in theory. |
|
The ccache or sccache optimization could be used in the build repo though https://github.com/nodejs/unofficial-builds |
|
Oh! Nevermind, it already is :) https://github.com/nodejs/unofficial-builds/blob/master/recipes/musl/run.sh#L19 |
|
I think we can close this one since we have the alpine build now |
Reimplement #918, only inject ccache related instructions on Travis CI build lifecycle.