Fix linking of .cmxs targets that link stubs. #159

whitequark · 2017-02-26T22:01:59Z

See the individual commit messages. In general, this addresses the reasons 0.10.1 got reverted, and also fixes several glaring omissions in the build system and test suite that made writing the test for the fix quite obnoxious.

gasche · 2017-02-26T22:55:21Z

Makefile

@@ -143,13 +143,22 @@ src/ocamlbuildlib.cmxa: src/ocamlbuild_pack.cmx $(EXTRA_CMX)

 # The packs

+# Build artifacts are first placed into tmp/ to avoid a race condition
+# described in https://caml.inria.fr/mantis/view.php?id=4991.


Would it not be simpler to just have src/ocamlbuild_pack.cmx depend on src/ocamlbuild_pack.cmi? This corresponds to serializing (byte before native), but it makes for simpler code (and makes sure there are no side-effects related to running in a different directory).

@gasche It doesn't work. The ocamlopt command touches the cmi file, leading to a spurious rebuild afterwards.

Ah... Now we understand #128!

gasche · 2017-02-26T22:57:34Z

src/ocaml_specific.ml

  (Ocaml_compiler.native_shared_library_link ~tags:["profile";"linkall"] "%.p.cmxa" "%.p.cmxs");;

 rule "ocaml: cmxa & a -> cmxs & so"
  ~prods:["%.cmxs"; x_dll]
-  ~deps:["%.cmxa"; x_a]
+  ~deps:["%.cmxa"]


if .a is not a dependency, the rule text/name should be cmxa -> cmxs & so. (Same above)

gasche · 2017-02-26T23:45:50Z

I would still like to do a release without any of the .cmxs features included, as was my original idea, to make sure that the safe features proposed by the users before it get their foot in the released door. I am fine with doing also a release with the feature-related parts of this PR right after that.

Before the recent .cmxs refactor, i.e. 89888f2...f865497, .cmxs files were built from .cmx and .o files. After commit f865497, .cmxs files are built from .cmxa and .a files, which is wrong, because .cmxa files already contain the autolink instructions necessary to link the .a file; and on top of that, no -I flag was provided when linking the .cmxs file, which resulted in the linker being unable to follow the autolink instructions. This caused failures in the wild when ocamlbuild 0.10.0 got released: ocaml/opam-repository#8157. After this commit, .a files are no longer explicitly passed when building a .cmxs file. Additionally, for every linked file, a -I flag is passed, with the directory containing the file being linked.

whitequark · 2017-02-27T23:16:01Z

@gasche CI is now green here.

whitequark · 2017-02-27T23:21:16Z

@gasche So, how exactly (in terms of workflow) do you imagine making this intermediate release?

gasche · 2017-02-27T23:22:30Z

Two things:

With the previous .cmxs / .mllib (wrong) changes, I could convince myself that the common ocamlbuild users that don't use .cmxs without a .ml(dy)lib would not see any behavioral change -- in particular, no regression. Your proposed patch touches a codepath that absolutely everyone uses. How do we convince ourselves that there are no regressions? (I suppose that the reasoning is: the linked functions are only called at "link" time, and for normal link-time usage -I flags mostly don't matter, the list of cases where it does matter is X, Y and Z, and for all of these the change improves the situation.)
My guts still tell me that it would be wise to do a release without any of the too-ambitious cmxs/mllib changes (including the toolchain feature for example), by reverting the currently broken commits, (we could call this 10.x or 11.0) and then a second release with those changes (11.0 or 12.0). Does that sound acceptable to you? I won't have time to work on this tonight, so it would only be later this week, which means I may feel bad about delaying the release of your work even more.

whitequark · 2017-02-27T23:25:06Z

I suppose that the reasoning is: the linked functions are only called at "link" time, and for normal link-time usage -I flags mostly don't matter, the list of cases where it does matter is X, Y and Z, and for all of these the change improves the situation.

Yes, this is exactly my argument.

How about just installing a non-insignificant amount of packages using ocamlbuild?

gasche · 2017-02-27T23:25:32Z

So, how exactly (in terms of workflow) do you imagine making this intermediate release?

My idea was:

Revert the cmxs/mllib changes
change the Changelog to present things as if the broken 10.x release had not happened, except a brief mention that they contained regressions and that we will just forget about them. Write the current Changes (the new features that were contributed since 9.x) in the section for this "safe" release (we need to decide whether to call it 10.x or 11.0; no strong opinion here)
Apply the cmxs/mllib again thanks to your fix.
Do another release (bumping the major version number).

We can merge the present PR before all of this (it would be reverted, and then un-reverted), or only at step (3).

Ideally I would have liked to test an opam build with the post-merge state (with the cmxs/mllib features in) before releasing it on the opam-repository, but I don't have the energy to setup this experiment right now.

whitequark · 2017-02-27T23:29:11Z

My guts still tell me that it would be wise to do a release without any of the too-ambitious cmxs/mllib changes (including the toolchain feature for example)

Is this really any better than the already-released 0.9.3? That's a lot of effort to expend on a number of minor fixes, and we still have to do the exactly same amount of testing for any more ambitious release on top of that.

gasche · 2017-02-27T23:36:23Z

This is the reasoning I had when deciding to skip this step and release 10.x, and then I regretted it.

That said, I am not in a strong position for negotiation right now. If you strongly think that we should go ahead and just release 11.x, I am willing to be convinced. (Especially if I have the impression impression that you might help mitigate potential regressions since 0.9.x that would appear after the release.)

whitequark · 2017-02-27T23:41:45Z

That said, I am not in a strong position for negotiation right now. If you strongly think that we should go ahead and just release 11.x, I am willing to be convinced. (Especially if I have the impression impression that you might help mitigate potential regressions since 0.9.x that would appear after the release.)

Yes, that's the idea, on both counts. In fact if anyone pinged me before reverting 0.10.x we might have avoided this whole thing.

gasche · 2017-02-27T23:46:52Z

Ok, let's go with it then. I propose to merge this and open a separate issue to discuss release coordination -- I can do some Changelog stuff, but I think it would be nice if you sent the announce when it happens. Do you have plans on the testing front?

whitequark · 2017-02-28T00:21:20Z

Ok, let's go with it then. I propose to merge this and open a separate issue to discuss release coordination -- I can do some Changelog stuff, but I think it would be nice if you sent the announce when it happens.

Sounds good.

Do you have plans on the testing front?

Yes, I'm running some tests now.

This was referenced Feb 26, 2017

Ocamlbuild.0.10.1 ocaml/opam-repository#8156

Closed

Add a .mllib -> .cmxs rule #131

Closed

gasche reviewed Feb 26, 2017

View reviewed changes

This was referenced Feb 26, 2017

Do not pass the -toolchain option when building a plugin #160

Merged

tgls-ios doesn't seem to install ocaml-cross/opam-cross-ios#5

Closed

gasche merged commit f175c7d into ocaml:master Feb 27, 2017

whitequark deleted the fix-cmxs-build branch February 28, 2017 00:21

Fix linking of .cmxs targets that link stubs. #159

Fix linking of .cmxs targets that link stubs. #159

Uh oh!

Conversation

whitequark commented Feb 26, 2017

Uh oh!

gasche Feb 26, 2017

Choose a reason for hiding this comment

Uh oh!

whitequark Feb 26, 2017

Choose a reason for hiding this comment

Uh oh!

gasche Feb 26, 2017

Choose a reason for hiding this comment

Uh oh!

gasche Feb 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whitequark Feb 26, 2017

Choose a reason for hiding this comment

Uh oh!

gasche commented Feb 26, 2017

Uh oh!

whitequark commented Feb 27, 2017

Uh oh!

whitequark commented Feb 27, 2017

Uh oh!

gasche commented Feb 27, 2017

Uh oh!

whitequark commented Feb 27, 2017

Uh oh!

gasche commented Feb 27, 2017

Uh oh!

whitequark commented Feb 27, 2017

Uh oh!

gasche commented Feb 27, 2017

Uh oh!

whitequark commented Feb 27, 2017

Uh oh!

gasche commented Feb 27, 2017

Uh oh!

whitequark commented Feb 28, 2017

Uh oh!

Uh oh!

gasche Feb 26, 2017 •

edited

Loading