Build programs with the $(EXE) extension #9652

shindere · 2020-06-07T20:47:41Z

This PR aims at fixing issue #7312.

In addition to what has already been implemented, a few other things
could be included here.

bytecomp/bytelink.ml defines a fix_exec_name function. Rather than
looking at Sys.os_type, this function could just check whether the
provided name ends with the defined extension, leave it
unchanged in that case and add the extension otherwise.
In tools/ocamlmktop.ml, the code that computes ocamlc,extra_quote
could be modified so that the computation of ocamlc becomes something
like "ocamlc" ^ Config.ext_exe.
But, at the moment, tools/ocamlmktop.ml does not depend on the
configuration module. So, wouldn't it be worth making ext_exe also
available e.g. from the Sys module?
Finally, in utils/config.mlp, the computation of
default_executable_name could also be reworked so that just the
basename depends on the OS type but the ext_exe extension then gets
systematically added.

Shall I go ahead and add all these changes to this PR?

Also, there is the case of the tools/read_cmt tool which gets
currently installed as ocamlcmt. Shall I change the PR so that this
program, too, gets built using the name it will have when installed?

Most lileky this PR breaks the dune build, in case somebody wants to
work on it, e.g. @trefis

dra27 · 2020-06-09T13:36:59Z

I'm in favour of simplifying this, certainly. A few comments:

