-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Alpine CI #13925
Alpine CI #13925
Conversation
1bd310e
to
79c9204
Compare
.github/workflows/push.yml
Outdated
asan: false | ||
- debug: true | ||
zts: true | ||
asan: true |
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 suggest just the latter, two builds seem like overkill to me. Instead, you could add both to nightly.
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.
Honestly, I wonder if it wouldn't be more useful to just move this to nightly but with all extensions enabled. WDYT? Or do you think this tests something unique (apart from alpine/musl)?
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.
Honestly, I wonder if it wouldn't be more useful to just move this to nightly but with all extensions enabled. WDYT? Or do you think this tests something unique (apart from alpine/musl)?
I planned to also add a nightly job after this, but I don't mind moving this to nightly initially.
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 won't object to either, but IMO the push itself is not a big value-add if it's unlikely to break, without the other builds breaking.
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've moved the job to nightly (tested in https://github.com/arnaud-lb/php-src/actions/runs/10388373807/job/28763941967). For now I didn't enable all extensions, but I may do later (in a separate change).
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 is not redundant. Have you seen the OpenSSL issue list: #14036 ? This already shows that there are significant enough differences. Catching issues after the merge usually takes time and it is also riskier for releases.
As I mentioned those are low resource jobs so the load on planet is minimal (you should really see what normally runs in enterprise pipelines where usually you don't see any nightly runs and lots of jobs are high resource ones - everything is often done on push).
The way how I see it is to have only long running and pipelines with not enough credits to be run in nightly. Anywya we don't probably agree on this so I will leave decision to you as you are the code owner for this part. Feel free to remove the push then.
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.
As mentioned, I don't inherently object to increasing CI time, but I do think we should keep it minimal, rather than keep growing it until it becomes a problem. I don't know what the OpenSSL issue is about, but since this change wasn't even caused by us, it wouldn't have prevented the failing of release branches either.
Anyway, this discussion has already gone on too long to be useful. I'll leave it up to Arnaud to decide whether to keep push or remove it.
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.
@arnaud-lb I understand @iluuu1994 as any added CI pipeline increases the CI time meaning not only more energy used but also CI might delay other CIs for different PRs.
However I understand @bukka as well. Push CI pipelines are great for evaluating if PR or any commit is working properly.
I was neutral on this because of these pros and cons, but in past I strugled with #13771 many times and as reported there, many times this was left unnoticed for a while on master branch.
Based on this, I would support having Alpine tested on push with #13771, ie. one push job where the extensions would be build separately testing if the header files and dependencies of each extension are defined properly on push.
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.
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.
#13771 about every second month the build was broken, so I would say it should be tested on push as it is not only an Alpine issue, but a build issue in general. But I am not the one who decide if it should be for nightly only or not.
And thank you for partnering in this PR and taking it to the finish line ❤🎉
93987b0
to
197fb39
Compare
This test is failing on Alpine as GBK encoding is probably not present, can some extra Alpine dependency fix it or should it be skipped on Alpine? Then is there anything else remaining in order to merge this PR? |
Yes I believe this is the last blocker. I'm trying to fix this test in #15072 :) |
@arnaud-lb I guess all prereqs are merged, right? Would be good to get review from @iluuu1994 or @TimWolla . From the quick look, it looks good to me but will leave to code owners to do a proper review. It will be really great to have Alpine CI... |
@bukka yes all prereqs are merged and the build is stable. Reviews welcome :) |
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.
Some remarks regarding the GitHub Action usage. Can't comment on the build steps themselves, because I have opinions about supporting Musl in the first place :-)
testArtifacts: | ||
default: null | ||
required: false |
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.
Unused.
.github/workflows/push.yml
Outdated
- name: Test Tracing JIT | ||
uses: ./.github/actions/test-alpine | ||
with: | ||
testArtifacts: ${{ matrix.debug && 'DEBUG' || 'RELEASE' }}_${{ matrix.zts && 'ZTS' || 'NTS' }}${{ matrix.asan && '_ASAN' || '' }}_Tracing JIT |
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.
Unused.
--enable-fpm \ | ||
--with-pdo-mysql=mysqlnd \ | ||
--with-mysqli=mysqlnd \ | ||
${{ inputs.skipSlow == 'false' && '--with-pgsql' || '' }} \ |
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.
There is no input called skipSlow
.
I will merge this week unless there are objections |
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.
LGTM otherwise
sapi/cli/php run-tests.php -P -q ${{ inputs.runTestsParameters }} \ | ||
-d opcache.jit=${{ inputs.jitType }} \ | ||
-d opcache.jit_buffer_size=64M \ | ||
-j$(($(nproc) - 1)) \ |
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.
You can probably do -j$(nproc)
. The -1
was used as an attempt to fix crashes specific to macOS.
.github/workflows/push.yml
Outdated
asan: false | ||
- debug: true | ||
zts: true | ||
asan: true |
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.
Honestly, I wonder if it wouldn't be more useful to just move this to nightly but with all extensions enabled. WDYT? Or do you think this tests something unique (apart from alpine/musl)?
Co-authored-by: Michael Voříšek <mvorisek@mvorisek.cz>
This reverts commit f309b05.
This adds an Alpine job to the push workflow.
See #13877
The "Alpine CI" commit is based on #8621 (I will add @mvorisek as co-author).
I will open separate PRs for other changes:
Most changes are not musl-specific and benefit other platforms.
The musl iconv implementation's behavior differs considerably from the glibc one. Currently the
iconv
tests rely on the glibc behavior, and it is required to build the extension against thegnu-libiconv
package. This solves discrependices in theiconv
extension, but unfortunately some libraries use musl iconv, at least on Alpine. For instance libxml does, and this affects the library's ability to handle some charsets.The
icu-data-full
package is also required because Alpine stripes locale data from the ICU package.I believe that the
hunspell-en
andenchant2-hunspell
packages also helped to make theenchant
test suite pass.