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

Remove boot/ocamldep #1537

Merged
merged 3 commits into from Mar 29, 2018

Conversation

Projects
None yet
4 participants
@nojb
Copy link
Contributor

nojb commented Dec 18, 2017

Following up to #1078, this PR removes boot/ocamldep, saving 2M in each bootstrap.

In fact boot/ocamldep is not used at all during building/bootstrapping/etc so apart from a couple trivial changes (see diff) no adaptations are necessary.

Does this need a Changes entry?

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 19, 2017

I vaguely remember that ocamldep was added to boot/ so that dependencies could be rebuilt even if they are so broken that tools/ocamldep cannot be built. @damiendoligez what do you think? Is this OK or should we also replace uses of tools/ocamldep by boot/ocamlc -depend in various Makefiles?

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Dec 20, 2017

The goal of #1078 was precisely to avoid the need to commit an additional bytecode executable in boot/. So yes, I think we should use boot/ocamlc -depend in Makefiles, and then get rid of boot/ocamldep for good.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Dec 20, 2017

(Well, Makefiles currently refer to tools/ocamldep, not boot/ocamldep.)

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Dec 20, 2017

Indeed, the Makefiles do not refer to boot/ocamldep at all, so the question is: should the references to tools/ocamldep be changed to boot/ocamlc -depend ?

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 20, 2017

should the references to tools/ocamldep be changed to boot/ocamlc -depend ?

If you do make sure it is boot/ocamlrun boot/ocamlc -depend. There is some confusion between boot/ocamlrun and byterun/ocamlrun in the Makefiles presently, which will need to be fixed, so let's not add any.

The question has no easy answer.

  • In favor of boot/ocamlc -depend, it makes it possible to rebuild the Caml dependencies almost from a bare checkout: all you need is the initial build of ocamlrun. So that's very robust against bad days.
  • Against the change: if ocamldep is improved or gets a bug fix, one full bootstrap cycle will be necessary until the changes propagate to boot/ocamlc and can be taken advantage of to rebuild the dependencies of the core system.

@damiendoligez any opinion?

@damiendoligez

This comment has been minimized.

Copy link
Member

damiendoligez commented Feb 21, 2018

I'm completely in favor of removing boot/ocamldep, thus completing the work started by @lefessan, it will save a lot of bandwidth and disk space when cloning the repo.

@nojb: Please make sure that you can rebuild the dependencies from a fresh checkout without compiling any OCaml code.

@nojb nojb force-pushed the nojb:remove_boot_ocamldep branch from bc74bc7 to 47b64b5 Feb 21, 2018

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Feb 21, 2018

If one comments out

beforedepend:: asmcomp/emit.ml

and

beforedepend:: bytecomp/opcodes.ml

from the main Makefile one can do make depend successfully from a fresh checkout (after compiling ocamlyacc and ocamlrun).

If we want to be able to do make alldepend from a fresh checkout we need to replace tools/ocamldep by boot/ocamlc -depend in the following places:

  • stdlib/Makefile
  • lex/Makefile
  • ocamltest/Makefile
  • tools/Makefile
  • ocamldoc/Makefile
  • otherlibs/...

@nojb nojb force-pushed the nojb:remove_boot_ocamldep branch from 17c5d2f to 52f37ce Feb 28, 2018

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Feb 28, 2018

@damiendoligez I checked that one could do make depend from a fresh checkout. One needs to comment out the lines in Makefile corresponding to files generated by OCaml tools but this is the same as before this PR.

We could replace tools/ocamldep by boot/ocamldep -depend everywhere else in the distribution which would allow us to do a make alldepend directly from a fresh checkout, but with the downside noted by Xavier.

Do you have an opinion on this?

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Mar 16, 2018

Changes Outdated
@@ -165,6 +165,10 @@ Working version
- GPR#1585: optimize output of "ocamllex -ml"
(Alain Frisch, review by Frédéric Bour and Gabriel Scherer)

- GPR#1537: boot/ocamldep is no longer included in the distribution;

This comment has been minimized.

@damiendoligez

damiendoligez Mar 28, 2018

Member

Could you add "in the source distribution" to make it clear that binary distributions are not affected by this change?

This comment has been minimized.

@nojb

nojb Mar 28, 2018

Author Contributor

Fixed, thanks.

@damiendoligez

This comment has been minimized.

Copy link
Member

damiendoligez commented Mar 28, 2018

Looks good except for the wording of the Changes entry. @nojb could you change it and rebase? Thanks.

@nojb nojb force-pushed the nojb:remove_boot_ocamldep branch from 52f37ce to a83d27d Mar 28, 2018

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Mar 29, 2018

Will merge this by the end of the day unless someone speaks up.

@nojb nojb merged commit ab16747 into ocaml:trunk Mar 29, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nojb nojb deleted the nojb:remove_boot_ocamldep branch Mar 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.