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

release pa_ppx.0.16 to support ocaml 5.3.0 syntax changes #27092

Merged
merged 11 commits into from
Dec 19, 2024

Conversation

chetmurthy
Copy link
Contributor

what it says on the can.

@chetmurthy
Copy link
Contributor Author

Looks like everything passed except for Windows. I think this is good-to-go ?

@@ -60,6 +60,8 @@ depends: [
conflicts: [
"ocaml-option-bytecode-only"
"pa_ppx_migrate" { <= "0.11" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not noticing this in earlier prs. Why are these added as conflicts instead of having the correct upper bounds in their respective opam files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[I deleted two replies, b/c they were invalidated by the results of CI runs. I think this is an accurate answer]

For the bytecode-only option, a reviewer suggested it b/c (IIRC) there was no way to indicate to only build bytecode binaries on a platform that lacked native-code compiler.

For the others (e.g. for pa_ppx_migrate), I added them here b/c it seemed convenient. But when I moved the constraint pa_ppx < "0.16" to the file pa_ppx_migrate.0.11, the CI build for that package failed on Debian, b/c the package libpcre-dev is no longer available on Debian. The CI build failed when trying to build "conf-libpcre".

More modern versions of camlp5 and pa_ppx_* all rely on pcre2, not pcre, so that problem should be going away.

But this does bring up a mystery and maybe a problem: I have one package, pa_ppx_regexp that depends on all of re, pcre, and pcre2. B/c it allows the user to choose which "backend" to use for their regexps. I just released a new version of this package, which went thru fine (you approved it yesterday). So I don't know why that went thru.

Concretely, and this is an answer I would not have had before going thru this (ahem) "process of discovery", the reason maybe it's useful to put these constraints here in the new package, is that it means you don't have to build the old packages that you might have modified otherwise, and that can mean avoiding doing a bunch of spurious builds that might even fail for entirely unrelated reasons.

Whew!

Sorry, hope this is a good-enough answer. I'm writing this before the CI run completes, so hopefully I won't be deleting it and writing -another- reply. grin

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, the CI run passed, as it did before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the bytecode-only I agree, my question was concerning the packages. If they are incompatible with pa_ppx, I would drop conflicts here and move the upper bounds on the respective packages. It is easier for the solver to deal with them than via conflicts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do that in 2 stages. First add the relevant upper bounds on the other packages, then remove the conflicts from this. I can help in a couple of days if you want. And sorry again for not noticing it before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I can move them, but then some of those respective packages will not build (or their deps/revdeps won't build -- is that OK?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's all right.

By the way, if you set an x-maintenance-intent field on your latest release, or if you flag the old ones as deprecated while you decide what you prefer to maintain, we can move them to the opam-repository-archive in the next stages of cleanup. See Plan and recent Announcement for more details

@mseri
Copy link
Member

mseri commented Dec 16, 2024

Thanks a lot for the fast changes

@chetmurthy
Copy link
Contributor Author

@mseri thank -you- for the quick turnaround on these reviews! There's a million of us, but only a few reviewers/maintainers.

@chetmurthy
Copy link
Contributor Author

I tried to get involved as a novice reviewer, but the timezone difference was just too much for me -- I couldn't hack waking up early enough to get to the meetings!

@mseri
Copy link
Member

mseri commented Dec 16, 2024

I understand perfectly, thanks a lot also for trying

@chetmurthy
Copy link
Contributor Author

OK, everything worked except for 2 opensuse builds that failed b/c the opensuse base image already contains a "diff", and when I try to install GNU diffutils, it won't allow it -- so the build fails. I've marked that package with

x-ci-accept-failures: [ "opensuse-tumbleweed" ]

but I'm still getting the "opam-ci" task marked as a faiure.

@chetmurthy
Copy link
Contributor Author

chetmurthy commented Dec 19, 2024

OK, the only failures are for opensuse-tumbleweed, for installing the system package "diffutils".

BTW, on arm64 there was a failure due to an infinite-loop running tests (pa_ppx_regexp), but when I clicked to "rebuild" it, it went thru fine.

@shonfeder
Copy link
Contributor

I tried to get involved as a novice reviewer, but the timezone difference was just too much for me -- I couldn't hack waking up early enough to get to the meetings!

If you'd like to try again at some point, I am in US Eastern Time, in case that is a better fit for your schedule, and would be happy to have some orientation meetings with you. LMK :)

OK, everything worked except for 2 opensuse builds that failed b/c the opensuse base image already contains a "diff", and when I try to install GNU diffutils, it won't allow it -- so the build fails. I've marked that package with

x-ci-accept-failures: [ "opensuse-tumbleweed" ]

but I'm still getting the "opam-ci" task marked as a faiure.

I cannot figure out the moment why this is not working as intended. But this is clearly an error in our CI and not your package. A proper fix is probably to move this logic out of bash and into the main pipeline logic so it is easier to reason about :)

Thanks for publishing your updates!

@shonfeder shonfeder merged commit b7e2017 into ocaml:master Dec 19, 2024
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants