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

PR#7172: ocamlc -config: add int_size, word_size, natdynlink, ext_exe #970

Merged
merged 1 commit into from Dec 14, 2016

Conversation

Projects
None yet
5 participants
@gasche
Member

gasche commented Dec 13, 2016

See MPR#7172.

Among the options that @dbuenzli asked, one that is not included is "nativecomp", which he specified as "whether native compilation is available". First, I'm not completely sure how to implement that (there is no obvious makefile variable I can reuse), and second it is unclear to me that we actually can give it a specification: config.mlp tells about how the compiler distribution was configured to be built, but not about what was actually installed on the user machine, so while I can say if native-compilation was disabled at configure-time, if I say true at config-time you still have to check that ocamlopt is actually installed to be sure.

Show outdated Hide outdated Makefile
@@ -423,10 +423,12 @@ utils/config.ml: utils/config.mlp config/Makefile
-e 's|%%ARCH%%|$(ARCH)|' \
-e 's|%%MODEL%%|$(MODEL)|' \
-e 's|%%SYSTEM%%|$(SYSTEM)|' \
-e 's|%%EXT_EXE%%||' \

This comment has been minimized.

@dra27

dra27 Dec 14, 2016

Contributor

I'm fairly sure this logic isn't correct - Cygwin should have .exe and possibly also Linux cross-compiling using mingw? (both of which use configure, not Makefile.nt)

@dra27

dra27 Dec 14, 2016

Contributor

I'm fairly sure this logic isn't correct - Cygwin should have .exe and possibly also Linux cross-compiling using mingw? (both of which use configure, not Makefile.nt)

This comment has been minimized.

@gasche

gasche Dec 14, 2016

Member

That's a good point, I'll change it to be whatever configure decided it would use for the installed binaries.

@gasche

gasche Dec 14, 2016

Member

That's a good point, I'll change it to be whatever configure decided it would use for the installed binaries.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Dec 14, 2016

Contributor

The "proper" way would be to alter configure so that native code compilation becomes effectively part of make world (i.e. no opt or opt.opt stages) but that would require a lot of care for bootstrapping, I think (though possibly no more than requiring one to disable native code compilation via configure specifically when bootstrapping). One for @shindere, I expect!

A more compatible, though time-consuming, way would be to alter config.mlp during make opt, but that would mean that make opt would need to recompile the bytecode compilers, which is unfortunate.

Contributor

dra27 commented Dec 14, 2016

The "proper" way would be to alter configure so that native code compilation becomes effectively part of make world (i.e. no opt or opt.opt stages) but that would require a lot of care for bootstrapping, I think (though possibly no more than requiring one to disable native code compilation via configure specifically when bootstrapping). One for @shindere, I expect!

A more compatible, though time-consuming, way would be to alter config.mlp during make opt, but that would mean that make opt would need to recompile the bytecode compilers, which is unfortunate.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Dec 14, 2016

Contributor

Actually, a third possibility for compatibility would be to have make install fail if ocaml is configured to build the native code compiler but you haven't. That sounds much more straightforward?

Contributor

dra27 commented Dec 14, 2016

Actually, a third possibility for compatibility would be to have make install fail if ocaml is configured to build the native code compiler but you haven't. That sounds much more straightforward?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 14, 2016

Member

I think your last suggestion is excellent, but I would like to keep nativecomp out of the scope of the present PR for simplicity. Could you post it on the mantis discussion?

Member

gasche commented Dec 14, 2016

I think your last suggestion is excellent, but I would like to keep nativecomp out of the scope of the present PR for simplicity. Could you post it on the mantis discussion?

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Dec 14, 2016

Contributor

Could we keep natdynlink of it for the same reason?

Contributor

dra27 commented Dec 14, 2016

Could we keep natdynlink of it for the same reason?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 14, 2016

Member

This is a reasonable argument, so I removed natdynlink for now.

Member

gasche commented Dec 14, 2016

This is a reasonable argument, so I removed natdynlink for now.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Dec 14, 2016

Contributor

This (unsurprisingly) all looks fine - anything to add, @dbuenzli?

Merge once the CI confirms? I'm happy to be assigned the rest of PR to be ready for 4.05, unless you have a burning desire to do it? It would be good (both philosophically and practically) not to be relying on tools such as OPAM to determine the installed state of the compiler.

Contributor

dra27 commented Dec 14, 2016

This (unsurprisingly) all looks fine - anything to add, @dbuenzli?

Merge once the CI confirms? I'm happy to be assigned the rest of PR to be ready for 4.05, unless you have a burning desire to do it? It would be good (both philosophically and practically) not to be relying on tools such as OPAM to determine the installed state of the compiler.

@gasche gasche merged commit d89b1a7 into ocaml:trunk Dec 14, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 14, 2016

Member

Thanks, I went ahead and merged.

I don't plan on working on the rest of the PR before 4.05, so you should feel free to do so if you wish. (Also I'm not comfortable hacking on the build system and I entertain the wishful thought that others such as you may enjoy it more.) That said, please be aware that this mantis issue is not high-priority, it's a nice-to-have: feel free to assign yourself for task-tracking purposes, but don't feel bad if you slip the 4.05 deadline.

Member

gasche commented Dec 14, 2016

Thanks, I went ahead and merged.

I don't plan on working on the rest of the PR before 4.05, so you should feel free to do so if you wish. (Also I'm not comfortable hacking on the build system and I entertain the wishful thought that others such as you may enjoy it more.) That said, please be aware that this mantis issue is not high-priority, it's a nice-to-have: feel free to assign yourself for task-tracking purposes, but don't feel bad if you slip the 4.05 deadline.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 14, 2016

Contributor
Contributor

shindere commented Dec 14, 2016

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Dec 14, 2016

Contributor

The only question is what happens if a bootstrap cycle is required? make world.opt would normally fail at this point because you should run make world bootstrap [bootstrap] opt (I think - I still don't fully have my head wrapped around that part of the build system!)

Contributor

dra27 commented Dec 14, 2016

The only question is what happens if a bootstrap cycle is required? make world.opt would normally fail at this point because you should run make world bootstrap [bootstrap] opt (I think - I still don't fully have my head wrapped around that part of the build system!)

@dbuenzli

This comment has been minimized.

Show comment
Hide comment
@dbuenzli

dbuenzli Dec 15, 2016

Contributor

Re nativecomp

First, I'm not completely sure how to implement that (there is no obvious makefile variable I can reuse),

@gasche As you mentioned you need a configuration variable to disable the construction of native code compilers for that.

config.mlp tells about how the compiler distribution was configured to be built,

I think we can reasonably devise a system where what you configure to be built coincides with what the default install procedure will install.

if I say true at config-time you still have to check that ocamlopt is actually installed to be sure.

Or not, you may be only interested in e.g. compiler-libs libraries. Checking for binaries is very brittle. For example checking the availability of ocamlopt doesn't work if you have a byte-code only switch and a system compiler installed with native code.

FWIW to determine this I currently check for the availability of libasmrun in the standard library directory reported by ocamlc -config.

Contributor

dbuenzli commented Dec 15, 2016

Re nativecomp

First, I'm not completely sure how to implement that (there is no obvious makefile variable I can reuse),

@gasche As you mentioned you need a configuration variable to disable the construction of native code compilers for that.

config.mlp tells about how the compiler distribution was configured to be built,

I think we can reasonably devise a system where what you configure to be built coincides with what the default install procedure will install.

if I say true at config-time you still have to check that ocamlopt is actually installed to be sure.

Or not, you may be only interested in e.g. compiler-libs libraries. Checking for binaries is very brittle. For example checking the availability of ocamlopt doesn't work if you have a byte-code only switch and a system compiler installed with native code.

FWIW to determine this I currently check for the availability of libasmrun in the standard library directory reported by ocamlc -config.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Dec 15, 2016

Contributor

Regarding the brittleness - the ocaml autoconf macros also check that ocamlopt and ocamlc use the same standard library directory. Though this is not perfect (OCAMLLIB can get in the way here), it does help eliminate the real problem of ocamlopt being detected from a different ocaml installation. You could, with excessive paranoia, also check that, say, pervasives.cmi has the correct magic numbers for the compiler.

Contributor

dra27 commented Dec 15, 2016

Regarding the brittleness - the ocaml autoconf macros also check that ocamlopt and ocamlc use the same standard library directory. Though this is not perfect (OCAMLLIB can get in the way here), it does help eliminate the real problem of ocamlopt being detected from a different ocaml installation. You could, with excessive paranoia, also check that, say, pervasives.cmi has the correct magic numbers for the compiler.

@@ -36,6 +36,10 @@ Next version (4.05.0):
and Damien Doligez, discussion with Alain Frisch and Xavier Leroy,
feature request from the Coq team)
- PR#7172, GPR#970: add extra (ocamlc -config) options
int_size, word_size, ext_exe
(Gabriel Scherer, request by Daniel Buenzli)

This comment has been minimized.

@murmour

murmour Feb 25, 2017

Contributor

Gabriel, you have lost Mr. Bünzli's umlaut. I guess he's too modest to complain. Once I sign the CLA, I'll fix this sacrilege.

@murmour

murmour Feb 25, 2017

Contributor

Gabriel, you have lost Mr. Bünzli's umlaut. I guess he's too modest to complain. Once I sign the CLA, I'll fix this sacrilege.

This comment has been minimized.

@dra27

dra27 Feb 25, 2017

Contributor

You don't need a CLA for tiny changes!

However, it's not lost - ue is a permitted alternative (note Daniel's GitHub handle)

@dra27

dra27 Feb 25, 2017

Contributor

You don't need a CLA for tiny changes!

However, it's not lost - ue is a permitted alternative (note Daniel's GitHub handle)

This comment has been minimized.

@gasche

gasche Feb 25, 2017

Member

I actually like umlaüten, so Bünzli is the spelling I tend to use, no idea why this one was different. There are also two clearly wrong "Bunzli" in the Changes file (I think it is Alain's fault), so we could do a pass to harmonize this. I'll have a look at whether other names should be fixed as well.

@gasche

gasche Feb 25, 2017

Member

I actually like umlaüten, so Bünzli is the spelling I tend to use, no idea why this one was different. There are also two clearly wrong "Bunzli" in the Changes file (I think it is Alain's fault), so we could do a pass to harmonize this. I'll have a look at whether other names should be fixed as well.

This comment has been minimized.

@murmour

murmour Feb 25, 2017

Contributor

I've fixed several here (including this one): 7dc4993.

@murmour

murmour Feb 25, 2017

Contributor

I've fixed several here (including this one): 7dc4993.

This comment has been minimized.

@gasche

gasche Feb 25, 2017

Member

Well, getting useful commits stuck in an ambitious pull request is bad. I can cherry-pick and go from there -- I just did a pass on the Changelog.

@gasche

gasche Feb 25, 2017

Member

Well, getting useful commits stuck in an ambitious pull request is bad. I can cherry-pick and go from there -- I just did a pass on the Changelog.

This comment has been minimized.

@gasche

gasche Feb 25, 2017

Member

In fact your commit is tangled with a Changes entry, so I won't cherry-pick it -- I had done the same changes independently, plus some.

@gasche

gasche Feb 25, 2017

Member

In fact your commit is tangled with a Changes entry, so I won't cherry-pick it -- I had done the same changes independently, plus some.

This comment has been minimized.

@gasche

gasche Feb 25, 2017

Member

Done: e65050e.

@gasche

gasche added a commit that referenced this pull request Feb 25, 2017

gasche added a commit that referenced this pull request Feb 25, 2017

gasche added a commit that referenced this pull request Mar 9, 2017

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

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