Skip to content

Conversation

infinity0
Copy link
Contributor

Otherwise the tests fail on architectures without ocamlopt, e.g. https://buildd.debian.org/status/fetch.php?pkg=ocamlbuild&arch=mips64el&ver=0.11.0-1&stamp=1501347472&raw=0

Reading Makefile.config to check ocamlopt existence, is not the most elegant; I'd be happy to change if there's a better way.

let rec loop () =
try
let line = String.trim (input_line chan) in
let len = String.length line in
Copy link
Member

Choose a reason for hiding this comment

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

This can all be replaced with line = "OCAML_NATIVE=true", can't it?

@gasche
Copy link
Member

gasche commented Jul 30, 2017

Another way to do this, which I personally think would be nicer, would be to call #mod_use "../src/ocamlbuil_config.ml";; to get configuration values.
But then there is the issue that OCAML_NATIVE is not currently exported in this config file, so you would need to add a line for this in configure.make. I think that having two extra declarations, ocaml_native and ocaml_native_tools, would be fine -- long-term it would be nice to reorder ocamlbuild_config to follow the more regular Makefile.config structure, but that is for another PR.

@infinity0
Copy link
Contributor Author

Ah yes that is nicer, I did not notice src/ocamlbuild_config.ml before. I've updated the commit.

@gasche
Copy link
Member

gasche commented Jul 31, 2017

Very nice, thank you. Before I merge, would you like to add a change entry in the Changes file for the next release? I think the credit line "(Ximin Luo, review by whitequark and Gabriel Scherer)" would be fine.

@infinity0
Copy link
Contributor Author

Changes updated.

@gasche gasche merged commit 556c0d3 into ocaml:master Aug 1, 2017
@gasche
Copy link
Member

gasche commented Aug 1, 2017

Thanks! Merged.

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.

3 participants