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

Simplify installation of OCaml header (.h) files #1127

Merged
merged 4 commits into from Apr 27, 2017

Conversation

@shindere
Copy link
Contributor

shindere commented Mar 29, 2017

This PR should NOT be merged right now because it would break ci-build.
It's for discussion and later merge.

The purpose here is to simplify the installation of OCaml header files.
Before this PR, header files were processed by a sed script before being
installed. The purpose of this script was twofold:

  1. Inlining the content of config/s.h and config/m.h (in
    byterun/caml/config.h)

  2. Removing the lines between special comments containing the and
    tags.

The private tags were used only in two places: config.h and memory.h.

This PR simplifies all this by:

  1. Replacing the private tags in memory.h by uses of #ifdef CAML_INTERNALS

Is that Okay with you, @cakeplus? ?

  1. Moving config/m.h to byterun/caml/machine.h and config/s.h to
    byterun/caml/system.h.

This lets config.h just include these files normally in a way which wokrs
both in the source tree and when the files get installed.

However it modifies the build system a bit, see e.g. the documentation
patch for building on Windows.

@shindere shindere force-pushed the shindere:simplify-headers-installation branch from e9a828e to 3f42d03 Mar 29, 2017
@murmour

This comment has been minimized.

Copy link
Contributor

murmour commented Mar 30, 2017

Replacing the private tags in memory.h by uses of #ifdef CAML_INTERNALS
Is that Okay with you, @cakeplus? ?

I think it's OK. There won't be much tragedy if the user sees a couple of internal functions wrapped into an #ifdef. I probably copied that <private> tag from config.h.

@damiendoligez

This comment has been minimized.

Copy link
Member

damiendoligez commented Mar 30, 2017

Some time ago, the <private> tag was used all over the headers, then it was replaced (by @lefessan) with #ifdef CAML_INTERNALS, which is a more standard way of doing such things (see for example the Linux system headers).

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Mar 30, 2017

@shindere shindere force-pushed the shindere:simplify-headers-installation branch from 3f42d03 to ad61db4 Apr 19, 2017
@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Apr 24, 2017

The PR has been updated and rebased on latest trunk.

In theory it should now be ready to be merged, except that it does not
build on AppVeyor. Looking at the error, it seems the system
is not properly configured before the build starts. Can anybody see why?
@dra27?

@shindere shindere force-pushed the shindere:simplify-headers-installation branch from ad61db4 to 3a38f39 Apr 26, 2017
shindere added 4 commits Mar 28, 2017
…irectory

This commit moves:
  - config/m.h to byterun/caml/m.h
  - config/s.h to byterun/caml/s.h

Consequently, m.h and s.h now get installed alongside other
OCaml header files.

This commit also updates the .depend files, introducing updates in the
dependencies which are not consequences of this commit itself.
Remove the <include> directives used by tools/cleanup-header to inline
the content of machine.h and system.h.
Now that these files are installed, the installed version of config.h can
include them the normal way.

This commit also gets rid of the <private> and </private> tags used
by the previously mentionned script.
As a consequence, the line it contains will be part of the installed
version of config.h, but that should not make a difference because it
seems unlikely that user code defines the BOOTSTRAPPING_FLEXLINK macro.
Protect private code with the #ifdef CAML_INTERNALS rather than <private> tag.

Thus, the protected code will be included in the installed version
of memory.h but should not be compiled when the header is included
in user code.
The tools/cleanup-header script has no effect any more, so stop
calling it during make install and remove it from the repository.
@shindere shindere force-pushed the shindere:simplify-headers-installation branch from 3a38f39 to 2557205 Apr 26, 2017
@shindere shindere merged commit 2557205 into ocaml:trunk Apr 27, 2017
1 of 4 checks passed
1 of 4 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shindere shindere deleted the shindere:simplify-headers-installation branch Apr 27, 2017
cp config/m-nt.h config/m.h
cp config/s-nt.h config/s.h
cp config/m-nt.h byterun/caml/m.h
cp config/s-nt.h caml/byterun/s.h

This comment has been minimized.

Copy link
@dra27

dra27 May 1, 2017

Contributor

This looks like the source of the AppVeyor failure...

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented May 1, 2017

Sorry for being very late to this one, I've been very focused on OPAM recently.

I must confess I'm slightly surprised at merging with a broken CI and at not really a chance for a consensus around a patch marked after-next-release?

I certainly don't find the change of build instructions for Windows to be particularly great, especially when considering (as I am at the moment) maintaining OPAM switches for pull requests, etc. Are there also potential impacts for other platforms or set-ups which may quite reasonably rely on configure emitting config/{s,m}.h and then for whatever reason edit the output? I don't believe that's ever been a disallowed thing?

Would it not be better to have byterun/caml/{m.s}.h remaining in .gitignore and simply copy (or symlink) the files from config/ in byterun/caml/? That would seem a more compatible way of doing this. Obviously, any pain for Windows goes away once configure is used to set things up, but there's clearly at least a gap (even if only trunk) before that's happening.

#1158 fixes AppVeyor.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 2, 2017

For the record, @xclerc just reported that this change breaks jbuilder, which greps things in config.h that are now only present in {m,s}.h. I don't think we want to change anything for the 4.06 release at this point, so there is nothing specific we need to do (I assume the jbuilder people will implement a workaround) -- I'm just posting for future reference and to improve our understanding of the affect of these kind of changes.

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Nov 2, 2017

@gasche gasche removed the work-in-progress label Nov 3, 2017
gasche added a commit that referenced this pull request Nov 3, 2017
@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Nov 12, 2017

@Chris00

This comment has been minimized.

Copy link
Member

Chris00 commented Nov 12, 2017

@shindere It is fine now — I did not update my CI instructions properly. Sorry for the false alarm (I thought I deleted the comment before Github sent it).

@shindere

This comment has been minimized.

Copy link
Contributor Author

shindere commented Nov 12, 2017

@hannesm hannesm mentioned this pull request Nov 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.