Skip to content

Conversation

@gasche
Copy link
Member

@gasche gasche commented Jan 25, 2016

As was decided during the 4.03 development cycle, this PR removes ocamlbuild from the compiler.

This move was announced on caml-list in [Caml-list] Announce: OCamlbuild to be distributed separately from the compiler. Since then, the project has moved to its own repository ( https://github.com/ocaml/ocamlbuild/ ), and had a first experimental release on OPAM today.

The compiler descriptions in OPAM should currently have a dummy base-ocamlbuild base package -- it was added in opam-repository#5363 for public switches. Any version not shipping with ocamlbuild packaged (in particular any OPAM switch for 4.03+dev created after the present PR is merged) should remove this base-ocamlbuild package from its description. The separate ocamlbuild package (version 0.9.0 merged in opam-repository#5483) conflicts with base-ocamlbuild, so it won't be installed if it is present.

The packages that live in OPAM today and depend on ocamlbuild should all have an ocamlbuild dependency -- otherwise they may fail to build with the upstream compiler once this PR is merged. There is a dummy ocamlbuild.0 package that is currently installable by anyone. Once the transition in trunk is other, I shall restrict it to (< 4.03). Then there is ocamlbuild.0.9.0, that is available on (>= 4.03), and that should work for everyone -- if not, please file a bug report at https://github.com/ocaml/ocamlbuild/issues/ .

@damiendoligez damiendoligez added this to the 4.03.0 milestone Jan 26, 2016
@gasche
Copy link
Member Author

gasche commented Jan 27, 2016

@diml the way Camlp4 is installed by the Travis script fails once ocamlbuild is removed from the distribution. I'm not sure what is the best way to fix that, would just asking the travis script to just fetch and locally install ocamlbuild.0.9.0 before proceeding with camlp4 be reasonable?

@ghost
Copy link

ghost commented Jan 27, 2016

Yes, I think that's the only solution until we stop building Camlp4 in the Travis script

@damiendoligez damiendoligez mentioned this pull request Jan 27, 2016
@mshinwell
Copy link
Contributor

Or we stop building camlp4 in Travis, which also sounds somewhat attractive. It would make feedback quicker.
I will attempt to fix the script once we make a decision.

@gasche
Copy link
Member Author

gasche commented Jan 29, 2016

@mshinwell I would leave the decision to have camlp4 in Travis or not to @diml (I think it's a matter of whether this helps them maintain Camlp4, and I think the impact is rather positive right now). I would rather change the Travis script to install ocamlbuild for now, and leave any Camlp4-level decision to a later time.

I had "fix Travis to install ocamlbuild" on my TODO-list, but I'm very busy right now so I would be very happy if you had time to indeed attempt to fix this.

@mshinwell
Copy link
Contributor

@diml would like to keep camlp4 in Travis, so I am going to adjust it to build and install ocamlbuild. There will be a separate pull request.

@mshinwell
Copy link
Contributor

@gasche Please merge gasche#2

@gasche
Copy link
Member Author

gasche commented Feb 1, 2016

@mshinwell Merged. I'm happy to see that my making-it-easy-to-configure-and-install efforts seemed to have made the process easy enough.

I'm a bit ambivalent about the use of ocamlbuild's master branch to build, as this means that the OCaml CI will break when whenever ocamlbuild trunk becomes incompatible. If I had done this change myself I would have probably targeted the fixed ocamlbuild 0.9.0 release, for which breakage should not happen (unless OCaml itself breaks backward-compatibility in bad ways). That said, this change is also good for the same reasons that having Camlp4 in the OCaml CI helps Camlp4 maintenance -- and I expect ocamlbuild to break much less often as it is less tied to the inner workings of the compiler. So I'd leave it as you did it for now.

@mshinwell
Copy link
Contributor

It wasn't actually that straightforward to figure out how to get ocamlbuild to build correctly...

@mshinwell
Copy link
Contributor

(Can you resolve the conflicts and then hopefully this will get a tick?)

@gasche gasche force-pushed the remove-ocamlbuild branch from d7f50fe to 45b6323 Compare February 1, 2016 14:56
@gasche
Copy link
Member Author

gasche commented Feb 1, 2016

I resolved the conflicts. On my machine ast-invariant fails on trunk, and the same fail in my branch:

Summary:
  560 tests passed
    5 tests skipped
    1 tests failed
    0 unexpected errors
  566 tests considered

List of failed tests:
    tests/ast-invariants

List of skipped tests:
    tests/lib-dynlink-csharp
    tests/lib-bigarray-2

@mshinwell
Copy link
Contributor

@damiendoligez Is this ok for merging?

@ghost
Copy link

ghost commented Feb 1, 2016

@gasche, if you send me tests/ast-invariants/test.result I can try to fix the test

@gasche
Copy link
Member Author

gasche commented Feb 1, 2016

The program does not run because

 ... testing with ocamloptFatal error: exception Sys_error("../../../.git/remotes: No such file or directory")
 => failed

As you may have guessed, this .git/remotes happens to be a symbolic link to a directory that has since been removed. (This is due to some fragility in the way git-new-workdir works.)

@ghost
Copy link

ghost commented Feb 1, 2016

Ok, I tried to fix the test in c32f981

@damiendoligez
Copy link
Member

OK for merge when @gasche confirms that the test is fixed.

@gasche
Copy link
Member Author

gasche commented Feb 1, 2016

Yes, the test is fixed on my machine. (It's orthogonal to remove-ocamlbuild: it failed on trunk and is now fixed on trunk as well.)

Note that I still get some testsuite output that seems wronger than I would expect (but the general output is "all passed", with 561 tests passed plus 5 tests skiped), namely:

 ... testing 'is_static':File "is_static.ml", line 26, characters 9-32:
Warning 55: Inlining impossible in this context: Function information unavailable
File "is_static.ml", line 122, characters 16-64:
Warning 55: Inlining impossible in this context: Function information unavailable
 => passed

@mshinwell, is this warning message suppose to be output on stderr by the testsuite?

@gasche gasche force-pushed the remove-ocamlbuild branch from 45b6323 to 751352c Compare February 1, 2016 20:09
@mshinwell
Copy link
Contributor

I'll ask @chambart to look at this test and any others producing this warning. I'm going to merge this PR now since these are entirely orthogonal issues.

mshinwell added a commit that referenced this pull request Feb 2, 2016
@mshinwell mshinwell merged commit ac4ce3e into ocaml:trunk Feb 2, 2016
@chambart
Copy link
Contributor

chambart commented Feb 2, 2016

Well that one is quite tricky: This is testing things both with and without flambda, but less things are expected without. With flambda it some functions need to be inlined for the test to pass, but without those properties relying on inlining are not expected. the test knows in which mode it is and assertions depends on it, but [@inline] attributes can't cannot depend on the mode. The only way I see to avoid that is not to run this test without flambda, and duplicate the common part in a separate test.

@gasche
Copy link
Member Author

gasche commented Feb 2, 2016

@chambart I think tests should never print weird stuff during their normal working schedule, so that any weird stuff happening can be reported and fixed¹. So whatever you have to do there would be worthwhile in my opinion.

¹: The people handling nuclear plants at EDF have a programme called MEEI, "Maintenir un État Exemplaire des Installations", that more or less consists in cleaning stuff and repainting it in nice colors, to be sure to notice when something weird happens (see for example this press release).

@damiendoligez
Copy link
Member

I propose #456 to solve this problem.

@damiendoligez
Copy link
Member

@gasche Unless I'm mistaken, you haven't removed ocamlbuild from the manual.

@gasche
Copy link
Member Author

gasche commented Feb 12, 2016

@damiendoligez oh indeed, I'll do that now.

lukemaurer added a commit to lukemaurer/ocaml that referenced this pull request May 17, 2021
chambart pushed a commit to chambart/ocaml-1 that referenced this pull request Jan 4, 2022
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
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.

4 participants