Skip to content

Add --toolchain option, for ocamlbuild -toolchain#87

Closed
whitequark wants to merge 1 commit intodbuenzli:masterfrom
whitequark:master
Closed

Add --toolchain option, for ocamlbuild -toolchain#87
whitequark wants to merge 1 commit intodbuenzli:masterfrom
whitequark:master

Conversation

@whitequark
Copy link
Copy Markdown
Contributor

@whitequark whitequark commented Nov 24, 2016

Depends on ocaml/ocamlbuild#125, fixes #86.

@dbuenzli
Copy link
Copy Markdown
Owner

I don't have this in my head but why can't this be simply defined by a configuration key, e.g. like in 2bf815d and appropriate lookup in Pkg.build_cmd ? Especially one baked by an environment variable which would allow to reuse topkg OPAM files without any change in cross-compilation repos.

@whitequark
Copy link
Copy Markdown
Contributor Author

I don't have this in my head but why can't this be simply defined by a configuration key, e.g. like in 2bf815d and appropriate lookup in Pkg.build_cmd ?

No reason other than that I find the code of the new topkg very hard to follow. TOPKG_TOOLCHAIN sounds fine?

Especially one baked by an environment variable which would allow to reuse topkg OPAM files without any change in cross-compilation repos.

This doesn't work on opam 1.x and I think it didn't properly work in opam 2.x either last time I checked that thread.

@dbuenzli
Copy link
Copy Markdown
Owner

TOPKG_TOOLCHAIN sounds fine?

Rather TOPKG_CONF_TOOLCHAIN

@whitequark
Copy link
Copy Markdown
Contributor Author

Done, and works in opam-cross-*

Comment thread src/topkg_build.ml Outdated
Topkg_os.Cmd.run @@
Topkg_cmd.(build_cmd ~use_toolchain:false c os %% v "-just-plugin") >>= fun () ->
Topkg_os.Cmd.run @@
Topkg_cmd.(build_cmd ~use_toolchain:true c os %% of_list files)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very fond of this two level invocation here, in particular the docs here will need update. Is there a valid use case why ocamlbuild would use the toolchain argument to build its own plugin ? Shouldn't ocamlbuild's toolchain option simply affect the requested targets ?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @gasche since this is more a discussion about that toolchain support.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using -plugin-option "-toolchain ..." could also work, couldn't it? (See ocaml/ocamlbuild#129 for a clarification of the various plugin-build-vs-plugin-run options.)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If whitequark agrees that the right semantics for ocamlbuild would be to have -toolchain never apply to the plugin build, I'm not opposed to changing it, of course. I know nothing about cross-compilation so I'll mostly do as I'm told.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gasche I had no idea -plugin-option existed, and I'm mildly confused as to its semantics. Does -plugin-option x mean "run ocamlbuild -just-plugin without x, then run ocamlbuild with x"? Or does it have different semantics depending on whether myocamlbuild.ml exists?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make it clear. I don't like the PR in this current state because for now users can reuse Pkg.build_cmd and simply add arguments (like -tag BLA options according to the configuration) to it without them having to redefine everything.

While this PR keeps backwards compat on this. Doing this after the PR, will break cross compilation for the package. So if possible the support for cross should go entirely in the definition of the build_cmd, if that is impossible we need to break the API and figure out a better way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbuenzli There is no need to separately invoke -just-plugin anymore because ocaml/ocamlbuild#130 (comment) got merged. I will update the PR shortly.

Copy link
Copy Markdown
Contributor Author

@whitequark whitequark Feb 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbuenzli So, how do I get the --toolchain option actually picked up by the CLI? Once again topkg is completely opaque here...

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your questions are completely obscure. In which context ? Configuration keys can be looked up with the configuration that is given to you in some contexts (e.g. package description describe, build description build). See the opaque API documentation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbuenzli Sorry for the opaque question, and thanks for your patience. The problem was not related to topkg after all, but was my mistake, several levels removed from topkg itself.

@whitequark
Copy link
Copy Markdown
Contributor Author

Ping?

Also you'll have to update whatever process by which the natdynlink capability gets discovered to work with -toolchain, as this is another instance in which I found topkg too opaque to patch properly.

@dbuenzli
Copy link
Copy Markdown
Owner

Also you'll have to update whatever process by which the natdynlink capability gets discovered to work with -toolchain, as this is another instance in which I found topkg too opaque to patch properly.

I don't exactly what you see as obscure here. But in any case topkg precisely allows to make a difference between build and host tools. This is described here with the lookup procedure in which you can hook so that that your binaries point to the right one. I'm actually surprised that you didn't try to use this rather than enshrine ocamlfind -toolchain approach which now bleeds into ocamlbuild aswell. You may want to review this ocamlbuild churn in light of the above capabilities offered by topkg (which I'll gadly adjust if they are found not to be suitable enough).

In any case, regarding natdynlink. The OCaml configuration is looked up using ocamlc -config using this mecanism, this is described here. Since the natdynlink capability is not exposed yet in the configuration (see ocaml/ocaml#970), it is discovered as described here, namely we look for the existence of the dynlink.cma file in the standard library returned by ocamlc -config), the corresponding code is here. If you correctly allow the lookup procedure described above to point to the right ocaml host and build os binaries, everything will work.

@whitequark
Copy link
Copy Markdown
Contributor Author

This is described here with the lookup procedure in which you can hook so that that your binaries point to the right one.

Thanks, the item #4 shows exactly the change that I need to perform:

Cmd.(v "ocamlfind" % "mytool") if "mytool" is part of the OCaml tools that can be invoked through ocamlfind.

This should be trivial to implement now.

@whitequark
Copy link
Copy Markdown
Contributor Author

I'm actually surprised that you didn't try to use this rather than enshrine ocamlfind -toolchain approach which now bleeds into ocamlbuild aswell.

BTW I am very happy about having ocamlfind -toolchain as well as (now) ocamlbuild -toolchain. This makes it far easier to work with the majority of packages, which do not use topkg; for example I think (but haven't checked yet) that with the ocamlbuild patch, cross-compilation could be achieved for oasis packages with a single configure.ml option.

Eventually I would like to generate cross-packages based on host-packages using a simple script instead of the current manual approach. Of course, first-class opam support would be better but I do not spend enough time changing them manually to justify implementing that and the opam team does not appear to have a strong interest in cross-compilation, seeing as to how little discussion ocaml/opam#2476 has generated.

@whitequark
Copy link
Copy Markdown
Contributor Author

@dbuenzli I have updated the PR to address your concern about overriding build_cmd, and verified that it works with my opam-cross-* repositories.

One unresolved question is: what do we do with the ocamlbuild dependency specification? Currently it's a {build} one. In my opinion it is incorrect because topkg invokes ocamlbuild.

  • One option is to just remove {build}. Then, no upgrades will be necessary, and the users of the --toolchain option (which after all is the domain of the repository and not package itself) could add the stricter dependency themselves.
  • Another option is to replace it with {>= 0.11.0} (once 0.11.0 gets released). This is more proper in my view, but would mandate some upgrades.

@dbuenzli
Copy link
Copy Markdown
Owner

Looks right, but could you please document and expose the configuration key to the end users API around here. Also the documentation of the default build command here should be updated.

You are right about the fact that build is inappropriate. I think we should simply drop it and add a {>= 0.11.0}. However which upgrades do you see happening ? Normally topkg packages only build depend on ocamlfind, ocamlbuild and topkg so I don't see the upgrades you see happening, topkg will simply impose a constraint on the version of ocamlbuild which I don't think should be a problem for the packages out there.

@whitequark
Copy link
Copy Markdown
Contributor Author

However which upgrades do you see happening ? Normally topkg packages only build depend on ocamlfind, ocamlbuild and topkg so I don't see the upgrades you see happening, topkg will simply impose a constraint on the version of ocamlbuild which I don't think should be a problem for the packages out there.

I mean existing installs with pre-0.11.0 ocambuild will have to be upgraded, potentially triggering a lot of rebuilds.

@whitequark
Copy link
Copy Markdown
Contributor Author

Looks right, but could you please document and expose the configuration key to the end users API around here. Also the documentation of the default build command here should be updated.

Done.

@dbuenzli
Copy link
Copy Markdown
Owner

Thanks ! Your patch is in as 05685ce. You may want to keep your fork around until a release is done because the current master depends on an unreleased version of cmdliner.

@dbuenzli dbuenzli closed this Feb 27, 2017
@whitequark
Copy link
Copy Markdown
Contributor Author

@dbuenzli ack. There is also no ocamlbuild release to require as an opam dependency, yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Needs a way to pass -toolchain option to ocamlfind

3 participants