Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add DUNE_WORKSPACE env var #1711

Merged
merged 1 commit into from
Feb 17, 2019
Merged

Add DUNE_WORKSPACE env var #1711

merged 1 commit into from
Feb 17, 2019

Conversation

rgrinberg
Copy link
Member

This variable is equivalent to setting --workspace.

Fix #1053

@andreypopp
Copy link
Member

Had a discussion about this feature with @rgrinberg.

Some background: when calling dune from inside esy environment we want all invocations to have the following arguments passed:

  • --only-packages $cur__name where $cur__name is the name of the package currently being built, so dune builds only the current package and won't try to build others (others are provided by esy via ocamlfind config).
  • --root . so dune won't try to climb up

This is not just about an invocation to build a package in release mode (dune build -p $cur__name is fine). In dev mode we want

esy dune build --only-packages $cur__name --root .

and then for any other dune COMMAND we want

esy dune COMMAND --only-packages $cur__name --root .

We really don't want to make users specify this long command lines.

@rgrinberg suggested that DUNE_WORKSPACE could help with that if it would allow to configure those two things above. Thus the feature request — could dune make --root and --only-packages configurable via dune-workspace?

If DUNE_WORKSPACE would be useful for esy then we want to generate it and supply to build env. In this case it would be more ergonomically if we could put the actual config value inside DUNE_WORKSPACE not the filename of the file which contains the config value. Just not to deal with on-the-fly generated files.

@ghost
Copy link

ghost commented Jan 7, 2019

Thus the feature request — could dune make --root and --only-packages configurable via dune-workspace?

Making --root configurable via the dune-workspace file might be a bit annoying as we decide what root is quite early. We could add an environment variable for it though. Allowing --only-packages in the dune-workspace file might work.

If DUNE_WORKSPACE would be useful for esy then we want to generate it and supply to build env. In this case it would be more ergonomically if we could put the actual config value inside DUNE_WORKSPACE not the filename of the file which contains the config value. Just not to deal with on-the-fly generated files.

That seems fine to me.

@rgrinberg
Copy link
Member Author

Not having --root configurable via the workspace file seems fine to me. @andreypopp are you ok with:

  • Have DUNE_WORKSPACE take an sexp value. I think we can also allow users to provide paths if they prefix the value with @. I.e. DUNE_WORKSPACE=@/path/to/dune.workspace.

  • Have --root configurable only via the command line.

@rgrinberg rgrinberg mentioned this pull request Jan 15, 2019
Merged
@rgrinberg
Copy link
Member Author

@andreypopp Is it correct to say that you don't really need this feature and don't plan on relying on it in esy?

@rgrinberg
Copy link
Member Author

In which case I will revert the feature to its original design. Taking a file path is more symmetric with how the command line works anyway.

@rgrinberg
Add DUNE_WORKSPACE env var
a352d12 
This variable is equivalent to setting --workspace.

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
Copy link
Member Author

I'm including this PR as it was designed originally. To be honest, even if esy needs this feature we'd be better if they just generated a file. As mentioned before, the error messages will be better.

@rgrinberg rgrinberg merged commit d8ba69c into ocaml:master Feb 17, 2019
@rgrinberg rgrinberg deleted the env-dune-workspace branch February 17, 2019 10:09
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Mar 7, 2019
@rgrinberg
[new release] dune (1.8.0)
234e8d2 
CHANGES:

