Skip to content
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

Nightlies for security branches? #16286

Open
cmb69 opened this issue Oct 7, 2024 · 24 comments
Open

Nightlies for security branches? #16286

cmb69 opened this issue Oct 7, 2024 · 24 comments

Comments

@cmb69
Copy link
Member

cmb69 commented Oct 7, 2024

Description

When I applied #16097 the other day, I was prepared to see many of our 8.1 nightly jobs fail (Linux due to not yet applied #16107, and macOS due to some build error). Checking the builds today revealed that Linux already fails due to a missing backport of #16011 (or some alternative solution), and the some of the Community steps are also failing (apparently, due to newer versions of the libraries/frameworks/tools, which are no longer compatible with PHP 8.1).

It is obviously hard to keep these builds green, if the run only occasionally (often not for weeks), and @iluuu1994 convinced me that nightly runs of all branches would be too much. And it also might not be the best idea to target security branches for all potentially relevant CI fixes. While something like #16144 (if targeted at PHP-8.1) might help, it may still be a lot of work to cater to all those issues.

So I wonder what to do. It seems to me that the Community builds don't make much sense anyway (since PHP-8.1 should be stable enough to not introduce unexpected breaks). And assuming the we'll merge #16148 (which I'm planning to do), we already have the ability to run the push workflows, which should catch the most serious issues. Okay, no Asan and MSan builds, and no uncommon configurations, but these should ideally be run before we merge security fixes anyway.

So I tend to suggest to exclude PHP-8.1 from the nightly runs. If not, we should at least resolve the most serious issues, so that the nightly runs are somewhat meaningful.

@iluuu1994
Copy link
Member

I agree that running old branches in nightly is meaningless if they are all red. I would not object to trying to fix the 8.1 issues now and seeing how much work it is to keep them green (or mostly green, at least). It could make sense to run all branches on Monday, for example. Then, we would find new issues on 8.1 every week instead of every few months. As mentioned previously, the configurations for the branches are diverging, which is one of the main annoyances. I have not yet looked into shared workflows, which might be a solution. Another might be always checking out ./.github from master, so that we can manage it in one place.

Community builds don't make much sense anyway (since PHP-8.1 should be stable enough to not introduce unexpected breaks).

Note that community builds run with ASan, so they are actually quite useful. It is possible though that they start dropping PHP versions that still receive security patches, in that case it would definitely make sense dropping them.

@iluuu1994
Copy link
Member

I brought this up in todays foundation meeting. Here's the summary:

We should strive for CI of security branches to stay green. Fixes to CI itself and tests should target security branches from now on. Small code fixes may be discussed on a case-by-case basis with RMs of the given security branch. Alternatively, the given errors/warnings may be ignored with -Wno- flags. Security branches should be run in the nightly workflow on a weekly basis.

Does this sound ok to you? I will now work on making the builds green, and then update the #general channel on Slack to inform everybody to send CI patches (including fixing of tests) to security branches from now on.

@cmb69
Copy link
Member Author

cmb69 commented Oct 28, 2024

That sounds great!

iluuu1994 added a commit that referenced this issue Oct 28, 2024
Closes GH-16469

Working towards GH-16286

commit e0db221143b808d97bc3a44e9f0968c6308794b4
Author: Ilija Tovilo <ilija.tovilo@me.com>
Date:   Fri Oct 25 22:48:20 2024 +0200

    Move CFLAGS into ./configure command for consistency

commit 8ad67768250d181cd7fef30e0c866625bbd8ac94
Author: Ilija Tovilo <ilija.tovilo@me.com>
Date:   Fri Oct 25 22:47:03 2024 +0200

    Also upgrade nightly to macOS 13

commit 58a88cc
Author: Ilija Tovilo <ilija.tovilo@me.com>
Date:   Wed Oct 23 19:07:59 2024 +0200

    Fix call to dc[n]gettext in tests with 0 $category

    This causes a segfault on PHP-8.1

commit 611af05
Author: Ilija Tovilo <ilija.tovilo@me.com>
Date:   Fri Dec 8 13:36:52 2023 +0100

    [skip ci] Skip intermittently failing curl test on macOS

    The test fails with "CURL ERROR: 56". I will create an issue for it shortly.

