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

ocaml 5.2.0 #172795

Merged
merged 22 commits into from
Jun 6, 2024
Merged

ocaml 5.2.0 #172795

merged 22 commits into from
Jun 6, 2024

Conversation

XVilka
Copy link
Contributor

@XVilka XVilka commented May 25, 2024

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@github-actions github-actions bot added the CI-build-dependents-from-source Pass --build-dependents-from-source to brew test-bot. label May 25, 2024
Copy link
Contributor

Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request.

@XVilka XVilka force-pushed the ocaml-5.2.0-version-bump branch from c65731c to 3d2bed1 Compare May 25, 2024 11:45
@XVilka

This comment was marked as resolved.

@XVilka

This comment was marked as resolved.

@XVilka XVilka force-pushed the ocaml-5.2.0-version-bump branch from 3d2bed1 to bd54380 Compare May 25, 2024 12:42
@XVilka

This comment was marked as resolved.

@chetmurthy
Copy link

I'm not sure where the test is in the brew build process, but the problem is that where you load "pr_o.cmo", now you must load "o_keywords.cmo pr_o.cmo". But the old way of invoking camlp5 for preprocessing (e.g. "camlp5 pa_o.cmo pr_o.cmo") is cumbersome and predates findlib. Instead you can use "not-ocamlfind preprocess -syntax camlp5o -package camlp5.pa_o,camlp5.pr_o") as is done here: https://github.com/drjdn/p5scm/pull/4/files

@chetmurthy
Copy link

Yes, "pr_op.cmo" provides extensions to the "pr_o.cmo" printer. The entire methodology of loading individual .cmo files into the camlp5 process is flawed, b/c you have to specify all the files you want, and in a compatible load-order. There is no way to get inference of which other files are needed, for the files you specify, etc. All of this sort of thing is the proper domain of findlib, which knows exactly these things and does it when constructing preprocessor commands -- so that is what not-ocamlfind preprocess hooks into.

@XVilka XVilka force-pushed the ocaml-5.2.0-version-bump branch from bd54380 to eea639c Compare May 25, 2024 13:40
@XVilka
Copy link
Contributor Author

XVilka commented May 25, 2024

Yes, "pr_op.cmo" provides extensions to the "pr_o.cmo" printer. The entire methodology of loading individual .cmo files into the camlp5 process is flawed, b/c you have to specify all the files you want, and in a compatible load-order. There is no way to get inference of which other files are needed, for the files you specify, etc. All of this sort of thing is the proper domain of findlib, which knows exactly these things and does it when constructing preprocessor commands -- so that is what not-ocamlfind preprocess hooks into.

Thanks for pointing this. This is my first time updating Homebrew package, so I don't know why this particular test was written this way, but from the comment purpose it was made to check the binary compatibility with the OCaml compiler. I will try to change to that not-ocamlfind instead.

@chetmurthy
Copy link

Ah, you're trying to check that the output of camlp5 is binary-compatible with the ocaml compiler ? In that case, you shouldn't be loading "pr_o.cmo" -- that produces text source. If you want to check binary compatibility, you could use not-ocamlfind preprocess -syntax camlp5o -package camlp5.pa_o,camlp5.pr_dump) . This would output the result of preprocessing as an OCaml binary AST.

@carlocab carlocab added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label May 25, 2024
Formula/c/camlp5.rb Outdated Show resolved Hide resolved
@chetmurthy
Copy link

If the issue here is tests, you could use the extensive test-suite that comes with Camlp5. When I took over maintenance, the test-suite was pretty limited, but I've added a massive set of tests of all sorts of function. You can look at the opam file to see how they're invoked. And if you have other tests in mind, I can add those too!

@XVilka XVilka force-pushed the ocaml-5.2.0-version-bump branch 4 times, most recently from 7e94cf2 to 1245820 Compare May 25, 2024 15:33
@XVilka
Copy link
Contributor Author

XVilka commented May 25, 2024

Ah, you're trying to check that the output of camlp5 is binary-compatible with the ocaml compiler ? In that case, you shouldn't be loading "pr_o.cmo" -- that produces text source. If you want to check binary compatibility, you could use not-ocamlfind preprocess -syntax camlp5o -package camlp5.pa_o,camlp5.pr_dump) . This would output the result of preprocessing as an OCaml binary AST.

For now I use the old solution, since the not-ocamlfind is not available at this stage.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Thanks!

@XVilka XVilka force-pushed the ocaml-5.2.0-version-bump branch 5 times, most recently from 99f7d98 to 8baf40a Compare May 26, 2024 01:22
@XVilka
Copy link
Contributor Author

XVilka commented May 26, 2024

