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

Merge Unix and Windows build systems in the ocamldoc directory #808

Merged
merged 1 commit into from Sep 14, 2016

Conversation

Projects
None yet
3 participants
@shindere
Contributor

shindere commented Sep 13, 2016

This PR continues the build-systems merging work started in PRs
#762, #764, #785 and #788.

Here again, the aim is to make ocamldoc/Makefile work for both
Unix and Windows, so that ocamldoc/Makefile.nt can just include it.

So, although this PR improves a few aspects of the build process of ocamldoc
(see commit log for details of those improvements and changes), it is
worth emphasizing that improving the build process itself is not the primary
target of this PR, whose goal is, again, to merge the build systems.

Show outdated Hide outdated ocamldoc/Makefile
ifeq "$(UNIX_OR_WIN32)" "unix"
OCAMLDEPFLAGS =
else # Windows
OCAMLDEPFLAGS = -slash

This comment has been minimized.

@alainfrisch

alainfrisch Sep 13, 2016

Contributor

It seems -slash does nothing under Unix, so one might be able to avoid this conditional.

@alainfrisch

alainfrisch Sep 13, 2016

Contributor

It seems -slash does nothing under Unix, so one might be able to avoid this conditional.

This comment has been minimized.

@shindere

shindere Sep 14, 2016

Contributor

Alain Frisch (2016/09/13 13:54 -0700):

+OCAMLDEP = $(OCAMLRUN) $(ROOTDIR)/tools/ocamldep
+ifeq "$(UNIX_OR_WIN32)" "unix"
+OCAMLDEPFLAGS =
+else # Windows
+OCAMLDEPFLAGS = -slash

It seems -slash does nothing under Unix,

Indeed (I chekced). Many thanks for having spotted this.

so one might be able to avoid this conditional.

Doone.

@shindere

shindere Sep 14, 2016

Contributor

Alain Frisch (2016/09/13 13:54 -0700):

+OCAMLDEP = $(OCAMLRUN) $(ROOTDIR)/tools/ocamldep
+ifeq "$(UNIX_OR_WIN32)" "unix"
+OCAMLDEPFLAGS =
+else # Windows
+OCAMLDEPFLAGS = -slash

It seems -slash does nothing under Unix,

Indeed (I chekced). Many thanks for having spotted this.

so one might be able to avoid this conditional.

Doone.

Show outdated Hide outdated ocamldoc/Makefile
OCAMLDEPFLAGS = -slash
endif
OCAMLLEX = $(OCAMLRUN) $(ROOTDIR)/boot/ocamllex
# TODO: clarify whether the preprocessor really needs to be different here

This comment has been minimized.

@alainfrisch

alainfrisch Sep 13, 2016

Contributor

You cannot simply do -pp './remove_DEBUG' under Windows because the command is executed with cmd.exe which does not know what to do with the shell script (but it knows how to call Cygwin's grep which is in the path). The following should work, though: -pp 'sh ./remove_DEBUG'.

That said, I'm wondering whether one should just remove all those DEBUG lines (and get rid of the preprocessing). Are they actually useful?

@alainfrisch

alainfrisch Sep 13, 2016

Contributor

You cannot simply do -pp './remove_DEBUG' under Windows because the command is executed with cmd.exe which does not know what to do with the shell script (but it knows how to call Cygwin's grep which is in the path). The following should work, though: -pp 'sh ./remove_DEBUG'.

That said, I'm wondering whether one should just remove all those DEBUG lines (and get rid of the preprocessing). Are they actually useful?

This comment has been minimized.

@shindere

shindere Sep 14, 2016

Contributor

Alain Frisch (2016/09/13 14:36 -0700):

You cannot simply do -pp './remove_DEBUG' under Windows because the
command is executed with cmd.exe which does not know what to do with
the shell script (but it knows how to call Cygwin's grep which is in
the path). The following should work, though: -pp 'sh ./remove_DEBUG'.

Indeed, thanks a lot for the suggestion. For sake of completeness,
it is worth mentionning that remove_DEBUG does not invoke grep but sed,
in order to preserve line numbers while removing the DEBUG lines.

The PR has been updated according to your suggestion, so that the
Windows build now uses the same preprocessor as the Unix one, since such
a unification really belongs to the idea of this PR.

That said, I'm wondering whether one should just remove all those
DEBUG lines (and get rid of the preprocessing). Are they actually
useful?

I don't know. I added a note about this in the Makefile because I think,
no matter what is decided, it should be implemented in a separate PR.

One alternative to mere removal could be to make it possible to enable
debugging at runtime rather than at compile time, e.g. by adding a
-debug option to ocamldoc (it does not have one yet).

@shindere

shindere Sep 14, 2016

Contributor

Alain Frisch (2016/09/13 14:36 -0700):

You cannot simply do -pp './remove_DEBUG' under Windows because the
command is executed with cmd.exe which does not know what to do with
the shell script (but it knows how to call Cygwin's grep which is in
the path). The following should work, though: -pp 'sh ./remove_DEBUG'.

Indeed, thanks a lot for the suggestion. For sake of completeness,
it is worth mentionning that remove_DEBUG does not invoke grep but sed,
in order to preserve line numbers while removing the DEBUG lines.

The PR has been updated according to your suggestion, so that the
Windows build now uses the same preprocessor as the Unix one, since such
a unification really belongs to the idea of this PR.

That said, I'm wondering whether one should just remove all those
DEBUG lines (and get rid of the preprocessing). Are they actually
useful?

I don't know. I added a note about this in the Makefile because I think,
no matter what is decided, it should be implemented in a separate PR.

One alternative to mere removal could be to make it possible to enable
debugging at runtime rather than at compile time, e.g. by adding a
-debug option to ocamldoc (it does not have one yet).

Merge Unix and Windows build systems in the ocamldoc directory
This commit changes the behaviour of both the Unix and Windows build systems.
The changes are either specific to one build system or common to both.

Changes specific to the Unix build system:

  - Use the -slash option when invoking ocamldep. This does nothing on
    Unix and makes it possible to use the same command under Unix and Windows.

  - install target:

    - Directories are created unconditionnally, instead of creating them
      only if they do not already exist
      (also true for the installopt target)

    - cp no longer uses the -f flag, to be consistent with the
      other directories

    - The custom directory $(INSTALL_LIBDIR)/custom is no longer created
      because nothing was installed there anyway

  - In the opt.opt target, consecutive calls to make have been
    replaced by prerequisites, enhancing parallelisation opportunities.

Changes specific to the Windows build system:

  - Whaen compiling .ml files, use the same line-number-preserving
    preprocessor as under Unix, rather than the grep -v DEBUG command.

  - ocamldoc generators and odoc_test are now compiled

  - ocamldoc can now be run from sources to generate the documentation
    of the standard library

  - The test targets are now also available under Windows

  - opt.opt now builds native-code versions of the generators

Changes affecting both Unix and Windows build systems:

  - The targets that were depending on the empty "dummy" target
    are now declared as .PHONY targets, dummy has been removed.

  - In the all target, successive calls to make have been replaced
    by prerequisites.

  - Commands executed by make clean are now displayed instead of being
    executed silently.

@alainfrisch alainfrisch merged commit 28f91df into ocaml:trunk Sep 14, 2016

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 15, 2016

Member

As a meta comment: you have been doing a lot of excellent work on deduplicating and improving the compiler distribution build system, in a series of distinct pull requests. Very few of those changes had a Changes entry giving credit for your work.

I think it is probably time for a comprehensive Changes entry that is global in scope (it corresponds to the work of sharing Unix and Windows Makefile across the whole compiler distribution). I think it would be best if it listed all affected directories (in case there is a regression somewhere, users can find the mention of the problematic directory in the changelog), and give the PR numbers of each.

Member

gasche commented Sep 15, 2016

As a meta comment: you have been doing a lot of excellent work on deduplicating and improving the compiler distribution build system, in a series of distinct pull requests. Very few of those changes had a Changes entry giving credit for your work.

I think it is probably time for a comprehensive Changes entry that is global in scope (it corresponds to the work of sharing Unix and Windows Makefile across the whole compiler distribution). I think it would be best if it listed all affected directories (in case there is a regression somewhere, users can find the mention of the problematic directory in the changelog), and give the PR numbers of each.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Sep 15, 2016

Contributor
Contributor

shindere commented Sep 15, 2016

@shindere shindere deleted the shindere:merge-ocamldoc-makefiles branch Sep 15, 2016

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Oct 31, 2016

Contributor

I've found a problem with this PR, related to calling the bytecode executable ocamldoc under Windows. Even if the PATH is extended with explicit paths to otherlibs/win32unix and otherlibs/str, ocamlrun will still look up the stub dlls in the installation directory first, as can be seen by setting OCAMLRUNPARAM=v=256.

If another version was installed in the same target directory before, the old dlls will be loaded which can lead to failure (e.g. I just got "Fatal error: unknown C primitive 'unix_lstat'").

A fix could be to set:

CAML_LD_LIBRARY_PATH="../otherlibs/win32unix;../otherlibs/str"

instead of changing PATH.

@shindere Do you think this is the right thing to do?

Contributor

alainfrisch commented Oct 31, 2016

I've found a problem with this PR, related to calling the bytecode executable ocamldoc under Windows. Even if the PATH is extended with explicit paths to otherlibs/win32unix and otherlibs/str, ocamlrun will still look up the stub dlls in the installation directory first, as can be seen by setting OCAMLRUNPARAM=v=256.

If another version was installed in the same target directory before, the old dlls will be loaded which can lead to failure (e.g. I just got "Fatal error: unknown C primitive 'unix_lstat'").

A fix could be to set:

CAML_LD_LIBRARY_PATH="../otherlibs/win32unix;../otherlibs/str"

instead of changing PATH.

@shindere Do you think this is the right thing to do?

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Nov 9, 2016

Contributor

Ping @shindere . @dra27 : also interested in your opinion about the note above.

Contributor

alainfrisch commented Nov 9, 2016

Ping @shindere . @dra27 : also interested in your opinion about the note above.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Nov 10, 2016

Contributor
Contributor

shindere commented Nov 10, 2016

shindere added a commit to shindere/ocaml that referenced this pull request Nov 10, 2016

Fix the way ocamldoc is called under Windows.
This fixes a problem related to the way ocamldoc's bytecode executable
is called under Windows, reported by @alainfrisch at
ocaml#808 (comment)

As he explains, ``Even if the PATH is extended with explicit paths to
otherlibs/win32unix and otherlibs/str, ocamlrun will still look up
the stub dlls in the installation directory first, as can be seen by
setting OCAMLRUNPARAM=v=256. If another version was installed in the
same target directory before, the old dlls will be loaded which can lead
to failure (e.g. I just got "Fatal error: unknown C primitive 'unix_lstat'").
A fix could be to set:
CAML_LD_LIBRARY_PATH="../otherlibs/win32unix;../otherlibs/str"
instead of changing PATH''.

This commit implements the proposed fix, due to @alainfrisch.

shindere added a commit to shindere/ocaml that referenced this pull request Nov 10, 2016

Fix the way the ocamldoc bytecode executable is called under Windows.
This fixes a problem related to the way ocamldoc's bytecode executable
is called under Windows, reported by @alainfrisch at
ocaml#808 (comment)

As he explains, ``Even if the PATH is extended with explicit paths to
otherlibs/win32unix and otherlibs/str, ocamlrun will still look up
the stub dlls in the installation directory first, as can be seen by
setting OCAMLRUNPARAM=v=256. If another version was installed in the
same target directory before, the old dlls will be loaded which can lead
to failure (e.g. I just got "Fatal error: unknown C primitive 'unix_lstat'").
A fix could be to set:
CAML_LD_LIBRARY_PATH="../otherlibs/win32unix;../otherlibs/str"
instead of changing PATH''.

This commit implements the proposed fix, due to @alainfrisch.
@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Nov 10, 2016

Contributor
Contributor

shindere commented Nov 10, 2016

shindere added a commit to shindere/ocaml that referenced this pull request Nov 10, 2016

Fix the way the ocamldoc bytecode executable is called under Windows.
This fixes a problem related to the way ocamldoc's bytecode executable
is called under Windows, reported by @alainfrisch at
ocaml#808 (comment)

As he explains, ``Even if the PATH is extended with explicit paths to
otherlibs/win32unix and otherlibs/str, ocamlrun will still look up
the stub dlls in the installation directory first, as can be seen by
setting OCAMLRUNPARAM=v=256. If another version was installed in the
same target directory before, the old dlls will be loaded which can lead
to failure (e.g. I just got "Fatal error: unknown C primitive 'unix_lstat'").
A fix could be to set:
CAML_LD_LIBRARY_PATH="../otherlibs/win32unix;../otherlibs/str"
instead of changing PATH''.

This commit implements the proposed fix, due to @alainfrisch.

alainfrisch added a commit that referenced this pull request Nov 10, 2016

Fix the way the ocamldoc bytecode executable is called under Windows. (
…#906)

This fixes a problem related to the way ocamldoc's bytecode executable
is called under Windows, reported by @alainfrisch at
#808 (comment)

As he explains, ``Even if the PATH is extended with explicit paths to
otherlibs/win32unix and otherlibs/str, ocamlrun will still look up
the stub dlls in the installation directory first, as can be seen by
setting OCAMLRUNPARAM=v=256. If another version was installed in the
same target directory before, the old dlls will be loaded which can lead
to failure (e.g. I just got "Fatal error: unknown C primitive 'unix_lstat'").
A fix could be to set:
CAML_LD_LIBRARY_PATH="../otherlibs/win32unix;../otherlibs/str"
instead of changing PATH''.

This commit implements the proposed fix, due to @alainfrisch.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge Unix and Windows build systems in the ocamldoc directory (#808)
This commit changes the behaviour of both the Unix and Windows build systems.
The changes are either specific to one build system or common to both.

Changes specific to the Unix build system:

  - Use the -slash option when invoking ocamldep. This does nothing on
    Unix and makes it possible to use the same command under Unix and Windows.

  - install target:

    - Directories are created unconditionnally, instead of creating them
      only if they do not already exist
      (also true for the installopt target)

    - cp no longer uses the -f flag, to be consistent with the
      other directories

    - The custom directory $(INSTALL_LIBDIR)/custom is no longer created
      because nothing was installed there anyway

  - In the opt.opt target, consecutive calls to make have been
    replaced by prerequisites, enhancing parallelisation opportunities.

Changes specific to the Windows build system:

  - Whaen compiling .ml files, use the same line-number-preserving
    preprocessor as under Unix, rather than the grep -v DEBUG command.

  - ocamldoc generators and odoc_test are now compiled

  - ocamldoc can now be run from sources to generate the documentation
    of the standard library

  - The test targets are now also available under Windows

  - opt.opt now builds native-code versions of the generators

Changes affecting both Unix and Windows build systems:

  - The targets that were depending on the empty "dummy" target
    are now declared as .PHONY targets, dummy has been removed.

  - In the all target, successive calls to make have been replaced
    by prerequisites.

  - Commands executed by make clean are now displayed instead of being
    executed silently.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Fix the way the ocamldoc bytecode executable is called under Windows. (
…#906)

This fixes a problem related to the way ocamldoc's bytecode executable
is called under Windows, reported by @alainfrisch at
ocaml#808 (comment)

As he explains, ``Even if the PATH is extended with explicit paths to
otherlibs/win32unix and otherlibs/str, ocamlrun will still look up
the stub dlls in the installation directory first, as can be seen by
setting OCAMLRUNPARAM=v=256. If another version was installed in the
same target directory before, the old dlls will be loaded which can lead
to failure (e.g. I just got "Fatal error: unknown C primitive 'unix_lstat'").
A fix could be to set:
CAML_LD_LIBRARY_PATH="../otherlibs/win32unix;../otherlibs/str"
instead of changing PATH''.

This commit implements the proposed fix, due to @alainfrisch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment