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.

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

Fix the bootstrap procedure and its documentation #1805

Merged
merged 4 commits into from May 31, 2018

Conversation

Projects
None yet
6 participants
@shindere
Copy link
Contributor

commented May 30, 2018

As mentionned in the Changes entry, this is joint work with @xavierleroy
and @damiendoligez.

TL+DR: the bootstrap procedure was broken and this PR fixes it

Therer were two main issues:

  1. stripdebug can actually not be used by the promote-cross target
    so this PR uses cp instead. Since stripdebug can be used by the
    promote target, which is called later than promote-cross during
    the bootstrap, the final executables will still be stripped of
    their debugging symbols.

  2. The make_opcodes tool was run on the wrong runtime during
    the bootstrap, which this PR also fixes.

In addition, the PR also contains a bit of makefile cleanup and
simplification and updates the documentation to make it reflect the current
state of affairs. This documentation update includes removing the
"Hard bootstrap how-to" that appeared at the beginning of the root
Makefile and moving the bootstrap-related explanations from INSTALL.adoc to
HACKING.adoc. Finally, INSTALL.adoc has been updated to suggest
running the testsuite rather than bootstrapping.

@gasche

This comment has been minimized.

Copy link
Member

commented May 30, 2018

The changes to the Makefile seem fine. Some minor comments on the change to HACKING.adoc:

  • The new content is more detailed and more technical than the rest of the file, so it kind of breaks the reading flow with more details than the user may want. Maybe the detailed bootstrapping steps could be in boot/HACKING.adoc or HACKING-bootstrap.adoc, with only a short description and reference in the root HACKING file?

  • It is not clear from the new documentation what "bootstrap the compiler" or "proceed with the bootstrap" actually means. The very last step is make bootstrap, but as a beginner reader I have no idea what this is supposed to do. In comparison, the previous text in INSTALL had an explanation: "you can try to bootstrap the system -- that is, to recompile all OCaml sources with the newly created compiler".

  • Reading the bootstrap section of HACKING, it is not clear to me when I need to do the steps documented there, or why I would want to. The relevant sentence starts with: "If you wish or need to modify and bootstrap the compiler, ...". Does this mean that I should do this everytime I wish to modify the compiler? Why would I want to bootstrap the compiler?

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

@lpw25

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

Does make bootstrap now do what the hard bootstrap procedure does? If not, why is it being removed from the makefile?

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

It would be nice if the new procedure worked correctly for removing a primitive. I had a lot of trouble with this recently but did eventually manage it -- I can dig out a shell history if it would be helpful.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

@mshinwell: for your convenience, I just pushed the change-exe-magic-number
and remove-primitive branches to my shindere/ocaml GitHub repository. These
are the commits I cherry-picked to make sure the procedure was actually
fixed. So if you checkout the code of this PR, you should be able to follow
the instructions that are currently in HACKING.adoc. As step 4, cherry-pick
the two previously mentionned commit. This should work and you should be
able to go until the make bootstrap step.

@lpw25

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

Well, I've frequently had to use the hard procedure when make bootstrap has failed. I've also had cases where I've needed to do various promotions by hand because even the hard bootstrap procedure wasn't enough to make the change.

The discussion always seems to be phrased in terms of adding and removing primitives, but I've had plenty of times where I'm not doing either of those but I've still needed to do a bootstrap to get things to compile. Is the discussion just needlessly narrow, or should we really only need to bootstrap for changes to the primitives and every other time I need to bootstrap it is some kind of bug in the build system.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

The discussion always seems to be phrased in terms of adding and removing primitives, but I've had plenty of times where I'm not doing either of those but I've still needed to do a bootstrap to get things to compile.

Indeed, anything that changes the format or meaning of .cmi, .cmo, .cma and bytecode executable files eventually needs a bootstrap, in particular to get the toplevel to work again. In particular, every time you change the representation of types, the .cmi format changes.

@lpw25

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

To help discussing this, would you be able to provide a concrete example
of a thing that did require a bootstrap without you expecting it?

Changes that affect the cmi format are the most common example.

The most awkward example I've had was adding a type parameter to the built in format types as part of my algebraic effects work -- that required manually handling the different promotions, especially the promotion of the standard library, since the change needs to happen part-way through one of the steps in the "hard bootstrap" procedure.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

@damiendoligez damiendoligez self-assigned this May 31, 2018

shindere added some commits May 16, 2018

Makefile: the promote-cross target should use cp rather than stripdebug
The compiler's bootstrap procedure involves calling the promote-cross
target of the root Makefile.

When this target is invoked e.g. in the context of an executable
magic number update, it is actually not possible to use the stripdebug
tool.

Thus, this commit makes sure the promote-cross target uses cp
to promote the bytecode tools, rather than stripdebug.

This is not an issue because in the promote rule used later
during the bootstrap the stripdebug tool can be used, so that the
bytecode executables one gets after a complete bootstrap will remain
small and won't contain any debugging symbol.
Make sure the make_opcode tool is run using byterun/ocamlrun
This tool is used to compile ocamlc. Given that make_opcodes is itself
recompiled as part of the bootstrap process, it needs to be run on the new
(rather than old) runtime. In other words, make_opcodes needs to be run
using byterun/ocamlrun rather than boot/ocamlrun.

This commit fixes this.
Root Makefile enhancements
This commit contains the following fixes and enhancements to the
root Makefile:

 * coreall: add dependency on runtime
 * all: before this commit, this target was depending on runtime and started
   by making coreall. This commit simplifies this and just makes this target
   depend on coreall, which is both simpler and semantically more accurate.
 * compare: let this target exit if the comparison fails
 * bootstrap: this commit removes the final invocation to make compare.
   The comparison was actually done twice, (1) as the final step of coreboot
   and (2) as the final step of bootstrap. The motivation for that was to
   ensure that the comparison messages were not lost and did appear last.
   Actually, it turns out that if the comparison fails, it makes no
   sense to continue the bootstrap process. That's why compare now exits
   immediately when it fails and there is thus no need to perform it twice.
 * Get rid of backup and restore targets which have become obsolete
   with the use of a revision control system

@shindere shindere force-pushed the shindere:fix-bootstrap branch 9 times, most recently from 59a1106 to a72a39b May 31, 2018

Document the bootstrap procedure
 * Remove the now obsolete comments at the beginning of the main Makefile
 * Move and fix documentation of the bootstrap from INSTALL to
   HACKING-bootstrap.adoc
 * Update install instructions

@shindere shindere force-pushed the shindere:fix-bootstrap branch from a72a39b to 96545e8 May 31, 2018

@damiendoligez
Copy link
Member

left a comment

LGTM

@gasche

gasche approved these changes May 31, 2018

Copy link
Member

left a comment

The new version is nicer, thanks!

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2018

Merged

@shindere shindere merged commit 96545e8 into ocaml:trunk May 31, 2018

0 of 4 checks passed

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

@shindere shindere deleted the shindere:fix-bootstrap branch May 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.