Do not fail if standard_runtime is missing in the output of ocamlc -config
#1326
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
ghost
requested review from
|
It was removed in the trunk of OCaml |
ghost
mentioned this pull request
|
Jérémie Dimino (2018/09/25 04:44 -0700):
It was removed in the trunk of OCaml
Well it's not merged yet, but many thanks!!
|
|
Thank you so much, @diml!!
|
emillon
reviewed
Sep 25, 2018
src/ocaml-config/ocaml_config.ml
Outdated
| @@ -340,7 +340,9 @@ let make vars = | |||
| let os_type = get vars "os_type" in | |||
| let standard_library_default = get vars "standard_library_default" in | |||
| let standard_library = get vars "standard_library" in | |||
| let standard_runtime = get vars "standard_runtime" in | |||
| let standard_runtime = | |||
| Option.value (get_opt vars "standard_runtime") ~default:"-" | |||
There was a problem hiding this comment.
is "-" a valid value, or is this just a placeholder? (ie: should this be an option instead?)
There was a problem hiding this comment.
It should probably be an option yeah. It's probably better to fail if the user tries to use this variable.
There was a problem hiding this comment.
Yes, I also think it would be best to fail. Ideally, it would be best for the error message to be useful as well - tell the user the variable has been removed.
There was a problem hiding this comment.
Actually, I think that this variable is obscure enough that we can just remove it. Technically, it's a breaking change, but if nobody is using it on opam (should be easy enough to grep) we don't have to bother.
There was a problem hiding this comment.
I just updated the default value to make things more explicit. Other variables might get removed in the furture, so we should have a general mechanism to deal with it, but for now this should do.
emillon
approved these changes
Sep 25, 2018
|
Since this is public API, I think it'd be easier if these were functions that we could deprecate one by one, rather than a public record, but we can do that later. |
|
We can consider that this API is still private, the ocamlfind name is |
|
Rudi Grinberg (2018/09/25 08:27 -0700):
rgrinberg commented on this pull request.
> @@ -340,7 +340,9 @@ let make vars =
let os_type = get vars "os_type" in
let standard_library_default = get vars "standard_library_default" in
let standard_library = get vars "standard_library" in
- let standard_runtime = get vars "standard_runtime" in
+ let standard_runtime =
+ Option.value (get_opt vars "standard_runtime") ~default:"-"
Actually, I think that this variable is obscure enough that we can
just remove it. Technically, it's a breaking change, but if nobody is
using it on opam (should be easy enough to grep) we don't have to
bother.
I did this grepping, see the discussion on OCaml's GPR #1992.
|
rgrinberg
added a commit
to rgrinberg/opam-repository
that referenced
this pull request
Oct 4, 2018
CHANGES: - Do not fail if the output of `ocamlc -config` doesn't include `standard_runtime` (ocaml/dune#1326, @diml) - Let `Configurator.V1.C_define.import` handle negative integers (ocaml/dune#1334, @Chris00) - Re-execute actions when a target is modified by the user inside `_build` (ocaml/dune#1343, fix ocaml/dune#1342, @diml) - Pass `--set-switch` to opam (ocaml/dune#1341, fix ocaml/dune#1337, @diml) - Fix bad interaction between multi-directory libraries the `menhir` stanza (ocaml/dune#1373, fix ocaml/dune#1372, @diml) - Integration with automatic formatters (ocaml/dune#1252, fix ocaml/dune#1201, @emillon) - Better error message when using `(self_build_stubs_archive ...)` and `(c_names ...)` or `(cxx_names ...)` simultaneously. (ocaml/dune#1375, fix ocaml/dune#1306, @nojb) - Improve name detection for packages when the prefix isn't an actual package (ocaml/dune#1361, fix ocaml/dune#1360, @rgrinberg) - Support for new menhir rules (ocaml/dune#863, fix ocaml/dune#305, @fpottier, @rgrinberg) - Do not remove flags when compiling compatibility modules for wrapped mode (ocaml/dune#1382, fix ocaml/dune#1364, @rgrinberg) - Fix reason support when using `staged_pps` (ocaml/dune#1384, @charlesetc) - Add support for `enabled_if` in `rule`, `menhir`, `ocamllex`, `ocamlyacc` (ocaml/dune#1387, @diml) - Exit gracefully when a signal is received (ocaml/dune#1366, @diml) - Load all defined libraries recursively into utop (ocaml/dune#1384, fix ocaml/dune#1344, @rgrinberg) - Allow to use libraries `bytes`, `result` and `uchar` without `findlib` installed (ocaml/dune#1391, @nojb)
rgrinberg
added a commit
to rgrinberg/opam-repository
that referenced
this pull request
Oct 10, 2018
CHANGES: - Do not fail if the output of `ocamlc -config` doesn't include `standard_runtime` (ocaml/dune#1326, @diml) - Let `Configurator.V1.C_define.import` handle negative integers (ocaml/dune#1334, @Chris00) - Re-execute actions when a target is modified by the user inside `_build` (ocaml/dune#1343, fix ocaml/dune#1342, @diml) - Pass `--set-switch` to opam (ocaml/dune#1341, fix ocaml/dune#1337, @diml) - Fix bad interaction between multi-directory libraries the `menhir` stanza (ocaml/dune#1373, fix ocaml/dune#1372, @diml) - Integration with automatic formatters (ocaml/dune#1252, fix ocaml/dune#1201, @emillon) - Better error message when using `(self_build_stubs_archive ...)` and `(c_names ...)` or `(cxx_names ...)` simultaneously. (ocaml/dune#1375, fix ocaml/dune#1306, @nojb) - Improve name detection for packages when the prefix isn't an actual package (ocaml/dune#1361, fix ocaml/dune#1360, @rgrinberg) - Support for new menhir rules (ocaml/dune#863, fix ocaml/dune#305, @fpottier, @rgrinberg) - Do not remove flags when compiling compatibility modules for wrapped mode (ocaml/dune#1382, fix ocaml/dune#1364, @rgrinberg) - Fix reason support when using `staged_pps` (ocaml/dune#1384, @charlesetc) - Add support for `enabled_if` in `rule`, `menhir`, `ocamllex`, `ocamlyacc` (ocaml/dune#1387, @diml) - Exit gracefully when a signal is received (ocaml/dune#1366, @diml) - Load all defined libraries recursively into utop (ocaml/dune#1384, fix ocaml/dune#1344, @rgrinberg) - Allow to use libraries `bytes`, `result` and `uchar` without `findlib` installed (ocaml/dune#1391, @nojb) - Take argument to self_build_stubs_archive into account. (ocaml/dune#1395, @nojb) - Fix bad interaction between `env` customization and vendored projects: when a vendored project didn't have its own `env` stanza, the `env` stanza from the enclosing project was in effect (ocaml/dune#1408, @diml) - Fix stop early bug when scanning for watermarks (ocaml/dune#1423, @diml)
shonfeder
pushed a commit
to shonfeder/dune
that referenced
this pull request
Dec 31, 2018
…config` (ocaml#1326) Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
/cc @shindere