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

Allow otherlibs not to be built #2053

Merged
merged 9 commits into from Jan 9, 2019

Conversation

Projects
None yet
6 participants
@dra27
Copy link
Contributor

commented Sep 19, 2018

This is a precursor to allowing compilation of certain parts of the source tree without building the compiler.

The main addition is --no-unix, --no-vmthreads and --no-str options. Rather pleasingly, this is largely a configure script change - the other changes are a minor tweak in the build system to skip building caml-tex and changes to allow the testsuite to adapt to these libraries not being available.

One point which may want addressing further: we always build caml-tex, but we don't install it, which seems daft. Perhaps an option should be added so that it's tested on CI but not built by default?

@dra27 dra27 force-pushed the dra27:optional-libs branch from 5250076 to e6c5c6a Sep 19, 2018

@shindere

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2018

I would like it in 4.08, but I don't mind in which order!

The testsuite and ocamltest changes can of course be merged in advance of the change to configure itself, if you'd be happy to review those separately?

@shindere

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2018

@shindere - thanks! I don't mind what the names of the parameters are. The current configure doesn't use GNU conventions, so I guess that the autoconf version is already going to be incompatible. Does that mean that you were already expecting existing ones like --no-graph to become --without-graph with no compatibility option? That seems fine to me.

Just for the full list, these are the options added:

  • --with-graph, --with-unix, --with-str, --with-vmthreads, --with-systhreads (all autoconf-compatible)
  • --no-unix, --no-str, --no-vmthreads, --no-systhreads (--no-graph already exists) would be --without-* in autoconf
  • --debugger would be --with-debugger (and the existing --no-debugger would become --without-debugger)
  • --ocamldoc would be --with-ocamldoc (and the existing --no-ocamldoc would be come --without-ocamldoc)

If the --no-* options are going to be removed and become --without-* I don't see any reason not to name all these ones here --without-* and cope with the inconsistency for the small amount of time before the existing ones get renamed too

@shindere

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

Concerning caml-tex, I agree that its status is a bit awkward. But since it depends on compiler-libs, it seemed better to raise an error as early as possible. Moving its build to a target build just before tests could be an option (after adding a bit of plumbing to the manual's Makefile). If I am not mistaken, it could be moved together with cmpbyt which is also only used by the test suite.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

This sounds like a quite unnatural location for a manual's tool compared to either tools or manual/tools.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2018

Indeed, it doesn’t seem to want moving, just that we don’t necessarily want it built unless it’s to be used (or for testing, of course)

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2018

@shindere - going back to the naming, it of course doesn't matter if autoconf gets in to 4.08 and this is merged with that, but perhaps it would be sensible for the naming in this GPR, being merged now, matches what the names would be in autoconf. So I'll change the new options added, as above, and add autoconf-compatible aliases for the ones which already exist (so --without-ocamldoc, --without-debugger and --without-graph added too).

@dra27 dra27 force-pushed the dra27:optional-libs branch 4 times, most recently from b7820fa to f511ca7 Sep 22, 2018

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2018

Added as fixup commits for clarity of reviewing (I'll rebase before it's merged).

So I changed --no to --without. I like your idea with the -lib suffix too - so it's now the clearer --with-graphics-lib, etc. I left it as --with-systhreads and --with-vmthreads since these both define a library called threads and really that is morally more about enabling support for something than specifically a library.

One further question, which had been in the back of my mind, but I hadn't checked. We use things like --with-debug-runtime at the moment, but in autoconf world these would more normally be --enable-debug-runtime with --with-* used for external dependencies (for example, --with-libunwind or, as used to be, --with-curses).

What have you done for these so far? I should rename them to --enable-*/--disable-* if the plan would be to use AC_ARG_ENABLE rather than AC_ARG_WITH.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

@shindere : since you approved this PR, is there anything blocking a merge (except for the conflicts)?

@shindere

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

Ok, I thought you decided it was worth merging this one before yours.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

@dra27 dra27 force-pushed the dra27:optional-libs branch from f511ca7 to 420ebe9 Oct 31, 2018

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

Rebased and updated. Sorry - this had slipped what with ICFP and all.

@shindere - I would like this to go into 4.08 if at all possible, though I'm very happy to have to rebase it onto the autoconf version

@shindere

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

@damiendoligez damiendoligez added this to the 4.08 milestone Nov 16, 2018

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

@dra27 Do you have time to rebase this one this week?

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

Fix --disable-pthread
$systhread_support must be a valid OCaml boolean, as it gets written to
tools/config.ml

dra27 added some commits Jan 8, 2019

Rename --enable-pthread to --enable-systhreads
Name better reflects the cross-platform nature of the library.
Add hasunix ocamltest action
This test is skipped if *neither* unix nor win32unix have been built.
Add debugger ocamltest action
Skipped if the debugger has not been built.
Add hasstr and hassysthreads ocamltest actions
Skipped if the given library hasn't been built.
Make unix, vmthreads and str optional libraries
--disable-unix-lib, --disable-vmthreads and --disable-str-lib added to
prevent building these three libraries.

ocamldoc, the debugger and caml-tex are automatically disabled if their
prerequisites are not built. Using --enable-debugger and
--enable-ocamldoc will result in errors if these tools cannot be built.

@dra27 dra27 force-pushed the dra27:optional-libs branch from 420ebe9 to 9da9a97 Jan 8, 2019

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

Sorry, only just back to work today! Rebased and updated - the ocamltest changes are unaltered, but the autoconf need reviewing. In particular, this starts the follow-on of some discussions in the original GPR for more appropriate names of some of the parameters (--enable-pthread renamed to --enable-systhreads, etc.)

@nojb

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

Sorry for coming in late to the discussion, but what about --no-bigarray?

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

@nojb - not at all, but do we really want that for the legacy library?

I tied it to --disable-unix-lib, as that's the actual dependency I think?

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

(so in other words --disable-unix-lib means unix isn't built and the legacy bigarray library - the module in the standard library is built regardless, obviously)

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

@nojb

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

My motivation is to have an easy way to not install the legacy bigarray; unless I am wrong one needs to edit the Makefile by hand to do this.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

Add --disable-bigarray-lib
Disables building the legacy bigarray library without also having to
disable the unix library.
@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

@nojb - ah, I see. Extra --disable-bigarray-lib added

@shindere - thanks!

@nojb

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

Thanks!

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

@dra27 dra27 merged commit 7491f8a into ocaml:trunk Jan 9, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

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.