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

camlp5: improve camlp5 formula test #35916

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@DanGrayson
Copy link
Contributor

DanGrayson commented Jan 11, 2019

... by ensuring that the ocaml compiler provided by the package ocaml is up to
date enough. We do that by adding a random compiled file from the ocaml
package ( bigarray.cma ) to the command line of the current test,
so camlp5 will try to link it in with the others. If that compiled file is too
old, then the magic number stored in it will be smaller than the magic number
in the compiled files used by camlp5.

There is a discussion about this issue here:

https://discourse.brew.sh/t/prebuilt-bottle-dependencies/3853

In addition to the tests below, I have verified that brew test camlp5 will now fail if the current
pre-built camlp5 bottle is installed.

  • [ yes] Have you followed the guidelines for contributing?
  • [yes ] Have you checked that there aren't other open pull requests for the same formula update/change?
  • [yes ] Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • [ yes] Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • [ yes] Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?
camlp5: improve camlp5 formula test
... by ensuring that the ocaml compiler provided by the package ocaml is up to
date enough.  We do that by adding a random compiled file from the ocaml
package ( bigarray.cma ) to the command line of the current test, so camlp5
will try to link it in with the others.  If that compiled file is too old, then
the magic number stored in it will be smaller than the magic number in the
compiled files used by camlp5.

There is a discussion about this issue here:

  https://discourse.brew.sh/t/prebuilt-bottle-dependencies/3853
@DanGrayson

This comment has been minimized.

Copy link
Contributor

DanGrayson commented Jan 11, 2019

Maybe I should add a comment to the effect that an error message like

Error while loading "/usr/local/opt/ocaml/lib/ocaml/bigarray.cma": interface mismatch on Stdlib__complex.

is what one expects when the ocaml package is too new, and that a dissimilar error could be
due to the test becoming broken.

@DanGrayson

This comment has been minimized.

Copy link
Contributor

DanGrayson commented Jan 12, 2019

If the test for coq had included building it from sources, it would have failed, because of the old camlp5 package:

brew$ brew install --build-from-source coq
brew install --build-from-source coq
==> Downloading https://github.com/coq/coq/archive/V8.8.2.tar.gz
Already downloaded: /Users/brew/Library/Caches/Homebrew/downloads/a4948cdfc5134f2819ef208fe8ec948d4f0f5d87b20bead0348eebab8895de65--coq-8.8.2.tar.gz
==> ./configure -prefix /usr/local/Cellar/coq/8.8.2 -mandir /usr/local/Cellar/coq/8.8.2/share/man -emacslib /us
==> make world
Last 15 lines from /Users/brew/Library/Logs/Homebrew/coq/02.make:
OCAMLC -c grammar/q_util.mli
244 states, 858 transitions, table size 4896 bytes
314 states, 4454 transitions, table size 19700 bytes
2927 additional bytes used for bindings
189 states, 503 transitions, table size 3146 bytes
CAMLP5O   ide/coqide_main.ml4
OCAMLDEP  checker/MLFILES checker/MLIFILES
File "grammar/q_util.mli", line 1:
Error: /usr/local/Cellar/camlp5/7.07/lib/ocaml/camlp5/mLast.cmi
is not a compiled interface for this version of OCaml.
It seems to be for an older version of OCaml.
make[1]: *** [grammar/q_util.cmi] Error 2
make[1]: *** Waiting for unfinished jobs....
2654 states, 8247 transitions, table size 48912 bytes
make: *** [submake] Error 2

READ THIS: https://docs.brew.sh/Troubleshooting
@@ -21,9 +21,11 @@ def install
(lib/"ocaml/camlp5").install "etc/META"
end

ocaml = Formula["ocaml"]

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jan 12, 2019

Member

Put this inside the test do instead, thanks 👍

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 12, 2019

Great work here @DanGrayson, thanks!

In addition to the tests below, I have verified that brew test camlp5 will now fail if the current
pre-built camlp5 bottle is installed.

Could you add a revision 1 to the formula which will ensure that when we pull this we also pull an updated camlp5 bottle?

Maybe I should add a comment to the effect that an error message like

Yeh a (Ruby) inline comment would be great 👍

Thanks again!

@DanGrayson

This comment has been minimized.

Copy link
Contributor

DanGrayson commented Jan 12, 2019

Okay, done.

I assume something will automatically update the hashcodes of the bottles.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jan 12, 2019

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @DanGrayson!

@DanGrayson

This comment has been minimized.

Copy link
Contributor

DanGrayson commented Jan 12, 2019

You're welcome! Homebrew is well done, so it's worth getting it perfect.

DanGrayson added a commit to DanGrayson/UniMath that referenced this pull request Jan 14, 2019

update INSTALL
Using Homebrew under Mac OS X, we no longer need to build camlp5 specially, because
my pull request was accepted, see Homebrew/homebrew-core#35916 ;
and we no longer need an extra tap for getting ocaml-findlib.

Under Arch Linux we now need to install ocaml-num.

@DanGrayson DanGrayson referenced this pull request Jan 14, 2019

Merged

update INSTALL #1150

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