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 ".byte" executables #1776

Merged
merged 3 commits into from May 14, 2018

Conversation

Projects
None yet
5 participants
@mshinwell
Copy link
Contributor

commented May 9, 2018

In environments where the executables compiled to native code, such as ocamlopt.opt, are always used in preference to the bytecode versions then space can be saved by not installing the latter. This patch provides a configure option to do such. It is relatively lightly engineered; in particular, it won't complain if the native code executables aren't themselves being built; but given this is an option for knowledgeable users I think that is reasonable.

@shindere

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

@diml

This comment has been minimized.

Copy link
Member

commented May 9, 2018

Why is it not the default? Is there any point in installing the bytecode versions?

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2018

@shindere I think point 1 is arguable either way. I take the "feature", that is to say something to be optionally enabled, as the disabling of the installation of bytecode executables. The common case is to not have the feature.

For point 2, the naming of this option follows the other options that currently exist in the configure script, no?

@xclerc

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

Not very important, but it could be extended to ocamldoc
(which, by the way, does not install through INSTALL_PROG).

@shindere

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2018

Well, I would have thought having one convention which is followed now, then a different convention later (at which point everything changes) was better than two conventions now then one later. Maybe that's just me though.

As regards @diml 's point: maybe this should be the default, but if it is the default, there should probably be an option to turn it off (in particular for use when developing the native code compiler, which sometimes causes miscompilations of the .opt versions). Also, a discussion about whether this should be the default can be separate from this pull request.

@gasche

This comment has been minimized.

Copy link
Member

commented May 9, 2018

New configure options should always come in both a negative or a positive version, except in the rare case where we feel exceedingly sure that the default value will never change in the future -- which is clearly not the case here. So {enable,disable}-bytecode-executables as suggested by @shindere, or {with,no}-bytecode-executables if you prefer the current partial convention ({with,no}-{debug,instrumented}-runtime, {with,no}-cplugins, etc.). (Personally I would follow the current convention.)

Besides the command-line settings, I agree with @shindere that a positive name for the internal option (WITH_BYTECODE_EXECS) would make more sense and be easier to maintain.

@shindere

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

@mshinwell mshinwell force-pushed the mshinwell:no_installed_bytecode_exes branch from b7953e7 to 691b286 May 10, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2018

I have pushed a revised version.

@gasche

This comment has been minimized.

Copy link
Member

commented May 10, 2018

The configure options are fine (-do-install and -do-not-install would sound better, but I'm content with -install and -no-install) and the naming is better. Maybe it would sense to also comment out the actions in the style of $(LN) ocamlc.byte$(EXE) ocamlc$(EXE) which create dead links with the current patch?

(You pointed out that you don't try to file if -no-install-... is used while native compilation is not available, but maybe we could make sure we don't create dead links in the install directory.)

I used git grep "LN.*\\.byte.*" to catch the occurrences of this pattern.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2018

Not creating the links is probably fine. @shindere do you have any other comments?

mshinwell added some commits May 14, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

@gasche It doesn't create bad links now. Please approve if this is now satisfactory.

@gasche

gasche approved these changes May 14, 2018

Copy link
Member

left a comment

Approved -- if the CI passes -- but you should rebase/squash before or when merging.

@gasche gasche merged commit 8054e4f 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.