Strange error while building Coq:

  ==> ./configure -prefix /home/linuxbrew/.linuxbrew/Cellar/coq/8.19.1_1 -mandir /home/linuxbrew/.linuxbrew/Cellar/coq/8.19.1_1/share/man -libdir /home/linuxbrew/.linuxbrew/lib/ocaml/coq -docdir /home/linuxbrew/.linuxbrew/Cellar/coq/8.19.1_1/share/coq/latex
  ocamlfind: Config file not found - neither /home/linuxbrew/.linuxbrew/etc/findlib.conf nor the directory /home/linuxbrew/.linuxbrew/etc/findlib.conf.d
  Error while running '/home/linuxbrew/.linuxbrew/opt/ocaml-findlib/bin/ocamlfind query findlib -format %v' (exit code 2)
  Configuration script failed!

@XVilka XVilka force-pushed the ocaml-5.2.0-version-bump branch from 8baf40a to 1e683eb Compare June 4, 2024 01:41
@XVilka
Copy link
Contributor Author

XVilka commented Jun 4, 2024

Strange error while building Coq:

  ==> ./configure -prefix /home/linuxbrew/.linuxbrew/Cellar/coq/8.19.1_1 -mandir /home/linuxbrew/.linuxbrew/Cellar/coq/8.19.1_1/share/man -libdir /home/linuxbrew/.linuxbrew/lib/ocaml/coq -docdir /home/linuxbrew/.linuxbrew/Cellar/coq/8.19.1_1/share/coq/latex
  ocamlfind: Config file not found - neither /home/linuxbrew/.linuxbrew/etc/findlib.conf nor the directory /home/linuxbrew/.linuxbrew/etc/findlib.conf.d
  Error while running '/home/linuxbrew/.linuxbrew/opt/ocaml-findlib/bin/ocamlfind query findlib -format %v' (exit code 2)
  Configuration script failed!

This looks similar to Homebrew/homebrew-test-bot#805 4f236f2, just different paths.

@XVilka XVilka force-pushed the ocaml-5.2.0-version-bump branch from 1e683eb to f10e7ec Compare June 4, 2024 02:08
@XVilka XVilka force-pushed the ocaml-5.2.0-version-bump branch from bd2a845 to 8802357 Compare June 4, 2024 13:41
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Test failures other than hyperkit look unrelated.

@carlocab carlocab added the ready to merge PR can be merged once CI is green label Jun 4, 2024
@chenrui333
Copy link
Member

@XVilka, thanks for your contribution to Homebrew! 🎉 🥇

Without awesome contributors like you, it would be impossible to maintain Homebrew to the high level of quality users have come to expect. Thank you!!!!

Copy link
Contributor

github-actions bot commented Jun 6, 2024

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label Jun 6, 2024
@BrewTestBot BrewTestBot enabled auto-merge June 6, 2024 03:57
@BrewTestBot BrewTestBot added this pull request to the merge queue Jun 6, 2024
Merged via the queue into Homebrew:master with commit ebed60d Jun 6, 2024
14 checks passed
Comment on lines -9 to -12
sha256 cellar: :any_skip_relocation, ventura: "3b67078315551718bc3c752b943b933713ddb69058f3cb72a0f65faa6e9295ab"
sha256 cellar: :any_skip_relocation, monterey: "da3b0d0374a85af5c649c86fb7796c1eecae468f5783bbb994a96d807e60712a"
sha256 cellar: :any_skip_relocation, big_sur: "f96e7270e9e853ce33f2195136b11338a5cf4d612ee50f3dd51b5c8506b4efcb"
sha256 cellar: :any_skip_relocation, catalina: "cd58afe172473278d3ed9404e9d25e10bee487fb4e27cd6de39c950a0ccaca87"
Copy link
Member

Choose a reason for hiding this comment

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

need to followup on this

Copy link
Member

@chenrui333 chenrui333 Jun 6, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hyperkit really looks like abandonware now:

Not much could probably be done for it, until developers/maintainers step in.

Copy link
Contributor Author

@XVilka XVilka Jun 6, 2024

Choose a reason for hiding this comment

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

Apologize for tagging you @djs55, but since you were hyperkit developer, you might be able to help reviving it.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, there are no new commits for the past two years.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not so much abandonware, but stable. There's no need to keep it up-to-date with the very latest OCaml, and I doubt there are many users accessing it via Homebrew. I'm happy to it to be removed from Homebrew to ensure there's no blocker on upstream OCaml updates. Copying in @djs55 as another maintainer of Hyperkit to confirm.

Copy link

@djs55 djs55 Jun 7, 2024

Choose a reason for hiding this comment

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

I would describe it as stable -- it's still shipped in Docker Desktop for Intel Macs. Removing hyperkit from Homebrew is fine.

Copy link
Contributor

github-actions bot commented Jun 6, 2024

@chenrui333 bottle request for hyperkit failed.

@XVilka XVilka deleted the ocaml-5.2.0-version-bump branch June 6, 2024 08:27
@chenrui333 chenrui333 mentioned this pull request Jun 6, 2024
6 tasks
@github-actions github-actions bot added the outdated PR was locked due to age label Jul 7, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-build-dependents-from-source Pass --build-dependents-from-source to brew test-bot. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. CI-no-fail-fast-deps Continue dependent tests despite failing GitHub Actions matrix tests. CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. long dependent tests Set a long timeout for dependent testing outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants