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

Add configure option to not install .cmt, .cmti, .ml and .mli files #1777

Merged
merged 1 commit into from May 14, 2018

Conversation

Projects
None yet
3 participants
@mshinwell
Copy link
Contributor

commented May 9, 2018

This patch adds a configure option to disable installation of .cmt, .cmti, .ml and .mli files. The rationale is that in an environment where multiple different compiler variants are installed (for example, non-flambda, flambda, flambda+spacetime, etc.) it suffices to use the .cmt and related files from just one of these installations (typically the default one with no special features). These files will not be exactly right---for example some constants in Config may differ---but for practical purposes this is a sufficiently close approximation. A substantial disk space saving may be made as a result.

@nojb

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

A couple of observations/questions:

  • I don't like the name NO_INSTALL_CMT since it is not very accurate; also I think "positive" variable names are clearer: INSTALL_CMT, etc.
  • Is it necessary to have two different variables for this and #1776 ? Why not have a single variable, say LEAN_INSTALL, that, when true, does both? Even better, why not make #1776 the default as @diml suggests?
  • In order to have a cleaner patch, maybe introduce variables INSTALL_DATA_X, INSTALL_PROG_X (naming to be decided) which ignore their arguments if doing a "lean" install?
@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2018

I think this only tangentially related to #1776, to be honest. If we would like to provide a "lean install" I think a better approach would be to add these fine-grained options and then a single other one which controls a convenient subset.

As regards the name of the option, I left it at -no-install-cmt because including all the file extensions in there would be very lengthy, and I think what follows from this option's effects seems fairly clear: if you're not installing the .cmt files (for Merlin in particular) then you don't need the .cmti, .mli or .ml files either. Do you have a better suggestion for the name?

I can change the variable name to be positive if really required, but I have to say that I genuinely cannot see the current naming is in practice likely to be confusing, and that the time taken to make such changes and re-test would probably be better spent elsewhere.

Let's not over-engineer these patches: we can always improve on an initial straightforward version at a later time. This is especially pertinent given that the whole make system is, if I understand correctly, up for significant upgrade and renewal in the relatively near future.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2018

One other point (in conjunction with @diml) to just note here, for potential implementation on another pull request: we should maybe consider adding an option to compress the .cmt files with one of the modern fast compression methods. The size of these files is in general excessive.

@shindere

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

@mshinwell mshinwell force-pushed the mshinwell:no_cmt branch from 8f4ae47 to c2e7d4b May 10, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2018

I've updated the name and the sense of the variable. OK?

@nojb

This comment has been minimized.

Copy link
Contributor

commented May 11, 2018

LGTM

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2018

@nojb Are you able to approve this, then?

@nojb

nojb approved these changes May 11, 2018

@nojb

This comment has been minimized.

Copy link
Contributor

commented May 11, 2018

@nojb Are you able to approve this, then?

Yes, done.

@mshinwell mshinwell merged commit ea2d6a1 into ocaml:trunk May 14, 2018

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
You can’t perform that action at this time.