Allow development configure options to be stored in Git by dra27 · Pull Request #8995 · ocaml/ocaml

dra27 · 2019-09-29T11:50:44Z

This PR proposes some pre-processing in the configure script solely for Git checkouts (i.e. for compiler developers, not compiler users).

If (and only if) the directory from which configure is executed contains .git (i.e. it's a Git clone), then two Git configuration values are queried.

ocaml.configure - this value is inserted (expanded) before the command line arguments passed to configure
ocaml.configure-cache - specifies a directory relative to the configure script where host-specific cache files should be read/written

The first (ocaml.configure) is the simplest. This allows options one might want all the time (for example, I know @stedolan is a fan of -C, which turns on caching and @mshinwell usually configures with --disable-ocamldoc). The insertion of these options before those passed to configure means that ./configure --enable-ocamldoc still has the meaning expected. Usefully, autoconf displays warnings only for --enable and --with options which don't exist, so one can set this even if working with multiple versions of OCaml. The use of Git configuration variables of course allows you to set --global but then override it in a specific clone (or even worktree).

The second (ocaml.configure-cache) is a little more complicated, but would certainly make my Windows development life less painful (cc @nojb and @shindere). A Windows developer is hugely motivated to use ./configure -C given that on my machine, ./configure --build=i686-pc-cygwin --host=x86_64-pc-windows -C takes 77 seconds the first time and "only" 22 seconds subsequently (and, for reference, make -j32 world.opt only takes 260 seconds, so that extra 55 seconds in configure is a significant chunk). Now if, like me, you have a lot of worktrees, it becomes tempting to share the config.cache file which you can do by specifying --cache-file= instead of -C. However, the cache is only valid if the build-host-target triplet and any precious environment variables (CFLAGS, etc.) are identical. So you now have to remember which cache file to use. That's a bit much for someone as forgetful as me, hence this feature. On my machine, with all the worktrees in the same directory, I can now issue git config --global ocaml.configure-cache .. and now ./configure --build=i686-pc-cygwin --host=x86_64-pc-windows will add --cache-file ../ocaml-x86_64-pc-windows.cache to the invocation. If I suddenly want to reconfigure the tree for mingw, I don't get an error message about a stale cache (which I would with -C). If --host is not specified then default is used.

Various observations on the safety of this:

Builds from the tarball are unaffected, since there's no .git entry. The tarballs could be hardened even further by not including the altered configure script (i.e. the release procedure could involve running autoconf --force and distributing the resulting configure instead).
If you don't set either of these options in Git, nothing happens!

If this change is accepted, I'll push a change to HACKING.adoc to advertise its availability.

gasche

(I haven't reviewed the actual code, just glanced at the diff.)

This sounds generally reasonable to me: it's not an invasive change and it makes some people's life easier.

This is a small risk of making the provenance of configuration more opaque/surprising. Do we get a notification in the configure.log of some sort when this feature affects the configuration options?

autogen

dra27 · 2019-09-29T14:39:06Z

Thanks, @gasche - there is a message, yes:

$ ./configure --build=i686-pc-cygwin --host=x86_64-pc-windows
ocaml.configure is set
Re-running ./configure --disable-debug-runtime --disable-ocamldoc --cache-file "./../ocaml-x86_64-pc-windows.cache" --build=i686-pc-cygwin --host=x86_64-pc-windows
configure: loading cache ./../ocaml-x86_64-pc-windows.cache
configure: Configuring OCaml version 4.10.0+dev0-2019-04-23

Obviously, that message is at the start - I could do similar to autoconf and ensure that the message is displayed at the end of the script (cf. the last 4 lines of configure)?

gasche · 2019-09-29T17:32:45Z

Having at the start is fine (especially if it gives the most natural implementation), and it's nice that the log shows the full options that are running. On the other hand, ocaml.configure is set is not very descriptive (I'm thinking of a situation where someone would have completely forgotten that this tweak exist by the time they try to audit their configure result). If you can control the message, maybe something more explicit like "The git setting ocaml.configure contains the extra parameters "...". could help rediscoverability?

dra27 · 2019-09-30T09:09:06Z

Rebased and messages improved. It now looks like:

$ ./configure
Detected Git configuration option ocaml.configure-cache set to ".."
Detected Git configuration option ocaml.configure set to "--disable-ocamldoc"
Re-running ./configure --disable-ocamldoc --cache-file "./../ocaml-default.cache" 
configure: creating cache ./../ocaml-default.cache
configure: Configuring OCaml version 4.10.0+dev0-2019-04-23

gasche · 2019-09-30T11:50:44Z

I like the new interface and I'm in favor of the feature.

I looked at the implementation briefly. I think it's reasonable, but I wonder whether some of the scripting logic could be moved away from an EOF embedding into autogen into a proper script in tools/ (that could be either called or included). I'm thinking for example of someone wishing to extend the script later to add support for another versioning system; having the code in a more natural place (and maybe testable independently of the autoconf context) could make it easier.

dra27 · 2019-10-01T09:39:12Z

I too had wondered about the fragment once it grew (even just wanting syntax highlighting, which you of course don't get in a heredoc). I've moved to a script of its own - I've put it at the root since you can now run ./git-dev-options.sh and you get the messages:

Detected Git configuration option ocaml.configure-cache set to ".."
Detected Git configuration option ocaml.configure set to "--disable-ocamldoc"
Re-running ./git-dev-options.sh --disable-ocamldoc --cache-file "./../ocaml-default.cache"

which you don't get if it's in tools/ (because the check for .git will fail).

On the basis that configure must be patched to include a call to the script anyway, embedding the script in configure seems best. I put a tiny of logic to stop the header from the fragment making configure look like my work!

gasche · 2019-10-01T10:01:14Z

Sorry to nitpick, but I would prefer if the script could be in tools -- I do actually sort of wonder about scalability of ls at the root. Wouldn't it work to use relative addressing (just .git) in the script instead of dirname "$0", so that it works when invoked from ./configure and also as sh tools/git-dev-options.sh?

(If we wanted to be really perfectionist about the usability of this assumed usage pattern (but we don't have to!), we could have the not-in-a-git-directory case print a readable message like Not invoked from the root of a git repository; that would be fine in the config.log trace and help users who would wrongly invoke it from tools directly.)

dra27 · 2019-10-01T10:09:03Z

I had out-of-tree builds in mind when I added the dirname check (I was being paranoid that enabling these options truly would mean you were on a Git checkout of OCaml and nothing else). That said, while you configure OCaml for an out-of-tree build, the build system doesn't support it...

gasche · 2019-10-01T10:19:30Z

I am satiated with nitpicking and I don't see anything else to do than merge when the CI comes back green. Thanks!

gasche · 2019-10-01T10:20:16Z

(Well actually, what about the commit history? Would you like me to squash-and-merge? Urgh.)

dra27 · 2019-10-01T10:36:45Z

Thanks. @gasche - I'll mention the stuff in HACKING as promised and tidy up the Git history with that

dra27 · 2019-10-05T12:48:30Z

Rebased and history tidied up - I added notes about the two options to HACKING.adoc. I also added /ocaml-*.cache to .gitignore so that if you do git config --global ocaml.configure-cache . (which is one of the suggestions) then the files are ignored by Git, just like config.cache.

gasche · 2019-10-05T13:42:01Z

I would recommend having a Changes entry for this new (development) feature, to make it more discoverable by contributors who don't just re-read HACKING periodically.

The Git configuration value ocaml.configure is now passed to the configure script's arguments before $@ if (and only if) OCaml is being configured from a Git clone. This allows, for example: - Developer-specific preferences (e.g. `--disable-ocamldoc` or `--disable-debug-runtime`) - Automatic use of autoconf cach files (-C option) It is implemented by inserting a test at the top of `configure`, which is bypassed if `.git` doesn't exist.

The Git configuration value ocaml.configure-cache can be used to specify a directory to keep autoconf cache files in, relative to the worktree root (so `git config ocaml.configure-cache .` enables the feature, and `git config --global ocaml.configure-cache ..` enables it for all worktrees, assuming they're at the same level). autoconf's --cache-file option speeds up future runs of configure by caching the results of previous tests. The cache is invalidated if any environment variables differ (e.g. LDFLAGS) or if the build-host-target triplet differs. This is a nuisance on Windows, where configure is both very slow and it's also common to build with multiple different --host values. This PR allows a tree to be quickly reconfigured from one Windows port to another.

dra27 · 2019-10-05T13:51:56Z

Howzat? 🙂

gasche · 2019-10-05T14:50:50Z

Having each commit contribute to the change entry to say what's in the commit itself? We should have medals for those PRs.

dra27 · 2019-10-05T15:26:39Z

🏅

gasche reviewed Sep 29, 2019

View reviewed changes

autogen Outdated Show resolved Hide resolved

dra27 force-pushed the dev-opts branch from 5dde4cd to 1d68e18 Compare September 30, 2019 09:08

dra27 mentioned this pull request Sep 30, 2019

New configure option to disable building and installing library manpages #8835

Merged

dra27 force-pushed the dev-opts branch from 1d68e18 to 62ec588 Compare October 1, 2019 09:35

dra27 force-pushed the dev-opts branch from b88fe91 to e50ed8a Compare October 5, 2019 12:45

dra27 added the no-change-entry-needed label Oct 5, 2019

dra27 added 2 commits October 5, 2019 14:50

dra27 force-pushed the dev-opts branch from e50ed8a to 4f09104 Compare October 5, 2019 13:51

dra27 removed the no-change-entry-needed label Oct 5, 2019

gasche merged commit 7e34c1e into ocaml:trunk Oct 5, 2019

dra27 deleted the dev-opts branch October 5, 2019 17:01

gasche mentioned this pull request Dec 12, 2019

Enough with multiple "common" definitions of C global variables #9180

Merged

Conversation

dra27 commented Sep 29, 2019

Uh oh!

gasche left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dra27 commented Sep 29, 2019

Uh oh!

gasche commented Sep 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dra27 commented Sep 30, 2019

Uh oh!

gasche commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dra27 commented Oct 1, 2019

Uh oh!

gasche commented Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dra27 commented Oct 1, 2019

Uh oh!

gasche commented Oct 1, 2019

Uh oh!

gasche commented Oct 1, 2019

Uh oh!

dra27 commented Oct 1, 2019

Uh oh!

dra27 commented Oct 5, 2019

Uh oh!

gasche commented Oct 5, 2019

Uh oh!

dra27 commented Oct 5, 2019

Uh oh!

gasche commented Oct 5, 2019

Uh oh!

dra27 commented Oct 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gasche left a comment •

edited

Loading

gasche commented Sep 29, 2019 •

edited

Loading

gasche commented Sep 30, 2019 •

edited

Loading

gasche commented Oct 1, 2019 •

edited

Loading