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 root directory #1033

Merged
merged 109 commits into from Feb 15, 2017

Conversation

Projects
None yet
5 participants
@shindere
Contributor

shindere commented Feb 8, 2017

Cc @xavierleroy @damiendoligez @alainfrisch @dra27 @adrien-n

This PR merges the two makefiles (Makefile and Makefile.nt) at the root
of the OCaml repository.

Although this PR tries to introduce a few easy improvements in addition to
the merge of build systems itself, there are many things that remain
to be improved and, given the current size of this PR (more than 100
commits), it is probably better to delay these remaining improvements until
after this PR has been merged.

In particular, although this PR may fix things that did not work before,
no particular effort has been done in this direction. More
efforts were invested in not breaking anything that was working
before and not introducing any regression. So, it may be a good idea,
while reviewing and making comments, to focus on ensuring that
nothing got accidentally broken, rather
than on what could be
improved from there, at least for a first pass.

Changes affecting both build systems

  • Introduce the BYTECODE_C_COMPILER and NATIVE_C_COMPILER make variables
    These variables represent the C compilers ocamlc and ocamlopt
    should use to compile a third-party C source file when no -cc
    command-line option has been specified.
  • Remove the PARTIALLD make variable since it is unused
  • Get rid of the ARCH_OCAMLOPT make variable
    (It is a synonymous of the ARCH variable and is not exported.)

Changes specific to the Unix build system

  • Stop removing -Werror from compiler options
    There was a hack to ensure -Werror will not be passed to the C compiler
    when called by ocamlc/ocamlopt to compile third-party C source files.
    Given that the flags used to compile third-party C source files
    have now been distinguished from those used to compile those C
    files that belong to the compiler distribution, this hack is no
    longer necessary.
  • Stop removing shared libraries during install
    This had been introduced in commit a49cf7b but does not seem useful
    any longer.
  • ocamldep is now called with the -slash command-line option as is done
    on Windows. This should not have any effect, though.
  • The coldstart rule was creating a symlink to ../byterun/caml in the
    stdlib directory. Stop doing that because it was not done under
    Windows and it seems not creating this symlink does not break anything.
  • The Unix build system has a base.opt rule that this PR currently preserves,
    but since this rule is not used anywhere and has no counterpart on
    the Windows build system, its usefulness probably needs to be confirmed.

Changes specific to the Windows build system

  • Stop installing Changes, README and LICENSE files
  • Take the configured value of the following variables into account when
    generating utils/config.ml from utils/config.mlp instead of using
    hard-coded values:
    SYSTHREAD_SUPPORT, ASM_CFI_SUPPORTED, LIBUNWIND_AVAILABLE,
    LIBUNWIND_LINK_FLAGS and CC_PROFILE. Also, define the
    WITH_FRAME_POINTERS in config/Makefile.m* and make use of this
    definition, instead of using a hard-coded value.
  • Make the install-compiler-sources target available
    It had been introduced by PR#827 but only on the Unix side
  • Fix the installopt rule to install:
    ** the .mli files from the middle_end, middle_end/base_types and
    asmcomp directories to $(INSTALL_COMPLIBDIR), as is done under Unix.
    ** ocamlc$(EXE) and ocamllex$(EXE) as copies of (or symlinks to)
    ocamlc.byte$(EXE) and ocamllex.byte$(EXE), when installopt is
    executed but installoptopt is not, as is done on Unix.
  • Install .mli files to the compilerlibs directory
    Before this PR, the mli files of the utils, parsing, typing,
    bytecomp, driver and toplevel directories were not installed to
    the compilerlibs directory on Windows as was done on Unix.
    This PR fixes this.
  • Install more topdirs-related files to INSTALL_LIBDIR
    Before this PR, only topdirs.cmi was installed to INSTALL_LIBDIR
    on Windows. This PR also installs topdirs.cmt, topdirs.cmti and
    topdirs.mli, as is done on Unix.
  • The manual-pregen and html_doc targets become available on Windows
    (they have not been tested yet, though.)
  • The checkstack target also becomes available on Winows
    (but checkstack itself does not work yet)
    The checkstack target has been made more verbose and the $(EXE)
    extension is now used in a more coherent way.
  • Add dependencies to the ocamllex and ocamllex.opt targets to match
    what is done on Unix. However the dependencies between ocamllex
    and ocamllex.opt do not seem very coherent either.
  • Make the otherlibraries target depend on ocamltools as is done on Unix
  • Make the ocamldebugger target depend on otherlibraries as is done on Unix
  • Since commit dd74659, the coreboot target on the Unix build
    system calls make promote with CAMLRUN=byterun/ocamlrun, which
    has never been the case on Windows. After this PR, coreboot passes
    CAMLRUN=byterun/ocamlrun to promote target on Windows, too.