- Clean up watch mode polling loop: improves signal handling and error handling
  during polling (ocaml/dune#1912, fix ocaml/dune#1907, fix ocaml/dune#1671, @aalekseyev)

- Change status messages during polling to be one-line, so that the messages are
  correctly erased by ^K. (ocaml/dune#1912, @aalekseyev)

- Add support for `.cxx` extension for C++ stubs (ocaml/dune#1831, @rgrinberg)

- Add `DUNE_WORKSPACE` variable. This variable is equivalent to setting
  `--workspace` in the command line. (ocaml/dune#1711, fix ocaml/dune#1503, @rgrinberg)

- Add `c_flags` and `cxx_flags` to env profile settings (ocaml/dune#1700 and ocaml/dune#1800,
  @gretay-js)

- Format `dune printenv` output (ocaml/dune#1867, fix ocaml/dune#1862, @emillon)

- Add the `(promote-into <dir>)` and `(promote-until-clean-into
  <dir>)` modes for `(rule ...)` stanzas, so that files can be
  promoted in another directory than the current one. For instance,
  this is used in merlin to promote menhir generated files in a
  directory that depends on the version of the compiler (ocaml/dune#1890, @diml)

- Improve error message when `dune subst` fails (ocaml/dune#1898, fix ocaml/dune#1897, @rgrinberg)

- Add more GC counters to catapult traces (fix908, @rgrinberg)

- Add a preprocessor shim for the `let+` syntax of OCaml 4.08 (ocaml/dune#1899,
  implements ocaml/dune#1891, @diml)

- Fix generation of `.merlin` files on Windows. `\` characters needed
  to be escaped (ocaml/dune#1869, @mlasson)

- Fix 0 error code when `$ dune format-dune-file` fails. (ocaml/dune#1915, fix ocaml/dune#1914,
  @rgrinberg)

- Configurator: deprecated `query_expr` and introduced `query_expr_err` which is
  the same but with a better error in case it fails. (ocaml/dune#1886, @ejgallego)
@rgrinberg rgrinberg mentioned this pull request Mar 7, 2019
Closed
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Mar 7, 2019
@rgrinberg
[new release] dune (1.8.0)
e0b2348 
CHANGES:

- Clean up watch mode polling loop: improves signal handling and error handling
  during polling (ocaml/dune#1912, fix ocaml/dune#1907, fix ocaml/dune#1671, @aalekseyev)

- Change status messages during polling to be one-line, so that the messages are
  correctly erased by ^K. (ocaml/dune#1912, @aalekseyev)

- Add support for `.cxx` extension for C++ stubs (ocaml/dune#1831, @rgrinberg)

- Add `DUNE_WORKSPACE` variable. This variable is equivalent to setting
  `--workspace` in the command line. (ocaml/dune#1711, fix ocaml/dune#1503, @rgrinberg)

- Add `c_flags` and `cxx_flags` to env profile settings (ocaml/dune#1700 and ocaml/dune#1800,
  @gretay-js)

- Format `dune printenv` output (ocaml/dune#1867, fix ocaml/dune#1862, @emillon)

- Add the `(promote-into <dir>)` and `(promote-until-clean-into
  <dir>)` modes for `(rule ...)` stanzas, so that files can be
  promoted in another directory than the current one. For instance,
  this is used in merlin to promote menhir generated files in a
  directory that depends on the version of the compiler (ocaml/dune#1890, @diml)

- Improve error message when `dune subst` fails (ocaml/dune#1898, fix ocaml/dune#1897, @rgrinberg)

- Add more GC counters to catapult traces (fix908, @rgrinberg)

- Add a preprocessor shim for the `let+` syntax of OCaml 4.08 (ocaml/dune#1899,
  implements ocaml/dune#1891, @diml)

- Fix generation of `.merlin` files on Windows. `\` characters needed
  to be escaped (ocaml/dune#1869, @mlasson)

- Fix 0 error code when `$ dune format-dune-file` fails. (ocaml/dune#1915, fix ocaml/dune#1914,
  @rgrinberg)

- Configurator: deprecated `query_expr` and introduced `query_expr_err` which is
  the same but with a better error in case it fails. (ocaml/dune#1886, @ejgallego)

- Make sure `(menhir (mode promote) ...)` stanzas are ignored when
  using `--ignore-promoted-rules` or `-p` (ocaml/dune#1917, @diml)
@rgrinberg rgrinberg mentioned this pull request Mar 7, 2019
Merged
@rgrinberg rgrinberg restored the env-dune-workspace branch April 20, 2019 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rgrinberg @andreypopp