commit ec74517
Author: Ilija Tovilo <ilija.tovilo@me.com>
Date:   Wed Oct 23 19:05:32 2024 +0200

    Backport parts of 9999a0c for gettext

    See 9999a0c

commit 5ce7034
Author: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date:   Sun Jul 28 14:34:26 2024 +0200

    Fix CI failure on macOS after Curl update

commit 714a3e7
Author: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date:   Sat Jul 27 16:09:50 2024 +0200

    Fix CI failure after Curl update (#15124)

commit 4f2eb92
Author: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date:   Thu May 23 22:20:37 2024 +0200

    Fix GH-14307: Test curl_basic_024 fails with curl 8.8.0

    Curl changed the behaviour, from the changelog:
      - lib: make protocol handlers store scheme name lowercase curl/curl@c294f9c

    From the docs: "The returned scheme might be upper or lowercase. Do
    comparisons case insensitively."

    Closes GH-14312.

commit 251195b
Author: Ayesh Karunaratne <ayesh@aye.sh>
Date:   Thu Feb 1 02:03:55 2024 +0700

    ext/curl: Fix failing tests due to string changes in libcurl 8.6.0

    Upstream libcurl 8.6.0 contains a change[^1] that caused a test failure.
    This fixes it by updating the test's `EXPECTF` to use a regex to account for both string patterns.

    [^1]: curl/curl@45cf4755e71f#diff-a8a54563608f8155973318f4ddb61d7328dab512b8ff2b5cc48cc76979d4204cL1683

    Closes GH-13293.

commit fc5d83f
Author: Christoph M. Becker <cmbecker69@gmx.de>
Date:   Wed Oct 16 22:46:20 2024 +0200

    Prepare for necessary move to macOS 13

    GH will remove macOS 12 runner images as of December 3rd, so we prepare
    for that.

    Besides the obvious need to change the runner, we also suppress a
    couple of warnings, because otherwise the build would fail due to
    `-Werror`.
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Oct 28, 2024
See phpGH-16286. The objective is to identify failed builds in security branches
semi-early. Previously, they would only be run when a fix was backported, which
would almost always result in a red build.
iluuu1994 added a commit that referenced this issue Oct 28, 2024
See GH-16286. The objective is to identify failed builds in security branches
semi-early. Previously, they would only be run when a fix was backported, which
would almost always result in a red build.
@nielsdos
Copy link
Member

Is this resolved now by 562677a ? I would think so unless I'm missing something?

@cmb69
Copy link
Member Author

cmb69 commented Oct 28, 2024

I think so. Maybe @iluuu1994 wants to keep it open for a while to check whether everything works as expected.

@iluuu1994
Copy link
Member

I think other builds still fail (freebsd). Unfortunately, I can't check as we're out of free credits.

@cmb69
Copy link
Member Author

cmb69 commented Nov 30, 2024

FreeBSD has been switched to GH (using emulation), and that works fine. However, macOS fails despite #16789, and despite that icu4c@74 is in PKG_CONFIG_PATH but not icu4c@76 (and the latter is not linked either).

@iluuu1994
Copy link
Member

@cmb69 Any suggestions on that part? We should also backport all of the flakiness checks containing the message "Occasionally segfaults on macOS for unknown reasons", as this seems to happen very frequently. The last issue is a JIT issue in the community build for one of the Symfony packages, which was fixed for stable branches in GH-16858. I just asked the RMs whether it's ok to backport it. If not, I will skip the affected package.

@cmb69
Copy link
Member Author

cmb69 commented Nov 30, 2024

No idea how to solve this. And even if we solve this, there will be new issues (at least when icu@74 will be no longer supported by Homebrew in May).

@iluuu1994
Copy link
Member

@cmb69 So, drop intl from the build?

@cmb69
Copy link
Member Author

cmb69 commented Nov 30, 2024

Or build ICU 74.2 manually, and cache it.

@iluuu1994
Copy link
Member

@cmb69 But if PHP 8.1 only works with ICU 74, and homebrew drops support for ICU 74, then what's the point of testing it? I highly doubt macOS folks will compile ICU on their own.

@cmb69
Copy link
Member Author

cmb69 commented Dec 1, 2024

I really don't know how people work on macOS, but there are other package managers, and it might even possible to simply avoid the updates. I'm also fine with dropping ext/intl from macOS CI builds, but I guess that RMs should have a say about this.@patrickallaert, @ramsey, thoughts?

@ramsey
Copy link
Member

ramsey commented Dec 1, 2024

despite that icu4c@74 is in PKG_CONFIG_PATH but not icu4c@76 (and the latter is not linked either)

I've been struggling with this locally. Something is still referencing icu4c@76 somehow. In the compilation logs, I continue to see attempts to use the 76 headers instead of the 74 ones.

What would cause this?

@iluuu1994
Copy link
Member

and it might even possible to simply avoid the updates

I don't use homebrew anymore, but I specifically remember running into major issues because homebrew removes unsupported packages immediately. When just installing a new package, homebrew would also update all of the installed packages, and even removed unsupported ones. OpenSSL specifically was an issue. I wouldn't count on things continuing to work once the package is removed.

@patrickallaert
Copy link
Contributor

I really don't know how people work on macOS, but there are other package managers, and it might even possible to simply avoid the updates. I'm also fine with dropping ext/intl from macOS CI builds, but I guess that RMs should have a say about this.@patrickallaert, @ramsey, thoughts?

No opinion here, also never ever used a Mac. @ramsey can have a final word here.

@ramsey
Copy link
Member

ramsey commented Dec 8, 2024

I really don't want to drop ext/intl from macOS builds, but this is a big PITA for building PHP 8.1 on macOS. I haven't heard a lot of complaints about this, though, so maybe there aren't enough folks still building PHP 8.1 on macOS, which means we can probably drop this from the macOS CI builds.

@bukka
Copy link
Member

bukka commented Dec 8, 2024

I would prefer to patch it. It's really small change and we have already done something similar for libxml . I don't see an issue to introduce fixes of this kind to security only branches. We should probably update our policy though.

@bukka
Copy link
Member

bukka commented Dec 8, 2024

Hmm looking at the intl ext, it seems to me like updating might be a bit more effort and possibly something that should not be done in 8.1. We could potentially build our own version but it might be too much effort. I think it might be best to drop it from MacOS build and look into it only if we have some security issue in intl extension (which is not that common so it might not actually happen).

iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Dec 9, 2024
Based on the discussion in phpGH-16286, drop the intl build from macOS + PHP 8.1,
since we cannot build with supported intl versions without too many changes.
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Dec 9, 2024
Based on the discussion in phpGH-16286, drop the intl build from macOS + PHP 8.1,
since we cannot build with supported intl versions without too many changes.
iluuu1994 added a commit that referenced this issue Dec 9, 2024
Based on the discussion in GH-16286, drop the intl build from macOS + PHP 8.1,
since we cannot build with supported intl versions without too many changes.

Closes GH-17092
See GH-16286
@bukka
Copy link
Member

bukka commented Jan 4, 2025

@iluuu1994 I assume this can be now closed, right?

@iluuu1994
Copy link
Member

I suppose, although it's not fully solved yet. I think the community tests are inherently problematic. For example, Symfony seems to have already dropped 8.1 support on their default branch (7.3). It also still errors with asan, and new issues have also popped up. Not sure what the best way forward is for those. ext/fileinfo/tests/cve-2014-3538-nojit.phpt now apparently also fails on macOS release builds (or we got unlucky with the same flaky test failing twice, not sure, https://github.com/php/php-src/actions/runs/12540512801/job/34967916493).

@bukka
Copy link
Member

bukka commented Jan 4, 2025

Maybe we could use some stable versions that till supports 8.1 for community tests? Not sure why ext/fileinfo/tests/cve-2014-3538-nojit.phpt does not work. I would expect it to still work so it might be a problem with the test but don't know that much about fileinfo ext.

@cmb69
Copy link
Member Author

cmb69 commented Jan 4, 2025

Not sure why ext/fileinfo/tests/cve-2014-3538-nojit.phpt does not work.

The test is performance sensitive, and terminates after 1 second. Perhaps just a timing issue (especially when running tests in parallel).

@iluuu1994
Copy link
Member

Maybe we could use some stable versions that till supports 8.1 for community tests?

We could switch to Symfony LTS (6.4) for 8.1. This may also become an issue for other libraries, so it may become another slightly annoying maintenance burden.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants