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

Make sure ocamlnat is built with a $(EXE) extension. #729

Closed
wants to merge 2 commits into
base: trunk
from

Conversation

Projects
None yet
3 participants
@shindere
Contributor

shindere commented Jul 31, 2016

Before this commit, 'make ocamlnat' was building 'ocamlnat' without any
extension, even on Windows. The extension was added by 'make install', though.

With this commit, 'ocamlnat' is given its $(EXE) extension already
at build time.

Share the rules needed to build the native toplevel.
One side-effect is that ocamlnat is now removed by make clean under
Windows (mingw and msvc), which was not the case before.
Show outdated Hide outdated Makefile.shared
# provide a phony 'ocamlnat' synonym
ifneq ($(EXE),)
.PHYNY: ocamlnat

This comment has been minimized.

@alainfrisch

alainfrisch Jul 31, 2016

Contributor

Typo PHYNY->PHONY.

@alainfrisch

alainfrisch Jul 31, 2016

Contributor

Typo PHYNY->PHONY.

This comment has been minimized.

@alainfrisch

alainfrisch Jul 31, 2016

Contributor

Why do we need this synonym for ocamlnat (and not other tools)?

@alainfrisch

alainfrisch Jul 31, 2016

Contributor

Why do we need this synonym for ocamlnat (and not other tools)?

This comment has been minimized.

@shindere

shindere Jul 31, 2016

Contributor

Alain Frisch (2016/07/31 06:29 -0700):

@@ -311,3 +311,30 @@ partialclean::
rm -f driver/compdynlink.mli
rm -f driver/compdynlink.mlopt

+# The native toplevel
+
+compilerlibs/ocamlopttoplevel.cmxa: $(OPTTOPLEVEL:.cmo=.cmx)

  • $(CAMLOPT) -a -o $@ $(OPTTOPLEVEL:.cmo=.cmx)
    +partialclean::
  • rm -f compilerlibs/ocamlopttoplevel.cmxa

+# When the native toplevel executable has an extension (e.g. ".exe"),
+# provide a phony 'ocamlnat' synonym
+
+ifneq ($(EXE),)
+.PHYNY: ocamlnat

Typo PHYNY->PHONY.

Fixed, thanks!!

@shindere

shindere Jul 31, 2016

Contributor

Alain Frisch (2016/07/31 06:29 -0700):

@@ -311,3 +311,30 @@ partialclean::
rm -f driver/compdynlink.mli
rm -f driver/compdynlink.mlopt

+# The native toplevel
+
+compilerlibs/ocamlopttoplevel.cmxa: $(OPTTOPLEVEL:.cmo=.cmx)

  • $(CAMLOPT) -a -o $@ $(OPTTOPLEVEL:.cmo=.cmx)
    +partialclean::
  • rm -f compilerlibs/ocamlopttoplevel.cmxa

+# When the native toplevel executable has an extension (e.g. ".exe"),
+# provide a phony 'ocamlnat' synonym
+
+ifneq ($(EXE),)
+.PHYNY: ocamlnat

Typo PHYNY->PHONY.

Fixed, thanks!!

This comment has been minimized.

@shindere

shindere Jul 31, 2016

Contributor

Alain Frisch (2016/07/31 06:30 -0700):

@@ -311,3 +311,30 @@ partialclean::
rm -f driver/compdynlink.mli
rm -f driver/compdynlink.mlopt

+# The native toplevel
+
+compilerlibs/ocamlopttoplevel.cmxa: $(OPTTOPLEVEL:.cmo=.cmx)

  • $(CAMLOPT) -a -o $@ $(OPTTOPLEVEL:.cmo=.cmx)
    +partialclean::
  • rm -f compilerlibs/ocamlopttoplevel.cmxa

+# When the native toplevel executable has an extension (e.g. ".exe"),
+# provide a phony 'ocamlnat' synonym
+
+ifneq ($(EXE),)
+.PHYNY: ocamlnat

Why do we need this synonym for ocamlnat (and not other tools)?

Well I wanted to preserve the ability to do

make ocamlnat

even under Windows

But I don't mind to remove it if it's something people dislike.

@shindere

shindere Jul 31, 2016

Contributor

Alain Frisch (2016/07/31 06:30 -0700):

@@ -311,3 +311,30 @@ partialclean::
rm -f driver/compdynlink.mli
rm -f driver/compdynlink.mlopt

+# The native toplevel
+
+compilerlibs/ocamlopttoplevel.cmxa: $(OPTTOPLEVEL:.cmo=.cmx)

  • $(CAMLOPT) -a -o $@ $(OPTTOPLEVEL:.cmo=.cmx)
    +partialclean::
  • rm -f compilerlibs/ocamlopttoplevel.cmxa

+# When the native toplevel executable has an extension (e.g. ".exe"),
+# provide a phony 'ocamlnat' synonym
+
+ifneq ($(EXE),)
+.PHYNY: ocamlnat

Why do we need this synonym for ocamlnat (and not other tools)?

Well I wanted to preserve the ability to do

make ocamlnat

even under Windows

But I don't mind to remove it if it's something people dislike.

This comment has been minimized.

@alainfrisch

alainfrisch Jul 31, 2016

Contributor

Please check, but I believe there is some magic in Cygwin's make which makes this useless.

@alainfrisch

alainfrisch Jul 31, 2016

Contributor

Please check, but I believe there is some magic in Cygwin's make which makes this useless.

This comment has been minimized.

@shindere

shindere Jul 31, 2016

Contributor

Alain Frisch (2016/07/31 07:20 -0700):

+ifneq ($(EXE),)
+.PHYNY: ocamlnat

Please check, but I believe there is some magic in Cygwin's make which
makes this useless.

I just tried commenting the rule. When it is commented, make ocamlnat
fails with the message

make: *** No rule to make target 'ocamlnat'. Stop.

@shindere

shindere Jul 31, 2016

Contributor

Alain Frisch (2016/07/31 07:20 -0700):

+ifneq ($(EXE),)
+.PHYNY: ocamlnat

Please check, but I believe there is some magic in Cygwin's make which
makes this useless.

I just tried commenting the rule. When it is commented, make ocamlnat
fails with the message

make: *** No rule to make target 'ocamlnat'. Stop.

This comment has been minimized.

@alainfrisch

alainfrisch Jul 31, 2016

Contributor
@alainfrisch

alainfrisch via email Jul 31, 2016

Contributor

This comment has been minimized.

@shindere

shindere Jul 31, 2016

Contributor

Alain Frisch (2016/07/31 08:13 -0700):

Did you try with the original rule targetting "ocamlnat" (even if it
produces "ocamlnat.exe" in reality)?

Do you mean something like

ocamlnat: compilerlibs/ocamlcommon.cmxa compilerlibs/ocamloptcomp.cmxa
compilerlibs/ocamlbytecomp.cmxa
compilerlibs/ocamlopttoplevel.cmxa
$(OPTTOPLEVELSTART:.cmo=.cmx)
$(CAMLOPT) $(LINKFLAGS) -linkall -o $@ ocamlnat$(EXE)

?

Well to be honest, this rule that may not exactly produce its target
looks a bit odd to me, I'd even say a bit awkward.

Even if that is going to work under Cygwin which is currently a build
requirement for OCaml under Windows, I find it a pity to introduce a
solution which relies on this when it i possible to do otherwise.

@shindere

shindere Jul 31, 2016

Contributor

Alain Frisch (2016/07/31 08:13 -0700):

Did you try with the original rule targetting "ocamlnat" (even if it
produces "ocamlnat.exe" in reality)?

Do you mean something like

ocamlnat: compilerlibs/ocamlcommon.cmxa compilerlibs/ocamloptcomp.cmxa
compilerlibs/ocamlbytecomp.cmxa
compilerlibs/ocamlopttoplevel.cmxa
$(OPTTOPLEVELSTART:.cmo=.cmx)
$(CAMLOPT) $(LINKFLAGS) -linkall -o $@ ocamlnat$(EXE)

?

Well to be honest, this rule that may not exactly produce its target
looks a bit odd to me, I'd even say a bit awkward.

Even if that is going to work under Cygwin which is currently a build
requirement for OCaml under Windows, I find it a pity to introduce a
solution which relies on this when it i possible to do otherwise.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Jul 31, 2016

Contributor

Other executables are also built without $(EXE) (which is added at installation time). Why is a different treatment of ocamlnat required?

(And btw, thanks for the extra sharing between Makefile and Makefile.nt.)

Contributor

alainfrisch commented Jul 31, 2016

Other executables are also built without $(EXE) (which is added at installation time). Why is a different treatment of ocamlnat required?

(And btw, thanks for the extra sharing between Makefile and Makefile.nt.)

Make sure ocamlnat is built with a $(EXE) extension.
Before this commit, 'make ocamlnat' was building 'ocamlnat' without any
extension, even on Windows. The extension was added by 'make install', though.

With this commit, 'ocamlnat' is given its $(EXE) extension already
at build time.
@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Jul 31, 2016

Contributor

Alain Frisch (2016/07/31 06:33 -0700):

Other executables are also built without $(EXE) (which is added at
installation time). Why is a different treatment of ocamlnat
required?

Well, I'd say that ultimately I believe the other executables should
also have the ".exe" added as soon as they are built rather than
installed. First it seems a bit more logical to me, second and perhaps
more concretely it lets one run the executable without having to install
it outside of a Cygwin environment.

(And btw, thanks for the extra sharing between Makefile and Makefile.nt.)

:)

Contributor

shindere commented Jul 31, 2016

Alain Frisch (2016/07/31 06:33 -0700):

Other executables are also built without $(EXE) (which is added at
installation time). Why is a different treatment of ocamlnat
required?

Well, I'd say that ultimately I believe the other executables should
also have the ".exe" added as soon as they are built rather than
installed. First it seems a bit more logical to me, second and perhaps
more concretely it lets one run the executable without having to install
it outside of a Cygwin environment.

(And btw, thanks for the extra sharing between Makefile and Makefile.nt.)

:)

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Jul 31, 2016

Contributor

