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

Use boot/ocamlc.opt for building, if available. #8514

Merged
merged 2 commits into from Mar 22, 2019

Conversation

@stedolan
Copy link
Contributor

commented Mar 18, 2019

With this patch, if the file boot/ocamlc.opt exists, it is used for building the compiler instead of boot/ocamlrun boot/ocamlc. This means that when working on and repeatedly building the compiler, one can cp ocamlc.opt boot/ to accelerate builds.

On my machine, this roughly halves the time needed to make clean; make world (from 73 to 36 seconds, on a build configured with --disable-instrumented-runtime --disable-debugger --disable-ocamldoc --disable-graph-lib).

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Gee, I worked on exactly the same thing, and for some reason I never got around to a nice-enough state to send a PR. I like the idea.

I'm not sure if we want to have boot/ocamlc.opt in gitignore, though. If we have no Makefile targets to handle this file (refresh it, delete it, etc.), and it's not versioned, then maybe I do want to be aware that it is around and it might be stale.

Makefile Outdated Show resolved Hide resolved
@stedolan stedolan force-pushed the stedolan:boot-ocamlc-opt branch 2 times, most recently from d30662d to 537dc1c Mar 18, 2019
@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

I'm not sure if we want to have boot/ocamlc.opt in gitignore, though. If we have no Makefile targets to handle this file (refresh it, delete it, etc.), and it's not versioned, then maybe I do want to be aware that it is around and it might be stale.

I certainly do want it gitignore, as that's the best way to ensure it doesn't get committed. A Makefile target to update/remove/etc. could useful (although I'm not sure that this helps making people aware that it's around). I'm on the fence about whether make clean should get rid of it.

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Makes sense -- those irresponsible commit -a users!

So how do we prevent people from shooting themselves in the foot with a stale boot/ocamlc.opt?

Maybe we could have warning when boot/ocamlc.opt is older than boot/ocamlc? In practice this means having to touch it at each checkout, but maybe that is a reasonable habit to take. (I also thought of checking their -version output but this is too coarse-grained to be useful.)

@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

I think checking the file dates is a good idea! I'd go further than a warning, though: we should only use boot/ocamlc.opt if it is newer than boot/ocamlc.

In practice, we won't have to touch at each checkout: when switching branches, git only touches the files that have actually changed. We'll only have to touch boot/ocamlc.opt when we switch to a branch that's using a different bootstrap compiler, in which case we probably shouldn't use the old boot/ocamlc.opt anyway.

This means that once you cp ocamlc.opt boot/, you'll get fast builds until the next update of the bootstrap compiler, after which point you'll have to copy ocamlc.opt again.

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

I think checking the file dates is a good idea! I'd go further than a warning, though: we should only use boot/ocamlc.opt if it is newer than boot/ocamlc.

Okay, but then one issue is silently forgetting to update ocamlc.opt -- I'll see in the build logs, though, that ocamlrun boot/ocamlc is being used.

In practice, we won't have to touch at each checkout: when switching branches, git only touches the files that have actually changed.

Nice! I had never checked.

Note: if we had a Makefile target, one nice thing to do would be to write the git commit at the time where ocamlc.opt was generated.

boot/ocamlc.opt: ocamlc.opt
    cp ocamlc.opt boot/
    git show --oneline | head -n 1 > boot/ocamlc.opt.stamp

(I'm sure people know better git commands that, for example, store the branch name as well.)

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

This seems nice to me, too! I would have clean leave boot/ocamlc.opt alone but distclean should either delete it or display a message that it hasn't cf. https://github.com/ocaml/opam/blob/master/src_ext/Makefile#L227

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

Another thought for promoting its existence - the opt.opt could always copy ocamlc.opt to boot/ocamlc.opt if .git exists

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

Frankly, it could possibly always copy it - for a release build, it would never be used, as the copy would take place right at the end of world.opt

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

I would vote against always promoting ocamlc.opt for now. It's a nice idea but it makes everyone, not just expert users, rely on the logic, and suffer from the extra fragility. Let's keep it a you-know-when-you-use-it thing for now, and think about making it the default behavior after we've gathered some experience using it in practice.

I realize now that what resulted in my own experimentation never getting released is probably that I over-thought all these aspects that we just started nitpicking on. @stedolan, there is clearly enough support for your idea to merge (especially if it remains opt-in rather than opt-out). I'll approve now; please make whatever changes you feel like making inspired by the discussion, and then merge when you are satisfied.

@gasche
gasche approved these changes Mar 18, 2019
@stedolan stedolan force-pushed the stedolan:boot-ocamlc-opt branch 2 times, most recently from 953e668 to a00770f Mar 18, 2019
@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@gasche:
I've changed it to check that ocamlc.opt is newer than ocamlc.

Other Makefile targets, more features, etc. welcome, but I'll leave them out of this PR.

@dra27:
Thanks for pointing out distclean. Now, boot/ocamlc.opt is deleted by distclean but not by clean.

I agree with @gasche that we shouldn't promote on every build - most of my compiler builds are broken experiments, and I certainly don't want to use them to build the next compiler!


I'm going to wait until Appveyor OK's this (to check that test -nt is OK in the windows build), then I'll merge.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Oops, in my mind I conflated the promotion of ocamlrun during coldstart with bootstrap promotion 😊

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

(@dra27 just fixed a check-typo issue. I happen to be around a bunch of OCaml developers at a big meeting today; could you give me three hours before merging, so that I make sure everyone is appropriately excited by this small-but-noticeable change?)

@stedolan stedolan force-pushed the stedolan:boot-ocamlc-opt branch from 76110bc to 32563ae Mar 20, 2019
@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@dra27 thanks! I'd actually fixed this but forgot to push. Something went wrong with your patch (make complaining about recursive variables?), so I just pushed my version over it. Hopefully check-typo's satisfied now.

@gasche sure, no problem.

@stedolan stedolan force-pushed the stedolan:boot-ocamlc-opt branch from 32563ae to 46c427f Mar 20, 2019
@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

@stedolan - oops, indeed - I defined BOOT_OCAMLC twice where the first should have been BOOT_OCAML_BYTE

It's a bitlot nitty, but I kinda preferred my "one-liner" version (albeit with a non-conflicting variable name) to spreading the command over multiple lines?

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Good news: Damien was fine with the change.

Good bad news: Armaël is asking for a HACKING.adoc explanation of the feature -- and of course he is right.

@stedolan stedolan force-pushed the stedolan:boot-ocamlc-opt branch from 5464b22 to fe2ef5f Mar 20, 2019
HACKING.adoc Outdated Show resolved Hide resolved
@stedolan stedolan force-pushed the stedolan:boot-ocamlc-opt branch from fe2ef5f to 9acd54e Mar 22, 2019
@gasche
gasche approved these changes Mar 22, 2019
Copy link
Member

left a comment

I like the new documentation, thanks! Good to merge when CI passes.

@stedolan stedolan merged commit f495bfb into ocaml:trunk Mar 22, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.