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 build systems in the byterun directory #981

Merged
merged 24 commits into from Dec 29, 2016

Conversation

Projects
None yet
4 participants
@shindere
Contributor

shindere commented Dec 20, 2016

This time, the PR contains several commits showing how variables and
rules have progressively been merged into Makefile.shared, its content
being finally moved to Makefile.

The story may look a bit long (in terms of commits), but it should also
be simpler to review and comment.

Once the reviewing has been done, it will probably be better to squash
all the commits before merging into trunk, because although there has
been an effort to make the commits working and coherent, they have not
been tested individually, so that they may introduce problems while
bisecting, if merged as-is.

One special remark is that nothing has been improved in the way .depend
files are generated and used, because it is not the aim of this PR to
work on this, as discussed in PR #941, on which this PR depends, by the
way.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 20, 2016

Contributor

This PR is currently being tested with precheck.

Contributor

shindere commented Dec 20, 2016

This PR is currently being tested with precheck.

@shindere

This comment has been minimized.

Show comment
Hide comment
Contributor

shindere commented Dec 20, 2016

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 20, 2016

Contributor

Rebased on latest trunk

Contributor

shindere commented Dec 20, 2016

Rebased on latest trunk

@adrien-n

Just a quick note. My take on this is that we'll want to do a global review of what is installed and what might be missing a bit before we call cross-compilers fully supported. It should be simpler than noting down extra files as we see them.

Show outdated Hide outdated byterun/Makefile.nt
# It is imperative that there is no space after $(NAME_OBJ_FLAG)
%.$(DBGO): %.c
$(CC) $(DFLAGS) $(BYTECCDBGCOMPOPTS) -c $(NAME_OBJ_FLAG)$@ $<
$(CC) $(DFLAGS) -c $(NAME_OBJ_FLAG)$@ $<

This comment has been minimized.

@adrien-n

adrien-n Dec 25, 2016

Contributor

Is $(BYTECCDBGCOMPOPTS) the same as $(BYTECCCOMPOPTS)? If not, some difference has been introduced here.

@adrien-n

adrien-n Dec 25, 2016

Contributor

Is $(BYTECCDBGCOMPOPTS) the same as $(BYTECCCOMPOPTS)? If not, some difference has been introduced here.

Show outdated Hide outdated byterun/Makefile.shared
DFLAGS=$(CFLAGS) -DDEBUG
ifneq "$(CCOMPTYPE)" "msvc"

This comment has been minimized.

@adrien-n

adrien-n Dec 25, 2016

Contributor

As far as I can tell, this changes the behaviour of the non-msvc builds and fixes an issue where non-msvc debug builds didn't have "-g" in $(DFLAGS). Am I right?

@adrien-n

adrien-n Dec 25, 2016

Contributor

As far as I can tell, this changes the behaviour of the non-msvc builds and fixes an issue where non-msvc debug builds didn't have "-g" in $(DFLAGS). Am I right?

Show outdated Hide outdated byterun/Makefile.shared
$(CC) $(CFLAGS) -c $<
%.$(DBGO): %.c
$(CC) $(DFLAGS) -c $(OUTPUTOBJ)$@ $<

This comment has been minimized.

@adrien-n

adrien-n Dec 25, 2016

Contributor

I think the comment "It is imperative that there is no space after $(NAME_OBJ_FLAG)" was quite useful. However I've always found this brittle and I'm wondering whether a macro ( https://www.gnu.org/software/make/manual/make.html#Call-Function ) would be a better choice: it would only take $(OUTPUTOBJ) to be either "-Fo$(1)" or "-o $(1)" and use would be $(call OUTPUTOBJ,$@).

@adrien-n

adrien-n Dec 25, 2016

Contributor

I think the comment "It is imperative that there is no space after $(NAME_OBJ_FLAG)" was quite useful. However I've always found this brittle and I'm wondering whether a macro ( https://www.gnu.org/software/make/manual/make.html#Call-Function ) would be a better choice: it would only take $(OUTPUTOBJ) to be either "-Fo$(1)" or "-o $(1)" and use would be $(call OUTPUTOBJ,$@).

Show outdated Hide outdated byterun/Makefile.shared
PRIMS=\
alloc.c array.c compare.c extern.c floats.c gc_ctrl.c hash.c \
intern.c interp.c ints.c io.c lexing.c md5.c meta.c obj.c parsing.c \
signals.c str.c sys.c terminfo.c callback.c weak.c finalise.c stacks.c \
dynlink.c backtrace_prim.c backtrace.c spacetime.c afl.c
OBJS=$(COMMONOBJS) $(UNIX_OR_WIN32).$(O) main.$(O)

This comment has been minimized.

@adrien-n

adrien-n Dec 25, 2016

Contributor

I think you can move $(UNIX_OR_WIN32).$(O) and main.($O) right in $(COMMONOBJS) and at that point you can probably rename $(COMMONOBJS) to $(OBJS) directly.

@adrien-n

adrien-n Dec 25, 2016

Contributor

I think you can move $(UNIX_OR_WIN32).$(O) and main.($O) right in $(COMMONOBJS) and at that point you can probably rename $(COMMONOBJS) to $(OBJS) directly.

Show outdated Hide outdated byterun/Makefile.shared
@@ -154,6 +159,21 @@ clean ::
rm -f primitives prims.c caml/opnames.h caml/jumptbl.h
rm -f caml/version.h
libcamlrun.$(A): $(OBJS)
$(call MKLIB,$@, $^)

This comment has been minimized.

@adrien-n

adrien-n Dec 25, 2016

Contributor

I think you can remove the space before $^. I see where it comes in the current sources from but it's not typical for gnu make code to have a space there.

@adrien-n

adrien-n Dec 25, 2016

Contributor

I think you can remove the space before $^. I see where it comes in the current sources from but it's not typical for gnu make code to have a space there.

Show outdated Hide outdated byterun/Makefile.nt
$(call MAKE_OCAMLRUN,ocamlrun$(EXE),prims.$(O) libcamlrun.$(A) \
$(call SYSLIB,ws2_32) $(EXTRALIBS))
ocamlrun$(EXE): prims.$(O) libcamlrun.$(A)
$(call MAKE_OCAMLRUN,$@,$^ $(call SYSLIB,ws2_32) $(EXTRALIBS))

This comment has been minimized.

@adrien-n

adrien-n Dec 25, 2016

Contributor

Tangential to your change: I think EXTRALIBS is always empty. I don't know if there are actual users of it (who might provide a non-empty value themselves).

@adrien-n

adrien-n Dec 25, 2016

Contributor

Tangential to your change: I think EXTRALIBS is always empty. I don't know if there are actual users of it (who might provide a non-empty value themselves).

Show outdated Hide outdated byterun/Makefile.shared
@@ -159,6 +159,21 @@ clean ::
rm -f primitives prims.c caml/opnames.h caml/jumptbl.h
rm -f caml/version.h
ifeq "$(UNIX_OR_WIN32)" "win32"

This comment has been minimized.

@adrien-n

adrien-n Dec 25, 2016

Contributor

I think it will be better to have this block near the top of the file, along the other variables definitions, before the rules.

@adrien-n

adrien-n Dec 25, 2016

Contributor

I think it will be better to have this block near the top of the file, along the other variables definitions, before the rules.

Show outdated Hide outdated byterun/Makefile.shared
ifdef BOOTSTRAPPING_FLEXLINK
MAKE_OCAMLRUN=$(MKEXE_BOOT)
else
MAKE_OCAMLRUN = $(MKEXE) -o $(1) $(2)

This comment has been minimized.

@adrien-n

adrien-n Dec 25, 2016

Contributor

Is there a reason not to add $(BYTECCLINKOPTS) like in the unix case? As far as I see, it's empty in practice for mingw* and msvc* so it wouldn't change anything but it'd be one difference less.

After that I'd be tempted to put MAKE_OCAMLRUN = $(MKEXE) ... before the first conditional and overwrite it in the win32 && BOOTSTRAPPING_FLEXLINK case. This would save quite a few lines and make the block more readable.

@adrien-n

adrien-n Dec 25, 2016

Contributor

Is there a reason not to add $(BYTECCLINKOPTS) like in the unix case? As far as I see, it's empty in practice for mingw* and msvc* so it wouldn't change anything but it'd be one difference less.

After that I'd be tempted to put MAKE_OCAMLRUN = $(MKEXE) ... before the first conditional and overwrite it in the win32 && BOOTSTRAPPING_FLEXLINK case. This would save quite a few lines and make the block more readable.

Show outdated Hide outdated byterun/Makefile.shared
@@ -198,3 +198,38 @@ libcamlrun_shared.$(SO): $(PICOBJS)
%.$(DBGO): %.c
$(CC) $(DFLAGS) -c $(OUTPUTOBJ)$@ $<
ifneq "$(TOOLCHAIN)" "msvc"

This comment has been minimized.

@adrien-n

adrien-n Dec 25, 2016

Contributor

OK for the code as it is I think. There's a lot I'd like to change here but I guess you too think it's better to leave that for later.

@adrien-n

adrien-n Dec 25, 2016

Contributor

OK for the code as it is I think. There's a lot I'd like to change here but I guess you too think it's better to leave that for later.

Show outdated Hide outdated byterun/Makefile.shared
@@ -198,6 +209,12 @@ libcamlrun_shared.$(SO): $(PICOBJS)
%.$(DBGO): %.c
$(CC) $(DFLAGS) -c $(OUTPUTOBJ)$@ $<
%.i.$(O): %.c
$(CC) $(IFLAGS) -c $(OUTPUTOBJ)$@ $<

This comment has been minimized.

@adrien-n

adrien-n Dec 25, 2016

Contributor

Same as earlier: a macro could avoid the special handling of $(OUTPUTOBJ).

@adrien-n

adrien-n Dec 25, 2016

Contributor

Same as earlier: a macro could avoid the special handling of $(OUTPUTOBJ).

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 27, 2016

Contributor
Contributor

shindere commented Dec 27, 2016

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 27, 2016

Contributor
Contributor

shindere commented Dec 27, 2016

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 27, 2016

Contributor
Contributor

shindere commented Dec 27, 2016

shindere added some commits Dec 14, 2016

In byterun's build system: remove invocations of ranlib from install …
…targets

Except for the msvc Windows port, ranlib is called when libraries are
built so calling it a second time on the installed libraries is not
necessary.

At the moment, on the MSVC port, ranlib is not called for built
libraries (see the MKLIB make function in config/Makefile.msvc*) but
since this port defines RANLIB to "echo", calling is a no-op and thus
removing the call at install-time has no consequence.
byterun/Makefile.shared: get rid of the CAMLRUN variable
This was set to ../boot/ocamlrun and used in the install rule.

Thus, the installed ocamlrun was the one from ../boot.

After this commit, the installed ocamlrun is the one from the bytrerun
directory. In priinciple it is supposed to be the same than the one in
../boot at install time so this should not change anything.

Still, it seems cleaner to install the ocamlrun of the current directory
because it is another part of the build system which is in charge of
copying it to ../boot so relying on this other part is less clean and
safe.
In byterun, simplify and improve make install
This commit contains two simplifications:

1. The caml directory under $(INSTALL_LIBDIR) was created only if it
didn't already exist. The commit removes the existence test and uses
the -p option to mkdir.

2. the commands of the form

  cp foo "$(INSTALL_DIR)/foo"

  have been replaced by:

  cp foo "$(INSTALL_DIR)"

This commit also contains a bugfix: the installation directories were
not quoted in the commands installing the instrumented toplevel, so that
an attempt to install it in a directory whose name contains spaces
would have failed before this commit and now works.
byterun/Makefile.shared: remove comment
The TODO comment removed by this commit has to do with
cross-compilation. The comment was wrongly attached to ocamlrund whereas
it is a general one. The issue it describes will be delt with later.
byterun/Makefile.shared: use variables to collect objects to build/in…
…stall

Before this commit, make clean did not remove the instrumented runtime,
if it had been built. This is fixed by this commit.

This commit also removes ``primitives'' as a prerequisite of the ``all''
target because it will be built anyway so it seems there is no reason to
list it explicitly.
byterun/Makefile*: factorize rules to build object files
This commit introduces the OUTPUTOBJ variable to define which option to
use to give a name to an object file.

We do not use the OBJ_NAME_FLAGS variable because it is not defined
everywhere and it seemed lighter to do it that way than to modify the
configure script, given that the changes are temporary.
byterun/Makefile.nt: simplify and generalize the rules building ocaml…
…run and ocamlrund

ocamlrund had main.$O) as a prerequisite but did not actually link this
object file so it has been removed from the list of prerequisites.
byterun/makefiles: introduce the LIBS variable
This makes the rules used to build the runtime systems closer to each
other between the two build systems.
config/Makefile.m*: define the MKEXEDEBUGFLAG variable
This variable is defined in the config/Makefile produced by the
configure script on Unix systems but was not defined in the Windows
makefiles.
Canonicalize byterun's build system
Move content of Makefile.shared to Makefile and adjust Makefile.nt
to include Makefile rather than Makefile.shared
@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 29, 2016

Contributor

Just rebased on latest trunk. Shall I squash?

Contributor

shindere commented Dec 29, 2016

Just rebased on latest trunk. Shall I squash?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 29, 2016

Member

Squashing should only be used to remove unnecessary back-and-forth, intermediate states that turned out to be wrong and where fixed within the same patchset, from the history. If you found the work easier to build and inspect and review as a series of meaningfully separated commit, there is no reason to squash, and there is reason not to squash: small, atomic commits are much more convenient when going back in time to understand a change, handling conflicts, etc.

Member

gasche commented Dec 29, 2016

Squashing should only be used to remove unnecessary back-and-forth, intermediate states that turned out to be wrong and where fixed within the same patchset, from the history. If you found the work easier to build and inspect and review as a series of meaningfully separated commit, there is no reason to squash, and there is reason not to squash: small, atomic commits are much more convenient when going back in time to understand a change, handling conflicts, etc.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 29, 2016

Contributor
Contributor

shindere commented Dec 29, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 29, 2016

Member

I think that it is your call to make (note: I'm not sure bisecting from the merge branch will automatically enter merge commits, I think that it bisects at the granularity of merge commits only, and then users have to bisect within the faulty merge). If it was my PR, I would probably try to test the build after each commit, but with 24 commits that could be too long. I think that whatever you choose will be good.

Member

gasche commented Dec 29, 2016

I think that it is your call to make (note: I'm not sure bisecting from the merge branch will automatically enter merge commits, I think that it bisects at the granularity of merge commits only, and then users have to bisect within the faulty merge). If it was my PR, I would probably try to test the build after each commit, but with 24 commits that could be too long. I think that whatever you choose will be good.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 29, 2016

Contributor
Contributor

shindere commented Dec 29, 2016

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Dec 29, 2016

Contributor

I'd keep the commits, personally (if you have the time to leave a script running to test the commits just before merge then great - but git bisect skip is always there if needed)

Contributor

dra27 commented Dec 29, 2016

I'd keep the commits, personally (if you have the time to leave a script running to test the commits just before merge then great - but git bisect skip is always there if needed)

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 29, 2016

Contributor
Contributor

shindere commented Dec 29, 2016

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Dec 29, 2016

Contributor

Sorry - I meant if all the commits are working on one platform the that's sufficient - the fixes for others in a bisect would either be trivial or can be skipped

Contributor

dra27 commented Dec 29, 2016

Sorry - I meant if all the commits are working on one platform the that's sufficient - the fixes for others in a bisect would either be trivial or can be skipped

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 29, 2016

Contributor
Contributor

shindere commented Dec 29, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 29, 2016

Member

I did some intermediate testing and tried to be very rigourous

Then please consider it "good enough", and go on with your day doing something else that I am sure will also be useful. Your work is already above the unspoken quality standards of the OCaml contribution -- which is fortunate given that the build system is a fiddly area requiring care.

Member

gasche commented Dec 29, 2016

I did some intermediate testing and tried to be very rigourous

Then please consider it "good enough", and go on with your day doing something else that I am sure will also be useful. Your work is already above the unspoken quality standards of the OCaml contribution -- which is fortunate given that the build system is a fiddly area requiring care.

@gasche gasche merged commit 895d4ae into ocaml:trunk Dec 29, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 29, 2016

Member

I added a no-changes-entry-needed label to make the CI happy and I merged.

I do think that this work deserves a Changes entry, but we already discussed this in this other PR, and my understanding is that @shindere plans to have a single change entry "at the end" of the merge marathon.

Member

gasche commented Dec 29, 2016

I added a no-changes-entry-needed label to make the CI happy and I merged.

I do think that this work deserves a Changes entry, but we already discussed this in this other PR, and my understanding is that @shindere plans to have a single change entry "at the end" of the merge marathon.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 29, 2016

Contributor
Contributor

shindere commented Dec 29, 2016

@shindere shindere deleted the shindere:merge-byterun-makefiles branch Dec 29, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 29, 2016

Member

Well, just my uninformed opinion:

  • One important aspect of changes is to keep a trace of the remarkable work that you and your trusted reviewers (@dra and @adrien-n in particular, but in general anyone that helped in the review process) have done on the general theme of merging the build system.

  • Another important aspect is to have the information in place for people to understand what was affected and, if they turn out to be surprised by an aspect of the change in the future (be it a bug, a simple choice, or wrong assumptions on their part), to be able to find the change that caused this surprise by browsing the changelog, and then have pointers to extra information to understand it. This is why we want to have all the GPR numbers in the change entry (and mantis tickets if applicable). It may also justify mentioning the changes to Makefile.config (with pointers to a place where people affected by the change can understand its justification and what they should do).

It is not an issue if the change entry is large, because the work itself is large as well.

Member

gasche commented Dec 29, 2016

Well, just my uninformed opinion:

  • One important aspect of changes is to keep a trace of the remarkable work that you and your trusted reviewers (@dra and @adrien-n in particular, but in general anyone that helped in the review process) have done on the general theme of merging the build system.

  • Another important aspect is to have the information in place for people to understand what was affected and, if they turn out to be surprised by an aspect of the change in the future (be it a bug, a simple choice, or wrong assumptions on their part), to be able to find the change that caused this surprise by browsing the changelog, and then have pointers to extra information to understand it. This is why we want to have all the GPR numbers in the change entry (and mantis tickets if applicable). It may also justify mentioning the changes to Makefile.config (with pointers to a place where people affected by the change can understand its justification and what they should do).

It is not an issue if the change entry is large, because the work itself is large as well.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Jan 9, 2017

Contributor
Contributor

shindere commented Jan 9, 2017

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

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