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

Aftermath of making PCRE2 the default #13103

Closed
straight-shoota opened this issue Feb 21, 2023 · 4 comments · Fixed by #13105
Closed

Aftermath of making PCRE2 the default #13103

straight-shoota opened this issue Feb 21, 2023 · 4 comments · Fixed by #13105
Assignees
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:text

Comments

@straight-shoota
Copy link
Member

straight-shoota commented Feb 21, 2023

When we merged the change to use PCRE2 by default in (#12978), several things broke.
For some reason, the CI builds on the PR were all green. Only later we started so say failing builds.
Some of that can be attributed to the docker build images, which originally lacked libpcre2, which we have since added. But I have no idea how the tests in environments that don't use the docker build images and clearly need fixes for PCRE2 support could've possibly succeeded.

The failures are cause by a couple of different bugs, either in the infrastructure setup or with portability of the libpcre2 bindings.
Fixes are in these PRs:

Since they fix independent bugs, each of them is only a partial contribution to make CI green again, but every single one still has failing jobs.
I've merged them all together into a CI test branch to make sure we're good when all are merged.
https://github.com/crystal-lang/crystal/tree/ci/test-pcre2-fixes
CI is all green, except for the release workflow (which is expected; I just included this test for informational purposes) and the occasional interpreter test failure (re-running that again, but this doesn't seem to be related to PCRE2).

So all looks good with these PRs and I'm gonna start merging them (as far as they are approved).

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:text labels Feb 21, 2023
@straight-shoota straight-shoota self-assigned this Feb 21, 2023
@beta-ziliani
Copy link
Member

They are all approved, however, I repeat what I said before: I still think they should all go in the same commit. Ideally each commit should take us from green to green, except if there’s really a necessity. These commits all contribute to fixing the same issue, even if from different angles. Imagine doing a bisect on a regression and finding it fails but for a completely different reason, because your problem was in an architecture that happened to be broken in one of these commits. Each PR in the commit can tackle each issue, no need to such atomic PRs.

@beta-ziliani
Copy link
Member

(Note: this is not a hill I will die on)

@Sija
Copy link
Contributor

Sija commented Feb 21, 2023

@beta-ziliani I guess this could be achieved by creating a meta-pr into which all of the above would be merged beforehand.

@straight-shoota
Copy link
Member Author

straight-shoota commented Feb 22, 2023

I see your point. Makes sense from that perspective, although I'm not a fan of merging largely unrelated changes in one. And some of the issues appear dependent on the build environment (that's why we didn't see them directly on #12978, only after updating the build docker images).

I guess an alternative could be to revert #12978, then apply the fixes individually and then re-apply #12978 when it's all green. But it's all a bit messy either way.

I'm happy to squash them all together in a meta PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:text
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants