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

Test linking against camlp4 (fix #39399) #39468

Closed

Conversation

Blaisorblade
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with 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 brew install <formula>)?

Bump revision to rebuild against 4.07.1, and add test to detect that linking against camlp4 fails. I verified that brew test succeeds after rebuilding from source and fails with the current bottle.

Fix #39399.

@Blaisorblade
Copy link
Contributor Author

Blaisorblade commented May 2, 2019

For a standalone version of the testcase, see https://github.com/Blaisorblade/ulex/tree/test-camlp4.
/cc @SMillerDev @fxcoudert from #39399.

EDIT: beware I am still no OCaml expert — I can tell this testcase makes sense, but an expert might have better ideas. I'd be comfortable with this being committed — a review from an OCaml might help. I'll try to find one. EDIT2: I asked on #ocaml.

@Blaisorblade
Copy link
Contributor Author

Blaisorblade commented May 2, 2019

And sorry, I added revision after audit. I'll fix. EDIT: done, and hopefully I redid all the checks I should have. Sorry for the rush.

And bump revision to force bottles being rebuilt for ocaml 4.07.1.
Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

I have no idea about ocaml or camlp but I do have one homebrew remark.

Formula/camlp4.rb Outdated Show resolved Hide resolved
Co-Authored-By: Blaisorblade <p.giarrusso@gmail.com>
@Blaisorblade
Copy link
Contributor Author

Only remark on #ocaml was "if you wanted to test it I think it would make sense to attempt to use a macro, not just open the module".
I could try, but I'm not sure how (and them neither), and formula tests seem to be pretty minimal, so I'm not sure I have time soon... Plus, it did work for the actual issue we had, so maybe YAGNI.

@SMillerDev SMillerDev added the ready to merge PR can be merged once CI is green label May 3, 2019
@SMillerDev
Copy link
Member

Thanks @Blaisorblade! Without contributions like yours it'd be impossible to keep homebrew going! 👍 🎉

@fxcoudert fxcoudert closed this in 24b5c0f May 3, 2019
@avsm
Copy link
Contributor

avsm commented May 3, 2019

Thanks, I appreciate you submitting this fix, as it's been causing problems for OCaml users (camlp4/camlp4#150).

As a general rule, whenever the OCaml compiler package is upgraded, all dependent OCaml libraries should be recompiled and their Homebrew package revision bumped. The OCaml compiler doesnt guarantee ABI compat across releases, and has CRC checks to ensure consistent linking.

So the test case in this PR will be most useful in spotting errors, but if there's some process the Homebrew team could use to ensure that the tests are run whenever the OCaml package is updated, that would be most appreciated.

@Blaisorblade
Copy link
Contributor Author

So the test case in this PR will be most useful in spotting errors, but if there's some process the Homebrew team could use to ensure that the tests are run whenever the OCaml package is updated, that would be most appreciated.

Thanks! IIUC the maintainers implied that this test is run when OCaml is upgraded, so next time the problem should be detected. @fxcoudert could you please confirm?

But this test is just for camlp4 and will only rebuild camlp4. Do any other packages need this setup? E.g. homebrew packages camlp5, but opam builds won't use the system camlp5, so maybe they avoid this problem? Executables are statically linked, hence fine.

@avsm
Copy link
Contributor

avsm commented May 3, 2019

It would be good to have the test for camlp5 -- it's still safer to recompile it with the host OCaml even if it's an executable. It might, for example, use Marshal to transmit data across a pipe which would fail.

Camlp4 is the only system package in opam due to its historically special status -- it was not possible to have a system OCaml compiler and a local camlp4 in the past due to some layout issues in the filesystem.

@fxcoudert
Copy link
Member

Whenever a formula is updated, our CI testing will run the tests for all the formulas that depend on it (including recursive dependences). So when a PR for ocaml is submitted, the following tests are run:

$ brew uses --recursive ocaml
bibtex2html     camlp5          coq             hevea           ledit           menhir          ocaml-num       ocamlsdl
camlp4          coccinelle      fstar           lablgtk         math-comp       ocaml-findlib   ocamlbuild

with the rebuilt ocaml. We consider it the responsibility of those formulas' test blocks to accurately exercise it enough that we know nothing is broken. Now the camlp4 test has been improved, and it appears the camlp5 test is checking its sync with ocaml.

Other formulas' tests need to be audited by someone with knowledge of how ocaml works.

@Blaisorblade Blaisorblade deleted the stricter-camlp4-tests branch May 6, 2019 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

camlp4.4.07+1 might need to be rebuilt for ocaml 4.07.1
4 participants