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.

Sign up for GitHub

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

Jump to bottom

Compile non-speed-critical tools to bytecode only #11993

Merged
merged 3 commits into from Feb 22, 2023
Merged

Compile non-speed-critical tools to bytecode only #11993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c9c0af7
Compile non-speed-critical tools to bytecode only
xavierleroy Feb 3, 2023
738e33a
Build and install ocamlobjinfo in native code, whenever possible
xavierleroy Feb 18, 2023
64c6ccf
Changes for #11993
xavierleroy Feb 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions Changes
View file
Open in desktop
Expand Up @@ -300,6 +300,13 @@ Working version
parameters.
(Florian Angeletti, report by Wiktor Kuchta, review by Jules Aguillon)

* #11993: install only bytecode executables for the `ocamlmklib`, `ocamlcmt`,
`ocamlprof`, `ocamlcp`, `ocamloptp`, and `ocamlmktop` tools, but no
native-code executables. A tool like `ocamlmklib` for example is now
installed directly to `$BINDIR/ocamlmklib`; `ocamlmklib.byte` and
`ocamlmklib.opt` are no longer installed to `$BINDIR`.
(Xavier Leroy, review by Gabriel Scherer)

### Manual and documentation:

- #9430, #11291: Document the general desugaring rules for binding operators.
Expand Down
41 changes: 23 additions & 18 deletions Makefile
View file
Open in desktop
Expand Up @@ -109,11 +109,15 @@ ocamllex_PROGRAMS = $(addprefix lex/,ocamllex ocamllex.opt)

ocamlyacc_PROGRAM = yacc/ocamlyacc

TOOLS_TO_INSTALL = \
ocamldep ocamlprof ocamlcp ocamlmklib ocamlmktop ocamlobjinfo
# Tools to be compiled to native and bytecode, then installed
TOOLS_TO_INSTALL_NAT = ocamldep ocamlobjinfo

# Tools to be compiled to bytecode only, then installed
TOOLS_TO_INSTALL_BYT = \
ocamlcmt ocamlprof ocamlcp ocamlmklib ocamlmktop

ifeq "$(NATIVE_COMPILER)" "true"
TOOLS_TO_INSTALL += ocamloptp
TOOLS_TO_INSTALL_BYT += ocamloptp
endif

# Clean should remove tools/ocamloptp etc. unconditionally because
Expand All @@ -122,9 +126,11 @@ endif
clean::
rm -f $(addprefix tools/ocamlopt,p p.opt p.exe p.opt.exe)

TOOLS = $(TOOLS_TO_INSTALL) ocamlcmt dumpobj primreq stripdebug cmpbyt
TOOLS_NAT = $(TOOLS_TO_INSTALL_NAT)
TOOLS_BYT = $(TOOLS_TO_INSTALL_BYT) dumpobj primreq stripdebug cmpbyt

TOOLS_PROGRAMS = $(addprefix tools/,$(TOOLS))
TOOLS_NAT_PROGRAMS = $(addprefix tools/,$(TOOLS_NAT))
TOOLS_BYT_PROGRAMS = $(addprefix tools/,$(TOOLS_BYT))

TOOLS_MODULES = tools/profiling

Expand All @@ -137,7 +143,7 @@ $(foreach PROGRAM, $(C_PROGRAMS),\

# OCaml programs that are compiled in both bytecode and native code

OCAML_PROGRAMS = ocamlc ocamlopt lex/ocamllex $(TOOLS_PROGRAMS)
OCAML_PROGRAMS = ocamlc ocamlopt lex/ocamllex $(TOOLS_NAT_PROGRAMS)

$(foreach PROGRAM, $(OCAML_PROGRAMS),\
$(eval $(call OCAML_PROGRAM,$(PROGRAM))))
Expand All @@ -149,7 +155,8 @@ $(foreach PROGRAM, $(OCAML_PROGRAMS),\
# We have to use dedicated rules to build it

OCAML_BYTECODE_PROGRAMS = expunge \
$(addprefix tools/,cvt_emit make_opcodes ocamltex)
$(TOOLS_BYT_PROGRAMS) \
$(addprefix tools/, cvt_emit make_opcodes ocamltex)

$(foreach PROGRAM, $(OCAML_BYTECODE_PROGRAMS),\
$(eval $(call OCAML_BYTECODE_PROGRAM,$(PROGRAM))))
Expand Down Expand Up @@ -1308,11 +1315,11 @@ lintapidiff: tools/lintapidiff.opt$(EXE)
# Tools

TOOLS_BYTECODE_TARGETS = \
$(filter-out tools/ocamloptp,$(TOOLS_PROGRAMS)) $(TOOLS_MODULES:=.cmo)
$(TOOLS_NAT_PROGRAMS) $(TOOLS_BYT_PROGRAMS) $(TOOLS_MODULES:=.cmo)

TOOLS_NATIVE_TARGETS = $(TOOLS_MODULES:=.cmx) tools/ocamloptp
TOOLS_NATIVE_TARGETS = $(TOOLS_MODULES:=.cmx)

TOOLS_OPT_TARGETS = $(TOOLS_PROGRAMS:=.opt)
TOOLS_OPT_TARGETS = $(TOOLS_NAT_PROGRAMS:=.opt)

.PHONY: ocamltools
ocamltools: ocamlc ocamllex
Expand Down Expand Up @@ -1617,7 +1624,7 @@ endif
ifeq "$(INSTALL_BYTECODE_PROGRAMS)" "true"
$(INSTALL_PROG) lex/ocamllex$(EXE) \
"$(INSTALL_BINDIR)/ocamllex.byte$(EXE)"
for i in $(TOOLS_TO_INSTALL); \
for i in $(TOOLS_TO_INSTALL_NAT); \
do \
$(INSTALL_PROG) "tools/$$i$(EXE)" "$(INSTALL_BINDIR)/$$i.byte$(EXE)";\
if test -f "tools/$$i".opt$(EXE); then \
Expand All @@ -1628,21 +1635,19 @@ ifeq "$(INSTALL_BYTECODE_PROGRAMS)" "true"
fi; \
done
else
for i in $(TOOLS_TO_INSTALL); \
for i in $(TOOLS_TO_INSTALL_NAT); \
do \
if test -f "tools/$$i".opt$(EXE); then \
$(INSTALL_PROG) "tools/$$i.opt$(EXE)" "$(INSTALL_BINDIR)"; \
(cd "$(INSTALL_BINDIR)" && $(LN) "$$i.opt$(EXE)" "$$i$(EXE)"); \
fi; \
done
endif
for i in $(TOOLS_TO_INSTALL_BYT); \
do \
$(INSTALL_PROG) "tools/$$i$(EXE)" "$(INSTALL_BINDIR)";\
done
Copy link
Member

@gasche gasche Feb 18, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when INSTALL_BYTECODE_PROGRAMS is true, the layout we get is that foo.byte and foo.opt are available, and foo is a symbolic link to foo.opt. For bytecode-only programs I would expect to have foo.byte available, and foo as a symbolic link to foo.byte.

If I understand correctly, this is not what this code does, it looks like it just installs foo and not foo.byte. Should we change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The .byte / .opt / symbolic link dance is intended to let users choose between the two implementations if they must, and pick the best implementation by default otherwise. If there's only one implementation, there's no choice to be made. Plus, there's @kit-ty-kate 's suggestion above to get rid of all this symbolic link nonsense, and I'm in favor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this PR, if users for some reason wanted to use a bytecode executable instead of its native counterpart, they would enable INSTALL_BYTECODE_PROGRAMS and then explicitly invoke foo.byte. Unless I am missing something, your change breaks this use-case by making foo.byte unavailable for some tools. (But then: maybe no one was doing that, and especially for those tools nobody cares about in practice.)

Independently of this backward-compatibility aspect, I don't see the problem with having a symlink. (My Linux box is full of symlinks all over, typically many of the /usr/lib/*.so files are symlinks.) It arguably even adds discoverability, if I do ls -l .../bin/ocamlc I can immediately tell that I have the native version on my system.

I also didn't interpret Kate's comment #11993 (comment) to be complaining about symlinks, I read it as suggesting that we install at most one version of each tool, with the absence of symlinks a side-effect of that (or in fact we could keep symlinks if we really wanted, but I agree that in this case it makes less sense).

To summarize: my opinion is still that having the symlinks would be nicer, and I think that they help with compatibility in INSTALL_BYTECODE_PROGRAMS mode. It would probably be okay (not my preference but okay) to do without the links outside the INSTALL_BYTECODE_PROGRAMS mode. Then we could discuss (in a separate PR?) disabling this mode by default, and we would get closer to your ideal world (with a configure option still working for .byte aficionados).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced by your argument. For these rarely-used, non-performance-critical tools, I just want to go back to what we did in OCaml 4.02 and earlier. namely compilation to bytecode only and installation under the final name (no symlink to .byte). So, we will agree to disagree here and try to move on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, if you insist, that works for me.

$(INSTALL_PROG) $(ocamlyacc_PROGRAM)$(EXE) "$(INSTALL_BINDIR)"
if test -f tools/ocamlcmt.opt$(EXE); then \
$(INSTALL_PROG)\
tools/ocamlcmt.opt$(EXE) "$(INSTALL_BINDIR)/ocamlcmt$(EXE)"; \
else \
$(INSTALL_PROG) tools/ocamlcmt$(EXE) "$(INSTALL_BINDIR)"; \
fi
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(removing this ad-hoc logic is a nice simplification)

$(INSTALL_DATA) \
utils/*.cmi \
parsing/*.cmi \
Expand Down