The Unix and Windows versions of the core, opt, opt.opt and runtop
rules somehow differ from each other. Currently this differences have been
preserved, but all these rules should probably be unified, ultimately.

Any help in this area would be greatly appreciated.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 8, 2017

Contributor

Even before starting to test and review further - congratulations on getting to this stage, @shindere!

Mainly so we can be lazy when new commits come in, could you push a Changes entry just to keep Travis happy?

Contributor

dra27 commented Feb 8, 2017

Even before starting to test and review further - congratulations on getting to this stage, @shindere!

Mainly so we can be lazy when new commits come in, could you push a Changes entry just to keep Travis happy?

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 8, 2017

Contributor
Contributor

shindere commented Feb 8, 2017

@damiendoligez

Looks good.

Show outdated Hide outdated configure
echo "PARTIALLD=$partialld" >> Makefile
echo "PACKLD=\$(PARTIALLD) \$(NATIVECCLINKOPTS) -o " \
| sed -e 's/ $/\\ /' >> Makefile
echo "PACKLD=$partialld $nativecclinkopts -o\ " >> Makefile

This comment has been minimized.

@damiendoligez

damiendoligez Feb 13, 2017

Member

It would be cleaner to use a double-backslash here.

@damiendoligez

damiendoligez Feb 13, 2017

Member

It would be cleaner to use a double-backslash here.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 13, 2017

Contributor
Contributor

shindere commented Feb 13, 2017

@adrien-n

This comment has been minimized.

Show comment
Hide comment
@adrien-n

adrien-n Feb 13, 2017

Contributor

Sidenote: GNU make has the -C option which chdir() to its argument. This is useful to replace the "cd foo && make ..." pattern since it can avoid spawning a shell but while I know that pmake has that optimization (i.e. looking for shell characters in the recipes and only spawning a shell when some are found), I'm not sure that GNU make also has it. If it does, that could be an easy speed win on Windows. Obviously, that's something to look at only later on.

Contributor

adrien-n commented Feb 13, 2017

Sidenote: GNU make has the -C option which chdir() to its argument. This is useful to replace the "cd foo && make ..." pattern since it can avoid spawning a shell but while I know that pmake has that optimization (i.e. looking for shell characters in the recipes and only spawning a shell when some are found), I'm not sure that GNU make also has it. If it does, that could be an easy speed win on Windows. Obviously, that's something to look at only later on.

@adrien-n

I would actually have installed at least README.{adoc,win32} and LICENSE on every platform instead. Definitely not at the root of the installation but somewhere under share/ or an equivalent (this seems to be meant for the OCaml Windows distribution and I haven't used it enough to know the corresponding needs).

Unfortunately it seems share/ is completely unused in the compiler but I know of no distributor that would want to skip LICENSE. I'm pretty sure all distributions copy that file manually.

This is however better handled as part of another set of changes.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 13, 2017

Contributor
Contributor

shindere commented Feb 13, 2017

Show outdated Hide outdated Makefile.shared
@@ -269,8 +268,7 @@ utils/config.ml: utils/config.mlp config/Makefile
-e 's|%%TARGET%%|$(TARGET)|' \
-e 's|%%WITH_FRAME_POINTERS%%|$(WITH_FRAME_POINTERS)|' \
-e 's|%%WITH_PROFINFO%%|$(WITH_PROFINFO)|' \
-e 's|%%WITH_SPACETIME%%|$(WITH_SPACETIME)|' \
utils/config.mlp > utils/config.ml
-e 's|%%WITH_SPACETIME%%|$(WITH_SPACETIME)|' $< > $@

This comment has been minimized.

@adrien-n

adrien-n Feb 13, 2017

Contributor

I would prefer the "$< > $@" to be kept on a separate line (in order to make the addition of the WITH_SUPERDUPER_EVERYTHING option simpler).

@adrien-n

adrien-n Feb 13, 2017

Contributor

I would prefer the "$< > $@" to be kept on a separate line (in order to make the addition of the WITH_SUPERDUPER_EVERYTHING option simpler).

Show outdated Hide outdated Makefile.shared
ifeq "UNIX_OR_WIN32" "unix"
EXTRAPATH=
else
EXTRAPATH=PATH=otherlibs/win32unix:"$(PATH)"

This comment has been minimized.

@adrien-n

adrien-n Feb 13, 2017

Contributor

Can you add spaces around the first '=' ?

I would also prefer to have always have spaces around makes =, := and ?= but don't mind much. This line however is a bit difficult to parse. I would also prefer the quotes to be around the whole new value rather than only $(PATH).

@adrien-n

adrien-n Feb 13, 2017

Contributor

Can you add spaces around the first '=' ?

I would also prefer to have always have spaces around makes =, := and ?= but don't mind much. This line however is a bit difficult to parse. I would also prefer the quotes to be around the whole new value rather than only $(PATH).

Show outdated Hide outdated Makefile.shared
BOOT_FLEXLINK_CMD=
else
BOOT_FLEXLINK_CMD=FLEXLINK_CMD="../boot/ocamlrun ../flexdll/flexlink.exe"
CAMLOPT:=OCAML_FLEXLINK="boot/ocamlrun flexdll/flexlink.exe" $(CAMLOPT)

This comment has been minimized.

@adrien-n

adrien-n Feb 13, 2017

Contributor

Same as with the previous commit, I would prefer spaces around := .

@adrien-n

adrien-n Feb 13, 2017

Contributor

Same as with the previous commit, I would prefer spaces around := .

@adrien-n

For future reference, why are such files modified at install-time rather than during the build?

@adrien-n

I would install the manpages everywhere. While currently uncommon, it is possible to have a manpage reader with UNIX_OR_WIN32 = win32. This is probably already the case with msys* setups. Moreover it is simpler to remove them if they're not wanted in a specific distribution than it is to add them if they are.

Show outdated Hide outdated Makefile.nt
@@ -180,9 +180,11 @@ endif
if test -n "$(WITH_DEBUGGER)"; then \
$(MAKE) -C debugger install; \
fi
ifeq "$(UNIX_OR_WIN32)" "win32"
if test -n "$(FLEXDLL_SUBMODULE_PRESENT)"; then \

This comment has been minimized.

@adrien-n

adrien-n Feb 13, 2017

Contributor

I'm not fond of having half of the test as gnu make conditional and the other one as a shell one. Could you make everything either gnu make conditionals or shell conditionals?

@adrien-n

adrien-n Feb 13, 2017

Contributor

I'm not fond of having half of the test as gnu make conditional and the other one as a shell one. Could you make everything either gnu make conditionals or shell conditionals?

@adrien-n

Could this be simplified with a rule like

asmcomp/%.ml: asmcomp/$(ARCH)/%.ml
ln -s $(subst asmcomp/,,$<) $@

I don't remember if stems work well for such cases and I'm not going to test this tonight.

Such a change should also probably wait until after the current changes are merged.

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Feb 13, 2017

Contributor

I would actually have installed at least README.{adoc,win32} and LICENSE on every platform instead. Definitely not at the root of the installation but somewhere under share/ or an equivalent (this seems to be meant for the OCaml Windows distribution and I haven't used it enough to know the corresponding needs).

Just to support this further, doing so in a configurable directory would allow opam to install docs at the right place so that odig can find them. Reading these files for the end user then becomes an odig {changes,readme,license} ocaml invocation away.

Contributor

dbuenzli commented Feb 13, 2017

I would actually have installed at least README.{adoc,win32} and LICENSE on every platform instead. Definitely not at the root of the installation but somewhere under share/ or an equivalent (this seems to be meant for the OCaml Windows distribution and I haven't used it enough to know the corresponding needs).

Just to support this further, doing so in a configurable directory would allow opam to install docs at the right place so that odig can find them. Reading these files for the end user then becomes an odig {changes,readme,license} ocaml invocation away.

Show outdated Hide outdated Makefile.shared
core:
ifeq "$(UNIX_OR_WIN32)" "unix"
$(MAKE) coldstart
else # Windows, to be fixed!

This comment has been minimized.

@adrien-n

adrien-n Feb 13, 2017

Contributor

Any news about this? Is bootstrapping even supposed to be possible on Windows?

@adrien-n

adrien-n Feb 13, 2017

Contributor

Any news about this? Is bootstrapping even supposed to be possible on Windows?

@adrien-n

This comment has been minimized.

Show comment
Hide comment
@adrien-n

adrien-n Feb 13, 2017

Contributor

Well, thank you and congratulations for carrying this through! I have to admit that I had troubles remaining focused throughout the review. This makes the work involved all the more palpable.

Also, now I see that my remark about make -C should have waited a bit more because you changed many, if not all, calls already.

I've refrained from writing down a few comments that are really best handled later on, especially with the runtime/coreall/core/... rules because things should work the same, these things can wait a bit more and will also be easier to change later on (it should also be much easier to get everyone involved into simplifying such lines now rather than when the differences were hidden away in a windows-specific file).

In the same spirit, if GNU make does not spawn an intermediate shell when it's not needed, several simple but common patterns should bring vast improvements, but these will also be a better fit for another batch.

Moreover, I expect these changes to be conflict-friendly and adding more changes will only make conflicts more likely and fixing horrible so I'm in favor of merging quickly.

Contributor

adrien-n commented Feb 13, 2017

Well, thank you and congratulations for carrying this through! I have to admit that I had troubles remaining focused throughout the review. This makes the work involved all the more palpable.

Also, now I see that my remark about make -C should have waited a bit more because you changed many, if not all, calls already.

I've refrained from writing down a few comments that are really best handled later on, especially with the runtime/coreall/core/... rules because things should work the same, these things can wait a bit more and will also be easier to change later on (it should also be much easier to get everyone involved into simplifying such lines now rather than when the differences were hidden away in a windows-specific file).

In the same spirit, if GNU make does not spawn an intermediate shell when it's not needed, several simple but common patterns should bring vast improvements, but these will also be a better fit for another batch.

Moreover, I expect these changes to be conflict-friendly and adding more changes will only make conflicts more likely and fixing horrible so I'm in favor of merging quickly.

Show outdated Hide outdated Makefile
$(MAKE) clean
rm -f boot/ocamlrun boot/ocamlrun.exe boot/camlheader boot/ocamlyacc \
boot/*.cm* boot/libcamlrun.a
distclean: clean

This comment has been minimized.

@damiendoligez

damiendoligez Feb 14, 2017

Member

You forgot the .PHONY for distclean.

@damiendoligez

damiendoligez Feb 14, 2017

Member

You forgot the .PHONY for distclean.

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Feb 14, 2017

Member

@shindere

Did you have a look to the yet-unmerged rules, though?

I think they can wait for another PR.

Member

damiendoligez commented Feb 14, 2017

@shindere

Did you have a look to the yet-unmerged rules, though?

I think they can wait for another PR.

shindere added some commits Dec 21, 2016

When possible, call $(MAKE) rather than $(MAKEREC)
MAKEREC is a make variable that allows one to call make with the
'-f Makefile.nt' option.

When the main build system builds something in a subdirectory where
Makefile and Makefile.nt have been merged, it can now do that by using
$(MAKE) rather than $(MAKEREC).

This commit implements this change. For the root directory, where the
makefiles have not yet been merged, the calls to $(MAKEREC) have been
kept.
Define BYTERUN as a real make variable
This variable is used to give a value to standard_runtime in
utils/config.ml.

Before this commit, its values were hard-coded in Makefile and
Makefile.nt, in the rules generating utils/config.ml from utils/config.mlp.

This commit gets rid of this hardcoding, to prepare the sharing of the
rules mentionned above.
Bugfix in Makefile.nt: take SYSTHREAD_SUPPORT into account
This variable is defined in config/Makefile.m* but before this commit
its value was ignored while generating utils/config.ml, Makefile.nt was
using true. This commit modifies Makefile.nt to take this variable into
account rather than hardcoding a value.
Windows build system enhancements
The following variables were defined in the Windows configuration files
(config/Makefile.*) but not taken into account by Makefile.nt when
generating utils/config.ml from utils/config.mlp: ASM_CFI_SUPPORTED,
LIBUNWIND_AVAILABLE, LIBUNWIND_LINK_FLAGS and CC_PROFILE.

This commit replaces the values hardcoded in Makefile.nt by the
appropriate variable references.

Similarly, the value of WITH_FRAME_POINTERS was also hardcoded in
Makefile.nt but not defined in the configuration files. This commit thus
defines the variable in each configuration file and uses it instead of
the hardcoded value in Makefile.nt.
Introduce the BYTECODE_C_COMPILER and NATIVE_C_COMPILER make variables
These variables represent the C compilers ocamlc and ocamlopt should use
to compile a third-party C source file when no -cc command-line option
has been specified.

Thanks to these variables, the substitutions performed in Makefile and
Makefile.nt to generate utils/config.ml from utils/config.ml become
similar.

(The NATIVE_C_COMPILER variable is not really necessary but it has still
been introduced to preserve symetry.)
Makefile: stop removing -Werror from compiler options
This commit removes the line introduced by commit
59853fa

Since the proposal to have separate variables for warning
options has been implemented, such a precaution should no longer be
necessary. Moreover, this had been implemented for the Unix build system
only, not for the Windows one.
Makefiles: sed expressions cleanup in the recipes building utils/conf…
…ig.ml

The recipes to build utils/config.ml from utils/config.ml in Makefile
and Makefile.nt essentially are sed invocations with several

expressions. This commit sorts the expressions in alphabetical order in
both files and also makes sure all the expressions use the same
quoting style, namely single quotes.

This has no practical effect but makes the two recipes easier to
compare.
Makefile.nt: remove useless sed expressions
In the recipe that builds utils/config.ml from utils/config.mlp, the sed
expressions related to BINUTILS_NM and BINUTILS_OBJCOPY are useless
because these variables are not defined anywhere so this commit
removes them.
Makefiles: unify the way file extensions are generated
This commit deals with the rule that generates utils/config.ml
Add support for FLEXLINK_FLAGS to Makefile
This commit affects the recipe used to generate utils/config.ml
from utils/config.mlp. It adds an expression to the sed command to
replace the %%FLEXLINK_FLAGS%% token by a value. Given that this
variable is not defined for Unix build this will have no effect, except
making the recipe identical to the one used on the Windows build system.
Makefile.shared: simplify the recipe producing utils/config.ml
In particular, get rid of the rm command.
Move flexdll-related rules from Makefile.nt to Makefile.shared
This commit also gets rid of .PHONY declarations for the
flexdll-common-err and flexdll-repo targets which do not exist.
Move the install-compiler-sources target from Makefile to Makefile.sh…
…ared

This target has been introduced by GPR #827 but added only to the
Unix build system. This commit makes it available to the Windows build
system, too.
Makefile.shared: define the LN variable
On Windows this is cp, on Unix it is ln -sf.

shindere added some commits Jan 15, 2017

Makefiles: deduplicate .PHONY declarations of tools-related rules
The ocamltools, ocamltoolsopt and ocamltoolsopt.opt rules were already
shared but their .PHONY declarations were duplicated.
Makefiles: deduplicate the coreboot rule
Note: since commit dd74659, the
coreboot target on the Unix build system calls make promote with
CAMLRUN=byterun/ocamlrun, which the equivalent target on the Windows
system did not use before the present commit.

The present commit uses CAMLRUN=byterun/ocamlrun on both build systems.
Makefiles: deduplicate the all rule
Before this commit, the rule was more sequential on the Unix build system
than on the Windows one. The more sequential version has been kept
so as not to break parallel builds.
Makefiles: deduplicate the runtop rule
The recipes for the Unix and Windows build systems are different. This
difference has been preserved for the moment but the rules should be
unified.
Move the reconfigure rule from Makefile to Makefile.shared
Make sure it can be used only on Unix, though, since ./configure does
not work on Windows yet.
Move the base.opt recipe from Makefile to Makefile.shared
This target thus becomes available under Windows (Makefile.nt did not
define it).

Since no other rule makes use of this target, its usefulness needs to be
confirmed.
Makefiles: deduplicate the opt and opt.opt rules
The recipes work differently on Unix and Windows. This commit
preserves this situation but this will probably need to be clarified.
Get rid of the MAKEREC and MAKECMD make variables
Now that the Unix and Windows build systems have been merged, thiese
variables are no longer useful.
Move the content of Makefile.shared to Makefile
Also update Makefile.nt so as to include Makefile
Makefile: minor fix of the clean rule
Move the phony declaration above the first definition of the clean rule.
@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 15, 2017

Contributor
Contributor

shindere commented Feb 15, 2017

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 15, 2017

Contributor
Contributor

shindere commented Feb 15, 2017

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 15, 2017

Contributor
Contributor

shindere commented Feb 15, 2017

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 15, 2017

Contributor
Contributor

shindere commented Feb 15, 2017

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 15, 2017

Contributor
Contributor

shindere commented Feb 15, 2017

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 15, 2017

Contributor
Contributor

shindere commented Feb 15, 2017

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 15, 2017

Contributor
Contributor

shindere commented Feb 15, 2017

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 15, 2017

Contributor
Contributor

shindere commented Feb 15, 2017

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 15, 2017

Contributor
Contributor

shindere commented Feb 15, 2017

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Feb 15, 2017

Contributor
Contributor

shindere commented Feb 15, 2017

shindere added some commits Feb 15, 2017

Makefile: declare the distclean and coreboot targets as PHONY
Makefile: declare the coreboot target as PHONY

@damiendoligez damiendoligez merged commit 9265523 into ocaml:trunk Feb 15, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@shindere shindere referenced this pull request Feb 17, 2017

Closed

also install ocamlyacc #1048

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

Merge pull request #1033 from shindere/merge-root-makefiles
Merge Unix and Windows build systems in the root directory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment