Makefile cleanup #1434

damiendoligez · 2017-10-18T15:22:12Z

main Makefile:

remove some dicrepancies between Unix and Windows
fix hard-bootstrap instructions
remove obsolete targets package-macosx and base.opt
various clean-ups

cc @garrigue : is base.opt really obsolete?

dra27 · 2017-10-18T15:45:30Z

Is this also going through precheck?

shindere · 2017-10-18T15:59:40Z

David Allsopp (2017/10/18 15:45 +0000):

Is this also going through precheck?

It did -- I was with Damien when he worked on it.

shindere · 2017-10-18T17:04:02Z

In the checkstack target, is the "else ; " line really necessary, actually?
(I realise it was already there, just wondering whether we couldn't get
rid of it, while we are at it)

In ci-build: is it really necessary to run core and coreboot
before world?

Would a code like:

if $make_native; then
target=world.opt
else
target=world
fi
$make $jobs $target

be okay?

xavierleroy

Looks good modulo a probably unwanted change to ci-build.

xavierleroy · 2017-10-19T07:27:05Z

Makefile

@@ -16,13 +16,18 @@
 # The main Makefile

 # Hard bootstrap how-to:
-# (only necessary in some cases, for example if you remove some primitive)
+# (only necessary if you remove or rename some primitive)
 #


This update to the "hard bootstrap how-to" is welcome, but note that these two rounds of bootstrap are needed only because the bootstrap is currently half-broken. We need to talk about ways to fix it. My feeling is that it can be fixed easily and that the bootstrap can be further simplified to "half a round".

After working on it for a few hours I believe that we cannot un-break it without reverting #324, so we have to choose between:

one round of bootstrap for every case

two rounds for removing a primitive and no bootstrap needed when adding one.

xavierleroy · 2017-10-19T07:31:07Z

tools/ci-build

@@ -218,7 +218,6 @@ case $configure in
  *) error "internal error";;
 esac

-$make $jobs coldstart
 $make $jobs core


This feels very wrong. If you start with a fresh copy of the sources (not from a previous build), boot/ is missing a number of files (boot/ocamlrun + the stdlib) that make core needs. It's the job of make coldstart to populate boot/.

More generally, I strongly suggest to do $make $jobs world.opt and skip the bootstrap in the main CI test. Reasons:

make world.opt is what most if not all users do. In the current ci-build we have a build procedure that doesn't match the procedure(s) that the end-user will apply. With accompanying risk of divergence.

It is wasteful to test the bootstrap on all configurations every time. Let's have a specific bootstrap test in the other-configs CI test.

But core does coldstart so it works.

Good point about world.opt, but what will users really do? If they go by README.adoc, they'll do:

make world make bootstrap [optional] make opt make opt.opt

and then they are told that they could have done make world.opt.

If they go by README.win32.adoc they'll do:

make world bootstrap opt opt.opt install

If they use OPAM, they'll do:

make world make world.opt

I see much room for improvement here.

But core does coldstart so it works.

Urgh, I didn't guess that.

I would still very much prefer make world.opt or, almost equivalently,

make world make opt make opt.opt

The bootstrap needs to be de-emphasized from README.adoc. In the very early days, it was the best test we had that the newly-built ocamlc worked. Nowadays we have the test suite as a much more comprehensive test. Building the .opt compilers is also a pretty good way to exercise a compiler that we just built, although it is ocamlopt rather than ocamlc.

I see much room for improvement here.

Agreed.

dra27

All the Makefile changes look good to me - just comments on whether the improvements could go further.

dra27 · 2017-10-19T08:30:42Z

Makefile

-else
-	$(MAKE) opt-core
-	$(MAKE) otherlibrariesopt ocamltoolsopt
-endif


Wouldn't it be better to have adopted the Windows version here?

opt: opt-core $(MAKE) otherlibrariesopt ocamltoolsopt

and make the relationship between opt and opt-core explicit?

If you look at the file's history you'll see a lot of back-and-forth between dependencies and recursive calls to $(MAKE), all of them committed to fix parallel make. In my eye, all these versions are equivalent. I probably chose this one with the idea that opt-core would disappear eventually.

Oh, and I don't like having a phony target as a dependency because then it's not clear that it will be built every time. Better to call it explicitly IMO.

dra27 · 2017-10-19T08:39:17Z

Makefile

-opt.opt: core opt-core ocamlc.opt all ocamlopt.opt ocamllex.opt \
-         ocamltoolsopt ocamltoolsopt.opt otherlibrariesopt $(OCAMLDOC_OPT) \
-         ocamltest.opt
-endif


Maybe this could be considered as not for this GPR, but if opt.opt is being tidied up then perhaps it should go the whole hog. There's no earthly reason why the target for "building the native-code versions of the tools" should run checkstack, runtime, core, ocaml, opt-core, otherlibraries $(WITH_DEBUGGER) $(WITH_OCAMLDOC) ocamltest, otherlibrariesopt or ocamltoolsopt.

I think most of this is here because world.opt is lazy? I think tests and manual-pregen should probably depend on world.opt rather than opt.opt if it is changed.

At some point we'll probably decide between making each of these targets self-sufficient, or properly documenting their dependencies. And then I hope we'll eliminate most of them.
But this is for another PR.

dra27 · 2017-10-19T08:53:37Z

Makefile

 	$(MAKE) core
 	$(MAKE) ocaml
-endif


Again, if we're in the business of tidying these, why not do the minimum necessary to start the toplevel:

$(MAKE) coldstart $(MAKE) ocamlc $(MAKE) ocaml

Note that the toplevel is started with option -I otherlibs/[win32]unix so we should probably also build otherlibraries for consistency.

dra27 · 2017-10-19T08:56:52Z

Makefile

 .PHONY: checkstack
 checkstack:
+ifeq "$(UNIX_OR_WIN32)" "unix"


Your Makefile-fu is superior to mine - is this one of those cases where it would be a good idea for the Windows part to have:

@

(i.e. have the Windows target being a recipe which does nothing, rather than a target with no recipe)?

I don't think it makes a difference in behavior, but I'll do it because it improves readability.

shindere · 2017-10-19T14:23:03Z

Generally speaking, I'f favour versions where $(MAKE) is not called with several targets simultaneously, like in $(MAKE) foo bar baz I'd prefer several invocations of $(MAKE) to make it really clear what happens.

shindere · 2017-10-19T14:50:31Z

Ultimately, I think the only thing end-users should have to do is make. IMO, it's configure's job to figure out what needs to be compiled, eventually with some command-line arguments to tune this. I realise this situation cannot be reached right now. Meanwhiile, I'd suggest to update all the docs and CI script to use either world or world.opt depending on whether one compiles just the bytecode compiler or both compilers. One other thing that could be done is to introduce a "build" script that would encapsulate all this. The docs (resp. CI scripts) could then refer to (resp. use) it. I realise this is not standard, but having this script in place before the migration to autoconf may make the transition easier for users like @dra27 who have to work in several environments.

damiendoligez · 2017-10-19T15:18:55Z

@xavierleroy The "probably unwanted" change is actually the starting point of this PR...

Anyway, here is a new commit that takes (most) remarks into account. @dra27 feel free to insist if you feel strongly about your first remark.

dra27 · 2017-10-19T15:22:23Z

All LGTM!

xavierleroy

OK, that will do just fine for the moment. But this boostrap step in the CI script will go away eventually, I'm sure.

xavierleroy · 2017-10-19T17:34:19Z

@damiendoligez I'm unable to guess if you intend this change for 4.06 or not, so I'll let you do the appropriate merge(s).

damiendoligez · 2017-10-20T08:57:40Z

I'm waiting to give @garrigue a chance to comment. No hurry for this one.

garrigue · 2017-10-24T00:12:34Z

Well, I suppose that I'm called in because I'm the originator of the optimized world.opt target.
But honestly, I don't have much to say at this point. We are talking of a different era, when compiling everything with the byte code version of the native compiler was very slow, so I introduced a mechanism to use the native version as much as possible. This was removed a while ago by @alainfrisch IIRC, to make things simpler. Also, most of the (big) otherlibraries and camlp4 are now out of the distribution, meaning that speed of compilation in this later phase is no longer an issue.

Making the targets more standard could indeed help new developers.
By the way, I don't think many end users compiled caml by hand these days, so they are less of a problem (on the other hand some packagers may not be ocaml experts).

shindere · 2017-10-24T05:04:51Z

Jacques Garrigue (2017/10/23 17:12 -0700):

Well, I suppose that I'm called in because I'm the originator of the optimized `world.opt` target.

No, rather because, according to git blame, it seems it's you who introduced the base.opt target. That's, I think, the one Damien was wondering about.

But honestly, I don't have much to say at this point. We are talking of a different era, when compiling everything with the byte code version of the native compiler was very slow, so I introduced a mechanism to use the native version as much as possible. This was removed a while ago by @alainfrisch IIRC, to make things simpler. Also, most of the (big) otherlibraries and camlp4 are now out of the distribution, meaning that speed of compilation in this later phase is no longer an issue.

Okay. From all this I deduce that you do nt yourself use the base.opt target explicitly and that you won't miss it.

* percent-encode base-64 code when sharing * to fix old share links, convert ' ' to '+' before base-64 decoding * add #rawcode= as alternative to #code= (takes plain non-base64, but percent-encoded text)

damiendoligez added 3 commits October 18, 2017 13:59

clean up Makefile

cc9b789

fix bootstrap instructions

577bada

ci-build: remove now-redundant make coldstart

41ee22b

xavierleroy requested changes Oct 19, 2017

View reviewed changes

dra27 reviewed Oct 19, 2017

View reviewed changes

damiendoligez added the no-change-entry-needed label Oct 19, 2017

take some reviewers' remarks into account

af78f02

xavierleroy approved these changes Oct 19, 2017

View reviewed changes

damiendoligez added this to the 4.07-or-later milestone Oct 20, 2017

gasche merged commit 3882302 into ocaml:trunk Dec 10, 2017

damiendoligez deleted the makefile-cleanup branch January 19, 2018 14:03

vicuna mentioned this pull request Mar 14, 2019

Hard bootstrap procedure is broken #7688

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makefile cleanup #1434

Makefile cleanup #1434

damiendoligez commented Oct 18, 2017

dra27 commented Oct 18, 2017

shindere commented Oct 18, 2017 via email

shindere commented Oct 18, 2017

xavierleroy left a comment

xavierleroy Oct 19, 2017

damiendoligez Oct 19, 2017

xavierleroy Oct 19, 2017

damiendoligez Oct 19, 2017

xavierleroy Oct 19, 2017 •

edited

Loading

dra27 left a comment

dra27 Oct 19, 2017

damiendoligez Oct 19, 2017

damiendoligez Oct 19, 2017

dra27 Oct 19, 2017

damiendoligez Oct 19, 2017

dra27 Oct 19, 2017

damiendoligez Oct 19, 2017 •

edited

Loading

dra27 Oct 19, 2017

damiendoligez Oct 19, 2017

shindere commented Oct 19, 2017 via email

shindere commented Oct 19, 2017 via email

damiendoligez commented Oct 19, 2017

dra27 commented Oct 19, 2017

xavierleroy left a comment

xavierleroy commented Oct 19, 2017

damiendoligez commented Oct 20, 2017

garrigue commented Oct 24, 2017

shindere commented Oct 24, 2017 via email

Makefile cleanup #1434

Makefile cleanup #1434

Conversation

damiendoligez commented Oct 18, 2017

dra27 commented Oct 18, 2017

shindere commented Oct 18, 2017 via email

shindere commented Oct 18, 2017

xavierleroy 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

xavierleroy Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

dra27 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

damiendoligez Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shindere commented Oct 19, 2017 via email

shindere commented Oct 19, 2017 via email

damiendoligez commented Oct 19, 2017

dra27 commented Oct 19, 2017

xavierleroy left a comment

Choose a reason for hiding this comment

xavierleroy commented Oct 19, 2017

damiendoligez commented Oct 20, 2017

garrigue commented Oct 24, 2017

shindere commented Oct 24, 2017 via email

xavierleroy Oct 19, 2017 •

edited

Loading

damiendoligez Oct 19, 2017 •

edited

Loading