Native by default by DemiMarie · Pull Request #512 · ocaml/ocaml

DemiMarie · 2016-03-16T23:10:51Z

Currently the "obvious" choice for the OCaml tools are ocamllex,
ocamlc, and ocamlopt. However, there is no good reason to use them
when ocamllex.opt, ocamlc.opt, and ocamlopt.opt are available.

Therefore, install the native-code versions with no suffixes when
they are present. The bytecode versions are now available with
the .byte suffix.

dbuenzli · 2016-03-16T23:27:51Z

xref MPR 7014.

gasche · 2016-03-17T01:20:32Z

Just to clarify: the proposed change does what one would expect, that is install the bytecode tool as foo.byte, but install the native compiler in both foo and foo.opt (for backward-compatibility).

Naive question: Is it safe to use ln -s to install files? I wonder how Windows handles that for example. Looking at the existing makefiles, the uses where either

internals to the compiler build tree, or
for the target installopt-noprof that I don't anyone ever uses in practice, or
coming from a recent change

so we cannot assume that this has been tested before.

DemiMarie · 2016-03-17T02:08:29Z

Yes, the bytecode tool is now foo.byte. The native tool is now in both foo and foo.opt, if it has been built. If the native tool has not been built, the bytecode tool is installed as foo.

The idea is that running foo will automatically run a native-code tool if available, or a bytecode tool otherwise. This makes it easy to write scripts that invoke the optimum OCaml tool cross-platform.

I got rid of the use of ln -s on Windows

alainfrisch · 2016-03-17T08:12:48Z

If there is no reason to use the bytecode version, why do we install them at all?

mshinwell · 2016-03-17T10:07:22Z

I have at least two use cases for installing bytecode.

(By example.) I'm working on a version of the compiler that adds certain instrumentation for native code. This has to be enabled at compiler configuration time, since the compiler and build system do not currently support a scheme where selecting a compiler flag at runtime causes objects to be generated that are incompatible with objects generated without that flag. It would be lovely if this would work, but it requires real effort to make this work properly. All of the libraries (including the ones in otherlibs/, not just the stdlib) need to be built multiple times and there has to be a reasonable way of ensuring the correct ones are linked against when the user just writes e.g. "threads.cmxa" on the command line. Without this support, what actually happens is the compiler itself ends up being built with instrumentation, which in this case cannot then be easily disabled at runtime. This may pose problems both for runtime performance and memory consumption. In such a scenario it might be useful to be able to run the ocamlopt that executes on the bytecode interpreter, since it avoids the instrumentation.
Production of ocamlopt.opt involves significantly more effort on the part of the compiler than production of ocamlopt (especially in Flambda mode), and thus there is a higher chance of a miscompilation due to a bug. In the fairly unlikely but possible scenario this happens, it may be possible to use the bytecode ocamlopt instead as a temporary workaround.

I don't really see the small overhead incurred by installing the bytecode versions to be a problem, and would suggest erring on the side of caution in any case.

dbuenzli · 2016-03-17T10:32:15Z

Production of ocamlopt.opt involves significantly more effort on the part of the compiler than production of ocamlopt (especially in Flambda mode), and thus there is a higher chance of a miscompilation due to a bug. In the fairly unlikely but possible scenario this happens, it may be possible to use the bytecode ocamlopt instead as a temporary workaround.

This suggest that we rather want to be able to configure which compilers get installed. From a build system perspective I'd like to be able not to be concerned about .opt or .byte suffixes and simply invoke a tool without extensions. Whether this is a native or byte code version should not be of concern to the build system and the build system should be able to assume that the tool it invokes will be the best choice under the situation at hand.

DemiMarie · 2016-03-21T22:19:32Z

I have changed the makefile so that all of the installed tools that are built in the tools/ directory are now installed in both bytecode and (where available) native code.

Should I squash the commits? The history is 16 commits long and some of them involve trivial typos.

DemiMarie · 2016-04-30T18:47:19Z

@damiendoligez any thoughts?

gasche · 2016-04-30T19:11:29Z

tools/Makefile.shared

 include ../config/Makefile
-CAMLRUN ?= ../boot/ocamlrun
-CAMLYACC ?= ../boot/ocamlyacc
+ifndef CAMLRUN


$$$$ looks a bit scary. Could you maybe add a comment on what the subtle code below is doing and why it is necessary?

gasche · 2016-04-30T20:11:07Z

tools/primreq.ml

 let scan_obj filename =
  let ic = open_in_bin filename in
-  let buffer = String.create (String.length cmo_magic_number) in
+  let buffer = Bytes.create (String.length cmo_magic_number) in


You should replace this and the following really_input by a single call to really_input_string, which allows to avoid the unsafe coercions below. If you do that and rebase the changes to this file in a single commit at the bottom of your commit stack (close to master), I'll push it to master now.

Can you explain how to do this? This is really hard in git.

@drbo There are some good tutorials online about git rebase --interactive. The high level explanation is that since your branch and the branch you want to merge into have both evolved independently, there'll be conflicts and the merge is guaranteed to be messy. By doing a rebase, git reapplies each of the commits in your branch on top of the branch to merge into, so you get to deal with conflicts one at a time and even do some cleanup like merging some of your commits together. Then once that's done you can open a Pull Request and it's easier to review and the merge is clean and painless. I can help you with this if you have Skype or Hangouts.

So here git rebase --interactive master is the command to start from (after you have pushed a last patch implementing the change in this file that you want to merge with the current diff). Feel free to accept @SGrondin's offer for additional explanations, otherwise just add a patch and I can rebase it on my side.

DemiMarie · 2016-05-03T01:03:45Z

Testsuite failure is same as trunk.

gasche · 2016-05-03T02:20:45Z

I pushed the really_input_string change in tools/primreq.

damiendoligez · 2016-05-04T08:40:17Z

Makefile

-INSTALL_MANDIR=$(DESTDIR)$(MANDIR)
+override shellquote = ""'''$(subst ','\'',$1)'#)'
+
+INSTALL_BINDIR:=$(call shellquote,$(DESTDIR)$(BINDIR))


Why do we need to shellquote these variables and not directly DESTDIR, BINDIR, etc?
Why should it be done in the Makefile rather than the configure script?

Address review comments and clean up quoting Fix inconsistent makefile quoting and testsuite bustage Fix includes of makefiles Fix configure bug make travis rerun Trying to debug travis issue Try to fix configure on Travis Add config.mk which I had previously forgotten to commit. Fix silly makefile bug Fix bug causing STUBLIBDIR to be empty Fix makefile quoting bugs Fix MSVC build break

damiendoligez · 2016-05-13T12:18:23Z

configure

+   }
+}'
+
+configure_options="$(awk -- "$awk_script" "$@" </dev/null)" || exit 1


Looks like you're trying to quote the arguments, but:

Your script doesn't work (try an argument with a quote on a system with gawk 4.1.1).

We don't want to depend on awk.

It's pointless anyway because configure_options is only used for debugging output (modulo your addition of the make configure target, a lot of complexity for little benefit IMO).

FWIW, here's my own implementation of a quoting function, without awk (using sed, which we already depend on):

quote1(){ printf "'%s'" "`printf %s "$1" | sed -e "s/'/'\\\\\\\\''/g"`"; } quote(){ while : ; do case $# in 0) break;; esac quote1 "$1" shift case $# in 0) ;; *) printf " ";; esac done }

usage: quote1 "$1" or quote "$@"

damiendoligez · 2016-05-13T13:06:40Z

There are too many changes in this PR that are not related to installing the native tools. You need to concentrate on the task at hand and leave other changes for other PRs.

DemiMarie · 2016-05-13T19:11:01Z

Can anyone help me figure out quoting on the Windows port?

DemiMarie · 2016-05-13T22:57:30Z

@damiendoligez would it be okay if I closed this PR and opened a new one with only the relevant changes included?

Previously, `ocamlc`, `ocamlopt`, `ocamllex`, and `ocamldep` defaulted to the bytecode versions of the tools. However, there is normally no advantage to the bytecode versions if the native-code versions are available, except in unusual circumstances. Instead, install the native-code versions without the `.opt` suffix. They are still installed with the `.opt` suffix for backwards compatibility. Finally, install the bytecode versions with the `.byte` suffix, and build native-code versions of tools that are not currently built in native code. Also, remove some duplication in tools/Makefile.shared. Supersedes GPR ocaml#512.

DemiMarie · 2016-05-16T19:18:08Z

Superseded by #587.

Previously, `ocamlc`, `ocamlopt`, `ocamllex`, and `ocamldep` defaulted to the bytecode versions of the tools. However, there is normally no advantage to the bytecode versions if the native-code versions are available, except in unusual circumstances. Instead, install the native-code versions without the `.opt` suffix. They are still installed with the `.opt` suffix for backwards compatibility. Finally, install the bytecode versions with the `.byte` suffix, and build native-code versions of tools that are not currently built in native code. Also, remove some duplication in tools/Makefile.shared. Supersedes GPR ocaml#512.

shindere · 2016-05-19T08:26:20Z

I realise this answer comes too late for this PR but for the next ones I think it may have been better to just cleanup the branch, I don't think it really makes sense to close the PR itself.

Previously, `ocamlc`, `ocamlopt`, `ocamllex`, and `ocamldep` defaulted to the bytecode versions of the tools. However, there is normally no advantage to the bytecode versions if the native-code versions are available, except in unusual circumstances. Instead, install the native-code versions without the `.opt` suffix. They are still installed with the `.opt` suffix for backwards compatibility. Finally, install the bytecode versions with the `.byte` suffix, and build native-code versions of tools that are not currently built in native code. Also, remove some duplication in tools/Makefile.shared. Supersedes GPR ocaml#512.

DemiMarie force-pushed the native-by-default branch 2 times, most recently from 84d4386 to a34b668 Compare March 21, 2016 23:40

damiendoligez added this to the 4.04 milestone Mar 31, 2016

gasche reviewed Apr 30, 2016
View reviewed changes

DemiMarie force-pushed the native-by-default branch from 8258420 to b53be9e Compare April 30, 2016 19:38

gasche reviewed Apr 30, 2016
View reviewed changes

DemiMarie force-pushed the native-by-default branch 2 times, most recently from c8eff40 to 5f9c9c8 Compare May 2, 2016 23:35

DemiMarie force-pushed the native-by-default branch from 4c472aa to 48b4451 Compare May 3, 2016 15:00

damiendoligez reviewed May 4, 2016
View reviewed changes

DemiMarie added 3 commits May 12, 2016 19:41

More documentation of tools/Makefile.shared

e187bf8

Update .depend and add -compat-32 to ocamlc calls

2f81713

DemiMarie force-pushed the native-by-default branch from 52a1eed to a778e49 Compare May 12, 2016 23:58

Fix AppVeyor build and Unix install when there is whitespace in paths.

109576e

damiendoligez reviewed May 13, 2016
View reviewed changes

Another attempt at fixing quoting on the Windows port

51edc92

DemiMarie added 2 commits May 15, 2016 13:45

Fix byterun/Makefile.nt quoting bug

0f6aaf2

Another attempt at a quoting fix

db72d0a

DemiMarie mentioned this pull request May 16, 2016

Install native versions of tools by default. #587

Merged

DemiMarie closed this May 16, 2016

gasche mentioned this pull request May 25, 2017

Menhir-generated parser #292

Merged

vicuna mentioned this pull request Mar 14, 2019

Is there any good reason to install both byte and native versions of the ocaml toolchain binaries ? #7014

Closed

mshinwell added a commit to mshinwell/ocaml that referenced this pull request Jun 30, 2021

Targetint_32_64 (ocaml#512)

fdc4d62

stedolan pushed a commit to stedolan/ocaml that referenced this pull request May 24, 2022

Remove bad assertion (ocaml#512)

cc214da

Conversation

DemiMarie commented Mar 16, 2016

Uh oh!

dbuenzli commented Mar 16, 2016

Uh oh!

gasche commented Mar 17, 2016

Uh oh!

DemiMarie commented Mar 17, 2016

Uh oh!

alainfrisch commented Mar 17, 2016

Uh oh!

mshinwell commented Mar 17, 2016

Uh oh!

dbuenzli commented Mar 17, 2016

Uh oh!

DemiMarie commented Mar 21, 2016

Uh oh!

DemiMarie commented Apr 30, 2016

Uh oh!

gasche Apr 30, 2016

Choose a reason for hiding this comment

Uh oh!

DemiMarie Apr 30, 2016

Choose a reason for hiding this comment

Uh oh!

gasche Apr 30, 2016

Choose a reason for hiding this comment

Uh oh!

DemiMarie May 1, 2016

Choose a reason for hiding this comment

Uh oh!

SGrondin May 1, 2016 • 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!

gasche May 1, 2016

Choose a reason for hiding this comment

Uh oh!

DemiMarie commented May 3, 2016

Uh oh!

gasche commented May 3, 2016

Uh oh!

damiendoligez May 4, 2016

Choose a reason for hiding this comment

Uh oh!

damiendoligez May 13, 2016

Choose a reason for hiding this comment

Uh oh!

damiendoligez commented May 13, 2016

Uh oh!

DemiMarie commented May 13, 2016

Uh oh!

DemiMarie commented May 13, 2016

Uh oh!

DemiMarie commented May 16, 2016

Uh oh!

shindere commented May 19, 2016 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

SGrondin May 1, 2016 •

edited

Loading