autoconf tweaks #8639 includes the ability to run make clean without configuration, but it's also (on purpose) meant to be the case that it cleans artefacts with different extensions. I quite regularly work in a single tree which switches between mingw-w64 (.o & .exe), msvc (.obj * .exe) and WSL (.o & no extension). git clean -dfX means I have to go through a slow reconfiguration stage and for build system hacking I quite like to be able to check what files are present (I use an alias git show-dirty which lists all files which git is ignoring through .gitignore). So I like make distclean to work without having to run configure first (although I can use git clean -dfX then as easily) but I also really like make clean to remove all the build artefacts, not just the ones currently configured.
From that, please could the clean targets here always delete foo and foo.exe, regardless of $(EXE) (this is why autoconf tweaks #8639 systematically expanded calls to $(A), etc. and is what was done for ocamlnat). That could be done relatively cleanly of course with CLEAN_PROGRAM macro so that $call CLEAN_PROGRAM,ocamldoc) expands to rm -f ocamldoc ocamldoc.exe ocamldoc.opt ocamldoc.opt.exe
I think Bytelink.fix_exec_name is an extremely old bug - that's the only place where the compiler forces the addition of .exe. I think that the fix really ought to be to compile the custom runtime there with .exe and then rename it after compilation back to the name requested with -o (or something like that), but it's obviously a corner case. In general, the drivers should never add .exe.
I don't think there's a problem introducing a dependency on Config for ocamlmktop is there? As with ocamlmklib, isn't it just the case that it's a tool which started life as a script (?!) and got converted/written in OCaml long before the compiler-libs abstraction existed? I would for now advise against adding Sys.ext_exe at this stage - that's confusing for cross-compilation.
Agreed that read_cmt could be built with the correct name

It's one thing having objects (.obj and .lib) floating around which might not be relevant, but it feels like a development irritation to end up with ocamlrun and ocamlrun.exe in the tree which are actually from entirely different builds?

A possibility which would simplify the cleaning as well. We already have near full machinery to install the files with .exe added... that could be inverted? Dune intentionally chose to build everything internally with .exe to stop having to worry about the difference and then removes the .exe when installing. This has the benefit that you'd never have a real ocamlopt (say from a WSL build) which is then ignored because of the .PHONY. Against that change is the fact that now on Linux if developing you have to run boot/ocamlrun.exe ./ocamlc which is weird and annoying. That could be fixed by symlinks, I guess.

shindere · 2020-06-10T17:21:38Z

David Allsopp (2020/06/09 06:37 -0700):

I'm in favour of simplifying this, certainly. A few comments: - #8639 includes the ability to run `make clean` without configuration, but it's also (on purpose) meant to be the case that it cleans artefacts with different extensions. I quite regularly work in a single tree which switches between mingw-w64 (.o & .exe), msvc (.obj * .exe) and WSL (.o & no extension). `git clean -dfX` means I have to go through a slow reconfiguration stage and for build system hacking I quite like to be able to check what files are present (I use an alias `git show-dirty` which lists all files which git is ignoring through .gitignore). So I like `make distclean` to work without having to run `configure` first (although I can use `git clean -dfX` then as easily) but I also really like `make clean` to remove all the build artefacts, not just the ones currently configured.

I have updated the PR to do that and also removed from it the commit that took the configuration into account, if it did exist.

- From that, please could the `clean` targets here _always_ delete `foo` and `foo.exe`, regardless of `$(EXE)` (this is why #8639 systematically expanded calls to `$(A)`, etc. and is what was done for `ocamlnat`). That could be done relatively cleanly of course with `CLEAN_PROGRAM` macro so that `$call CLEAN_PROGRAM,ocamldoc)` expands to `rm -f ocamldoc ocamldoc.exe ocamldoc.opt ocamldoc.opt.exe`

It seemed okay to do it manually, actually.

- I think `Bytelink.fix_exec_name` is an extremely old bug - that's the only place where the compiler forces the addition of `.exe`. I _think_ that the fix really ought to be to compile the custom runtime there with `.exe` and then rename it after compilation back to the name requested with `-o` (or something like that), but it's obviously a corner case. In general, the drivers should never add `.exe`.

I think I'll leave that for later (and perhaps for somebody else) because it's not directly related to the issue this PR wants to fix, I think.

- I don't think there's a problem introducing a dependency on `Config` for ocamlmktop is there? As with `ocamlmklib`, isn't it just the case that it's a tool which started life as a script (?!) and got converted/written in OCaml long before the compiler-libs abstraction existed? I would for now advise against adding `Sys.ext_exe` at this stage - that's confusing for cross-compilation.

I implemented the change in ocamlmktop.

- Agreed that `read_cmt` could be built with the correct name

I did that. One last thing that could be done here: two files in tools/ are kept at clean time: ocamldep and cvt_emit. When cleaning, they get renamed with a `.bak` extension rather than deleted because they are considered "precious". Given that `boot/ocamlc` now has the ability to compute dependencies, I don't think tools/ocamldep is still so precious that make clean can't delete it. Is it? I'm less sure about cvt_emit but my understanding is that it is used to build the native compiler and can then always be rebuilt rather easily. So how aobut getting rid of the "precious" thing and just removing ocamldep and cvt_emit like any other file when cleaning?

shindere · 2020-06-11T07:29:49Z

Actually, how about going one step further and building e.g. ocamlc.byet
rather then just ocamlc since the .byte suffix will be added at install
time anyway?

Would somebody disagree with this?

If not I'd happily integrate it to the present PR.

Please, let me know your opinions whatever they are.

#FeedbackWanted

gasche · 2020-06-11T08:17:29Z

i'm not excited. For .exe, there was a technical argument to do the change, which was .exe binaries are nicer for Windows user outside a Cygwin environment. For .byte there is no such argument, so you would be breaking developer's habits without a clear justification.

damiendoligez

Looks good. Just a few remarks and questions.

damiendoligez · 2020-06-16T13:34:51Z

Makefile

 	$(MAKE) coreall

 # Check if fixpoint reached
+
+CMPBYT := $(CAMLRUN) tools/cmpbyt


I'd expect this one to get a $(EXE) too...

damiendoligez · 2020-06-16T13:39:00Z

Makefile

+	($(MAKE) OCAML_FLEXLINK="../boot/ocamlrun$(EXE) \
+	  ./flexlink" MSVC_DETECT=0 \


You have cut the line in a very strange place.

Suggested change

($(MAKE) OCAML_FLEXLINK="../boot/ocamlrun$(EXE) \

./flexlink" MSVC_DETECT=0 \

($(MAKE) OCAML_FLEXLINK="../boot/ocamlrun$(EXE) ./flexlink" \

MSVC_DETECT=0 \

damiendoligez · 2020-06-16T13:50:03Z

Makefile.common

@@ -117,3 +117,12 @@ endif

 $(DEPDIR):
 	$(MKDIR) $@
+
+# When exeecutable files have an extension (e.g. ".exe"),


Suggested change

# When exeecutable files have an extension (e.g. ".exe"),

# When executable files have an extension (e.g. ".exe"),

damiendoligez · 2020-06-16T13:55:55Z

ocamltest/Makefile

@@ -42,6 +42,8 @@ else
  endif
 endif

+exe := $(EXE)


You define this variable but AFAICT you never use it.

damiendoligez · 2020-06-16T14:01:15Z

tools/Makefile


 # ocamldep is precious: sometimes we are stuck in the middle of a
 # bootstrap and we need to remake the dependencies
 clean::
-	if test -f ocamldep; then mv -f ocamldep ocamldep.bak; else :; fi
-	rm -f ocamldep.opt
+	if test -f ocamldep.exe; then \


I agree we can get rid of this special case for ocamldep: it's not used by the build system anymore, so we won't need it while bootstrapping.

damiendoligez · 2020-06-16T14:02:24Z

tools/Makefile

+$(eval $(call PROGRAM_SYNONYM,cvt_emit))
+
+$(cvt_emit): cvt_emit.cmo
+	$(CAMLC) $(LINKFLAGS) -o $@ $<


Suggested change

$(CAMLC) $(LINKFLAGS) -o $@ $<

$(CAMLC) $(LINKFLAGS) -o $@ $^

damiendoligez · 2020-06-16T14:09:14Z

tools/Makefile

-	$(ROOTDIR)/otherlibs/str/str.cma \
-	$(ROOTDIR)/otherlibs/$(UNIXLIB)/unix.cma \
-	caml_tex.ml
+caml_tex_objects := \


Why rename this macro? These are libraries and a source file, none of them are object files...

damiendoligez

Travis is not happy because:

check-typo complains
the dependencies for ocamlcmt have moved inside the .depend file (because ocamlcmt is not at the same place as read_cmt in alphabetical order)
the Changes entry is missing

I think we can ignore the last two.

damiendoligez · 2020-06-16T14:14:20Z

tools/ocamlmktop.ml

-  let ocamlc,extra_quote =
-    if Sys.win32 then "ocamlc.exe","\"" else "ocamlc",""
-  in
+  let ocamlc = "ocamlc" ^ Config.ext_exe in 


check-typo is not happy...

Suggested change

let ocamlc = "ocamlc" ^ Config.ext_exe in

let ocamlc = "ocamlc" ^ Config.ext_exe in

shindere · 2020-06-17T07:31:54Z

Thanks a lot for the careful review. Damien Doligez (2020/06/16 07:11 -0700):

@damiendoligez approved this pull request. Looks good. Just a few remarks and questions. > $(MAKE) coreall # Check if fixpoint reached + +CMPBYT := $(CAMLRUN) tools/cmpbyt I'd expect this one to get a `$(EXE)` too...

You are correct. This has now been fixed.

> + ($(MAKE) OCAML_FLEXLINK="../boot/ocamlrun$(EXE) \ + ./flexlink" MSVC_DETECT=0 \ You have cut the line in a very strange place. ```suggestion ($(MAKE) OCAML_FLEXLINK="../boot/ocamlrun$(EXE) ./flexlink" \ MSVC_DETECT=0 \ ```

Ah indeed. Fixed. thanks.

> @@ -117,3 +117,12 @@ endif $(DEPDIR): $(MKDIR) $@ + +# When exeecutable files have an extension (e.g. ".exe"), ```suggestion # When executable files have an extension (e.g. ".exe"), ```

Fixed too. thanks.

> @@ -42,6 +42,8 @@ else endif endif +exe := $(EXE) You define this variable but AFAICT you never use it.

You are right, thanks. THe useless definition has been removed.

# ocamldep is precious: sometimes we are stuck in the middle of a # bootstrap and we need to remake the dependencies clean:: - if test -f ocamldep; then mv -f ocamldep ocamldep.bak; else :; fi - rm -f ocamldep.opt + if test -f ocamldep.exe; then \ I agree we can get rid of this special case for `ocamldep`: it's not used by the build system anymore, so we won't need it while bootstrapping.

I have stopped removing it to `ocamldep.bak`.

-cvt_emit: $(CVT_EMIT) - $(CAMLC) $(LINKFLAGS) -o cvt_emit $(CVT_EMIT) +$(eval $(call PROGRAM_SYNONYM,cvt_emit)) + +$(cvt_emit): cvt_emit.cmo + $(CAMLC) $(LINKFLAGS) -o $@ $< ```suggestion $(CAMLC) $(LINKFLAGS) -o $@ $^ ```

Okay, done. Although the previous code was not wrong. It's more robust that way, you are right.

> @@ -340,28 +356,31 @@ CMPBYT=$(ROOTDIR)/compilerlibs/ocamlcommon.cma \ $(call byte_and_opt,cmpbyt,$(CMPBYT),) -CAMLTEX= $(ROOTDIR)/compilerlibs/ocamlcommon.cma \ - $(ROOTDIR)/compilerlibs/ocamlbytecomp.cma \ - $(ROOTDIR)/compilerlibs/ocamltoplevel.cma \ - $(ROOTDIR)/otherlibs/str/str.cma \ - $(ROOTDIR)/otherlibs/$(UNIXLIB)/unix.cma \ - caml_tex.ml +caml_tex_objects := \ Why rename this macro?

I renamed the _variable_ because I wanted to use `camltex` for the name of the program and it felt odd to have both `camltex` and `CAMLTEX` around.

These are libraries and a source file, none of them are object files...

I renamed the variable to `camltex_files`.

shindere · 2020-06-17T07:44:21Z

I removed the trailing whitespace that displeased check-typo, sorry about that.

shindere · 2020-06-17T09:40:43Z

@xavierleroy: @damiendoligez has advised to ask you about cvt_emit being
precious, see the comment in tools/Makefile.

In my understanding, the tool is used to compile the native compiler and
should thus have nothing to do with bootstrap. It seems to me it could
hence simply be cleaned rather than renamed. Can you confirm?

shindere · 2020-06-17T09:55:53Z

tools/.depend has now been fixed, so Travis should stop complaining about
that.

xavierleroy · 2020-06-17T16:08:15Z

@damiendoligez has advised to ask you about cvt_emit being precious, see the comment in tools/Makefile.

I thought that was Damien's obsession :-) Just like it says in the comment: before you can analyze dependencies on the OCaml code, all generated .ml and .mli files must be generated. cvt_emit is the generator for asmcomp/emit.ml. So, to rebuild dependencies you need to be able to rebuild a working cvt_emit or have one already.

I think Damien was worried about having to rebuild the dependencies in an inconsistent-bootstrap state where cvt_emit cannot be compiled.

Personally I'm not too worried. If I get into such an inconsistent state, maybe restarting from a consistent state is better.

damiendoligez · 2020-06-18T08:28:23Z

Nowadays I would just checkout OCaml in another directory and build cvt_emit there, so it doesn't matter anymore, I think we can remove it.

Use argv[0] rather than hardcoding "objinfo_helper". That way the message is accurate whether there is a ".exe" extension or not.

…fo_helper

Before this commit, the name of the compiler to use (ocamlc or ocamlc.exe) was determined base on the OS type. This commit replaces this by a mere string concatenation of "ocamlc" and the configured extension for executable files. This introduces a dependency of tools/ocamlmktop on the compiler's configuration module.

Commit 13786d7 accidentally removed a file twice.

The `core` target can depend on `coldstart` rather than invoking `$(MAKE) coldstart` as its first command.

Use the EXE build variable rather than OS type to determine the extension of executable files

This commit touches neither boot/ocamlc nor boot/ocamllex It has the side-effect of fixing the cleanup rules which did not use the $(EXE) extension when removing a file although it was produced with the $(EXE) extension.

Now that programs are built with their $(EXE) suffix, their installation rules can be simplified a bit because most of the programs get installed under the name they have been built with.

shindere · 2020-06-18T09:22:28Z

Damien Doligez (2020/06/18 01:28 -0700):

Nowadays I would just checkout OCaml in another directory and build cvt_emit there, so it doesn't matter anymore, I think we can remove it.

thanks. I amended the PR to also stop considering cvt_emit as precious. CI was okay before this but I will still wait until it passes again and then I will merge. Thanks a lot to the reviewers for their time!

shindere force-pushed the build-programs-with-exe-extension branch 3 times, most recently from 15cc65a to f361fe4 Compare June 9, 2020 07:40

shindere mentioned this pull request Jun 9, 2020

Clean without deps: reworking CAN_BE_UNCONFIGURED #9527

Merged

shindere force-pushed the build-programs-with-exe-extension branch from f361fe4 to 5c1c4db Compare June 9, 2020 10:57

shindere force-pushed the build-programs-with-exe-extension branch 2 times, most recently from 377c281 to 89ac3bb Compare June 10, 2020 17:10

shindere force-pushed the build-programs-with-exe-extension branch from 89ac3bb to 1ed4709 Compare June 16, 2020 07:03

damiendoligez approved these changes Jun 16, 2020

View reviewed changes

damiendoligez reviewed Jun 16, 2020

View reviewed changes

damiendoligez added the no-change-entry-needed label Jun 16, 2020

shindere force-pushed the build-programs-with-exe-extension branch from 1ed4709 to 9c6675f Compare June 17, 2020 07:44

shindere force-pushed the build-programs-with-exe-extension branch from 9c6675f to 5dc4aba Compare June 17, 2020 09:54

shindere added 7 commits June 18, 2020 11:07

objinfo_helper: slightly generalise usage message

92331f8

Use argv[0] rather than hardcoding "objinfo_helper". That way the message is accurate whether there is a ".exe" extension or not.

Make ext_exe public in the configuration module of the compiler

16006d5

objinfo: take possible .exe extension into account when calling objin…

758ab7d

…fo_helper

Fix distclean rule

d9dc10e

Commit 13786d7 accidentally removed a file twice.

Makefile: introduce the CMPBYT variable

1c158f4

Makefile: avoid one recursive invocation of make

2ee6e82

The `core` target can depend on `coldstart` rather than invoking `$(MAKE) coldstart` as its first command.

shindere added 5 commits June 18, 2020 11:07

lex/Makefile: simplification

b7d8a97

ocamltest: improve the way executable file extension is determined

a50d177

Use the EXE build variable rather than OS type to determine the extension of executable files

tools/Makefile: stop considering ocamldep and cvt_emit as precious

41e4dc1

Build system: simplify installation rules

8087f7f

Now that programs are built with their $(EXE) suffix, their installation rules can be simplified a bit because most of the programs get installed under the name they have been built with.

shindere force-pushed the build-programs-with-exe-extension branch from 5dc4aba to 8087f7f Compare June 18, 2020 09:19

shindere merged commit 82f77b8 into ocaml:trunk Jun 18, 2020

shindere deleted the build-programs-with-exe-extension branch June 18, 2020 10:57

shindere mentioned this pull request Jun 22, 2020

Build compiler distribution tools with $(EXE) extension from the start #7312

Closed

dra27 mentioned this pull request Sep 12, 2020

Fix unnecessary rebuilding on Windows #9908

Merged

dra27 mentioned this pull request Mar 9, 2021

make partialclean remove files from boot #10267

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build programs with the $(EXE) extension #9652

Build programs with the $(EXE) extension #9652

shindere commented Jun 7, 2020 •

edited by damiendoligez

dra27 commented Jun 9, 2020

shindere commented Jun 10, 2020 via email

shindere commented Jun 11, 2020

gasche commented Jun 11, 2020 •

edited

damiendoligez left a comment

damiendoligez Jun 16, 2020

damiendoligez Jun 16, 2020

damiendoligez Jun 16, 2020

damiendoligez Jun 16, 2020

damiendoligez Jun 16, 2020

damiendoligez Jun 16, 2020

damiendoligez Jun 16, 2020

damiendoligez left a comment

damiendoligez Jun 16, 2020

shindere commented Jun 17, 2020 via email

shindere commented Jun 17, 2020 via email

shindere commented Jun 17, 2020

shindere commented Jun 17, 2020

xavierleroy commented Jun 17, 2020

damiendoligez commented Jun 18, 2020

shindere commented Jun 18, 2020 via email

		($(MAKE) OCAML_FLEXLINK="../boot/ocamlrun$(EXE) \
		./flexlink" MSVC_DETECT=0 \

	# When exeecutable files have an extension (e.g. ".exe"),
	# When executable files have an extension (e.g. ".exe"),

	$(CAMLC) $(LINKFLAGS) -o $@ $<
	$(CAMLC) $(LINKFLAGS) -o $@ $^

	let ocamlc = "ocamlc" ^ Config.ext_exe in
	let ocamlc = "ocamlc" ^ Config.ext_exe in

Build programs with the $(EXE) extension #9652

Build programs with the $(EXE) extension #9652

Conversation

shindere commented Jun 7, 2020 • edited by damiendoligez

dra27 commented Jun 9, 2020

shindere commented Jun 10, 2020 via email

shindere commented Jun 11, 2020

gasche commented Jun 11, 2020 • edited

damiendoligez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiendoligez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shindere commented Jun 17, 2020 via email

shindere commented Jun 17, 2020 via email

shindere commented Jun 17, 2020

shindere commented Jun 17, 2020

xavierleroy commented Jun 17, 2020

damiendoligez commented Jun 18, 2020

shindere commented Jun 18, 2020 via email

shindere commented Jun 7, 2020 •

edited by damiendoligez

gasche commented Jun 11, 2020 •

edited