Sébastien Hinderer (2016/07/31 15:44 +0200):

Alain Frisch (2016/07/31 06:33 -0700):

Other executables are also built without $(EXE) (which is added at
installation time). Why is a different treatment of ocamlnat
required?

Well, I'd say that ultimately I believe the other executables should
also have the ".exe" added as soon as they are built rather than
installed. First it seems a bit more logical to me, second and perhaps
more concretely it lets one run the executable without having to install
it outside of a Cygwin environment.

Oh and by the way: ocamlrun is already in this situation of being built
with a .exe extension so ocamlnat would not be the first file to be
treated that way.

Contributor

shindere commented Jul 31, 2016

Sébastien Hinderer (2016/07/31 15:44 +0200):

Alain Frisch (2016/07/31 06:33 -0700):

Other executables are also built without $(EXE) (which is added at
installation time). Why is a different treatment of ocamlnat
required?

Well, I'd say that ultimately I believe the other executables should
also have the ".exe" added as soon as they are built rather than
installed. First it seems a bit more logical to me, second and perhaps
more concretely it lets one run the executable without having to install
it outside of a Cygwin environment.

Oh and by the way: ocamlrun is already in this situation of being built
with a .exe extension so ocamlnat would not be the first file to be
treated that way.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Jul 31, 2016

Contributor

I've no strong opinion on which is the best approach, although your argument about being able to use the tools (before installation) from outside Cygwin seems compelling (but not overwhelming: who would do that in practice?). In any case, if we go this direction, I think we should apply the same treatment to all tools.

Contributor

alainfrisch commented Jul 31, 2016

I've no strong opinion on which is the best approach, although your argument about being able to use the tools (before installation) from outside Cygwin seems compelling (but not overwhelming: who would do that in practice?). In any case, if we go this direction, I think we should apply the same treatment to all tools.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Jul 31, 2016

Contributor

Alain Frisch (2016/07/31 07:07 -0700):

I've no strong opinion on which is the best approach, although your
argument about being able to use the tools (before installation) from
outside Cygwin seems compelling (but not overwhelming: who would do
that in practice?).

End-users would probably not, indeed. Developers may be more likely to
have this need.

But also, as I said, it looks odd to me to add an extension at install
rather than build time, in general.

In any case, if we go this direction, I think we
should apply the same treatment to all tools.

I mean, as I said the situation was already a mixture of the two
strategies before this PR, ocamlrun.exe being the only tool that had the
.exe added at build rather than install time. So indeed, this PR does
not realy fix the fact that things are mixed. but it makes one step in
this direction...

Contributor

shindere commented Jul 31, 2016

Alain Frisch (2016/07/31 07:07 -0700):

I've no strong opinion on which is the best approach, although your
argument about being able to use the tools (before installation) from
outside Cygwin seems compelling (but not overwhelming: who would do
that in practice?).

End-users would probably not, indeed. Developers may be more likely to
have this need.

But also, as I said, it looks odd to me to add an extension at install
rather than build time, in general.

In any case, if we go this direction, I think we
should apply the same treatment to all tools.

I mean, as I said the situation was already a mixture of the two
strategies before this PR, ocamlrun.exe being the only tool that had the
.exe added at build rather than install time. So indeed, this PR does
not realy fix the fact that things are mixed. but it makes one step in
this direction...

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 1, 2016

Member

Given that our build system is nicer with this patch applied than before, I would rather be in favor of merging as-is and creating a mantis issue to track the need for the transition for other tools (I can take care of that).

Member

gasche commented Aug 1, 2016

Given that our build system is nicer with this patch applied than before, I would rather be in favor of merging as-is and creating a mantis issue to track the need for the transition for other tools (I can take care of that).

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 1, 2016

Contributor

Manually rebased and pushed to trunk. Will be part of the version strictly after 4.04 (4.05?).

Contributor

alainfrisch commented Aug 1, 2016

Manually rebased and pushed to trunk. Will be part of the version strictly after 4.04 (4.05?).

@alainfrisch alainfrisch closed this Aug 1, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 1, 2016

Member

I created an issue: MPR#7312.

Member

gasche commented Aug 1, 2016

I created an issue: MPR#7312.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Aug 9, 2016

Contributor
Contributor

shindere commented Aug 9, 2016

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Aug 9, 2016

Contributor

Alain Frisch (2016/08/01 08:32 -0700):

Manually rebased and pushed to trunk. Will be part of the version
strictly after 4.04 (4.05?).

Thanks!

Contributor

shindere commented Aug 9, 2016

Alain Frisch (2016/08/01 08:32 -0700):

Manually rebased and pushed to trunk. Will be part of the version
strictly after 4.04 (4.05?).

Thanks!

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Aug 9, 2016

Contributor

Gabriel Scherer (2016/08/01 08:43 -0700):

I created an issue:
MPR#7312.

Thanks!

Contributor

shindere commented Aug 9, 2016

Gabriel Scherer (2016/08/01 08:43 -0700):

I created an issue:
MPR#7312.

Thanks!

@shindere shindere deleted the shindere:improve-ocamlnat-building branch Aug 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment