Skip to content

Fix bad interaction between env stanza and vendored projects#1408

Merged
3 commits merged intomasterfrom
unknown repository
Oct 8, 2018
Merged

Fix bad interaction between env stanza and vendored projects#1408
3 commits merged intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Oct 5, 2018

When a vendored project didn't have its own env stanza, the env stanza from the enclosing project was in effect. This patch fixes this.

I added a test for this case in the env test and also refactored a bit to make it easier to understand. The patch also moves the filling of the env table in Super_context.t from Super_context.create to Super_context.Env.get. The code is a bit more straightforward and lazy this way.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@ghost ghost requested review from emillon and rgrinberg as code owners October 5, 2018 18:18
@dra27
Copy link
Copy Markdown
Member

dra27 commented Oct 5, 2018

@avsm - I have a feeling this may break the platform build, or at least its propagation of warnings flags?

@avsm
Copy link
Copy Markdown
Member

avsm commented Oct 8, 2018

Yeah, we are using the toplevel env stanza to turn off warnings in vendored projects. It's not a disaster, but it would be nice if there was a way to have a global 'flags' setting. It might also be useful for, e.g., setting flambda flags and such.

@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 8, 2018

I suppose we could add a way to make it clear that the default will cross-project boundaries, such as a cross_project_env stanza.

@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 8, 2018

In the meantime, this PR fixes an inconsistency between projects that have a top-level env stanzas and those that don't, so it should be merged.

@ghost ghost merged commit d762e24 into ocaml:master Oct 8, 2018
@avsm
Copy link
Copy Markdown
Member

avsm commented Oct 8, 2018

yes agree this should be merged -- however it would be good to highlight the change in backwards compatibility prominently as it could cause surprising side-effects as vendored projects shift to using dune-project files from jbuilder (since presumably the flag will stop being propagated now when the dune-project file shows up).

@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 8, 2018

I opened #1419 to discuss alternative possibilities.

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)
This pull request was closed.
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.

4 participants