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.

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

Implement #3432 #3691

Merged
merged 2 commits into from
Sep 4, 2020
Merged

Implement #3432 #3691

merged 2 commits into from
Sep 4, 2020

Conversation

rgrinberg
Copy link
Member

Use a temp dir that's inside the build directory. This makes it easier
to debug builds.

@rgrinberg rgrinberg requested review from a user, NathanReb and nojb August 8, 2020 03:35
@rgrinberg
Copy link
Member Author

I ended up implementing #3432 so that we get the temp dir stuff out of the way once and for all.

@rgrinberg
Copy link
Member Author

Should we add a CHANGES entry? I guess this observable by the user, but the old behavior is not something they should have cared about.

@ghost
Copy link

ghost commented Aug 11, 2020

Use a temp dir that's inside the build directory

There might be a performance concern here: the temporary directory is usually on a tmpfs (so in ram), while the build directory is on disk. Might be worth benching this change.

@ghost
Copy link

ghost commented Aug 11, 2020

Should we add a CHANGES entry? I guess this observable by the user, but the old behavior is not something they should have cared about.

I'd say it's worth it. It should indeed not be visible in theory, however it feels like a change that could be visible in practice. In such cases, it is good if the user can rely on the changelog to quickly get an idea of which version might cause the change in behaviour.

@rgrinberg
Copy link
Member Author

Use a temp dir that's inside the build directory

There might be a performance concern here: the temporary directory is usually on a tmpfs (so in ram), while the build directory is on disk. Might be worth benching this change.

That's a good point. I'm going to change the implementation to just use a directory inside the current TMPDIR to be on the safe side

@rgrinberg
Copy link
Member Author

I'd say it's worth it. It should indeed not be visible in theory, however it feels like a change that could be visible in practice. In such cases, it is good if the user can rely on the changelog to quickly get an idea of which version might cause the change in behaviour.

Alright, I've added a CHANGES entry and preserved the original temp dir set by the user.

src/stdune/path.mli Outdated Show resolved Hide resolved
src/stdune/temp.mli Show resolved Hide resolved
src/stdune/temp.mli Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Aug 12, 2020

What I had in mind initially was to setup a temporary directory for all temporary files created by Dune and the sub-processes started by Dune. Here, we are only capturing the temporary files created by sub-processes, but for instance we are not capturing the ones from the Process module for capturing the output of commands. Putting all the temporary files in a single temporary directory feels a bit cleaner to me.

/cc @aalekseyev who might want to chime on the design/implem

@aalekseyev

This comment has been minimized.

@aalekseyev
Copy link
Collaborator

A single temporary directory seems reasonable to me, but I don't see a big problem if dune uses the ambient $TMPDIR for some things, if using the build dir is awkward (e.g. if we need the temp file to outlive the build dir).

@rgrinberg
Copy link
Member Author

What I had in mind initially was to setup a temporary directory for all temporary files created by Dune and the sub-processes started by Dune. Here, we are only capturing the temporary files created by sub-processes, but for instance we are not capturing the ones from the Process module for capturing the output of commands. Putting all the temporary files in a single temporary directory feels a bit cleaner to me.

Okay, I amended the PR to do this. The only question I have is how should we handle TEMPDIR that is set by the user? The current implementation will not override the user's settings. That seems like a safe default to me.

@aalekseyev
Copy link
Collaborator

I think an invariant that I expect all well-behaved programs to follow is that export TMPDIR=/tmp is ~equivalent to unset TMPDIR. To uphold this, dune should not honor TMPDIR, just the same as it won't be using /tmp by default.

@rgrinberg rgrinberg added this to the 2.8 milestone Aug 13, 2020
@rgrinberg
Copy link
Member Author

@aalekseyev this PR is ready now.

src/stdune/path.ml Outdated Show resolved Hide resolved
src/stdune/temp.ml Outdated Show resolved Hide resolved
Copy link
Collaborator

@aalekseyev aalekseyev left a comment

Choose a reason for hiding this comment

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

The changes look good to me.
Maybe you could add short doc comments to all new functions, though?
(as Jeremie requested before)

@rgrinberg
Copy link
Member Author

Maybe you could add short doc comments to all new functions, though?
(as Jeremie requested before)

Done. I usually prefer to do this at the end as the implementation of these functions is still subject to change during the review.

Use a temp dir that's inside the build directory. This makes it easier
to debug builds.

Signed-off-by: Rudi Grinberg <[email protected]>
We aren't modifying dune's own environment for now

Signed-off-by: Rudi Grinberg <[email protected]>
@rgrinberg rgrinberg merged commit b6c871b into ocaml:master Sep 4, 2020
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jan 13, 2021
…ne-action-plugin, dune-private-libs and dune-glob (2.8.0)

CHANGES:

- `dune rules` accepts aliases and other non-path rules (ocaml/dune#4063, @mrmr1993)

- Action `(diff reference test_result)` now accept `reference` to be absent and
  in that case consider that the reference is empty. Then running `dune promote`
  will create the reference file. (ocaml/dune#3795, @bobot)

- Ignore special files (BLK, CHR, FIFO, SOCKET), (ocaml/dune#3570, fixes ocaml/dune#3124, ocaml/dune#3546,
  @ejgallego)

- Experimental: Simplify loading of additional files (data or code) at runtime
  in programs by introducing specific installation sites. In particular it allow
  to define plugins to be installed in these sites. (ocaml/dune#3104, ocaml/dune#3794, fixes ocaml/dune#1185,
  @bobot)

- Move all temporary files created by dune to run actions to a single directory
  and make sure that actions executed by dune also use this directory by setting
  `TMPDIR` (or `TEMP` on Windows). (ocaml/dune#3691, fixes ocaml/dune#3422, @rgrinberg)

- Fix bootstrap script with custom configuration. (ocaml/dune#3757, fixes ocaml/dune#3774, @marsam)

- Add the `executable` field to `inline_tests` to customize the compilation
  flags of the test runner executable (ocaml/dune#3747, fixes ocaml/dune#3679, @lubegasimon)

- Add `(enabled_if ...)` to `(copy_files ...)` (ocaml/dune#3756, @nojb)

- Make sure Dune cleans up the status line before exiting (ocaml/dune#3767,
  fixes ocaml/dune#3737, @alan-j-hu)

- Add `{gitlab,bitbucket}` as options for defining project sources with `source`
  stanza `(source (<host> user/repo))` in the `dune-project` file.  (ocaml/dune#3813,
  @rgrinberg)

- Fix generation of `META` and `dune-package` files when some targets (byte,
  native, dynlink) are disabled. Previously, dune would generate all archives
  for regardless of settings. (ocaml/dune#3829, ocaml/dune#4041, @rgrinberg)

- Do not run ocamldep to for single module executables & libraries. The
  dependency graph for such artifacts is trivial (ocaml/dune#3847, @rgrinberg)

- Fix cram tests inside vendored directories not being interpreted correctly.
  (ocaml/dune#3860, fixes ocaml/dune#3843, @rgrinberg)

- Add `package` field to private libraries. This allows such libraries to be
  installed and to be usable by other public libraries in the same project
  (ocaml/dune#3655, fixes ocaml/dune#1017, @rgrinberg)

- Fix the `%{make}` variable on Windows by only checking for a `gmake` binary
  on UNIX-like systems as a unrelated `gmake` binary might exist on Windows.
  (ocaml/dune#3853, @kit-ty-kate)

- Fix `$ dune install` modifying the build directory. This made the build
  directory unusable when `$ sudo dune install` modified permissions. (fix
  ocaml/dune#3857, @rgrinberg)

- Fix handling of aliases given on the command line (using the `@` and `@@`
  syntax) so as to correctly handle relative paths. (ocaml/dune#3874, fixes ocaml/dune#3850, @nojb)

- Allow link time code generation to be used in preprocessing executable. This
  makes it possible to use the build info module inside the preprocessor.
  (ocaml/dune#3848, fix ocaml/dune#3848, @rgrinberg)

- Correctly call `git ls-tree` so unicode files are not quoted, this fixes
  problems with `dune subst` in the presence of unicode files. Fixes ocaml/dune#3219
  (ocaml/dune#3879, @ejgallego)

- `dune subst` now accepts common command-line arguments such as
  `--debug-backtraces` (ocaml/dune#3878, @ejgallego)

- `dune describe` now also includes information about executables in addition to
  that of libraries. (ocaml/dune#3892, ocaml/dune#3895, @nojb)

- instrumentation backends can now receive arguments via `(instrumentation
  (backend <name> <args>))`. (ocaml/dune#3906, ocaml/dune#3932, @nojb)

- Tweak auto-formatting of `dune` files to improve readability. (ocaml/dune#3928, @nojb)

- Add a switch argument to opam when context is not default. (ocaml/dune#3951, @tmattio)

- Avoid pager when running `$ git diff` (ocaml/dune#3912, @AltGr)

- Add `(root_module ..)` field to libraries & executables. This makes it
  possible to use library dependencies shadowed by local modules (ocaml/dune#3825,
  @rgrinberg)

- Allow `(formatting ...)` field in `(env ...)` stanza to set per-directory
  formatting specification. (ocaml/dune#3942, @nojb)

- [coq] In `coq.theory`, `:standard` for the `flags` field now uses the
  flags set in `env` profile flags (ocaml/dune#3931 , @ejgallego @rgrinberg)

- [coq] Add `-q` flag to `:standard` `coqc` flags , fixes ocaml/dune#3924, (ocaml/dune#3931 , @ejgallego)

- Add support for Coq's native compute compilation mode (@ejgallego, ocaml/dune#3210)

- Add a `SUFFIX` directive in `.merlin` files for each dialect with no
  preprocessing, to let merlin know of additional file extensions (ocaml/dune#3977,
  @vouillon)

- Stop promoting `.merlin` files. Write per-stanza Merlin configurations in
  binary form. Add a new subcommand `dune ocaml-merlin` that Merlin can use to
  query the configuration files. The `allow_approximate_merlin` option is now
  useless and deprecated. Dune now conflicts with `merlin < 3.4.0` and
  `ocaml-lsp-server < 1.3.0` (ocaml/dune#3554, @voodoos)

- Configurator: fix a bug introduced in 2.6.0 where the configurator V1 API
  doesn't work at all when used outside of dune. (ocaml/dune#4046, @aalekseyev)

- Fix `libexec` and `libexec-private` variables. In cross-compilation settings,
  they now point to the file in the host context. (ocaml/dune#4058, fixes ocaml/dune#4057,
  @TheLortex)

- When running `$ dune subst`, use project metadata as a fallback when package
  metadata is missing. We also generate a warning when `(name ..)` is missing in
  `dune-project` files to avoid failures in production builds.

- Remove support for passing `-nodynlink` for executables. It was bypassed in
  most cases and not correct in other cases in particular on arm32.
  (ocaml/dune#4085, fixes ocaml/dune#4069, fixes ocaml/dune#2527, @emillon)

- Generate archive rules compatible with 4.12. Dune longer attempt to generate
  an archive file if it's unnecessary (ocaml/dune#3973, fixes ocaml/dune#3766, @rgrinberg)

- Fix generated Merlin configurations when multiple preprocessors are defined
  for different modules in the same folder. (ocaml/dune#4092, fixes ocaml/dune#2596, ocaml/dune#1212 and
  ocaml/dune#3409, @voodoos)

- Add the option `use_standard_c_and_cxx_flags` to `dune-project` that 1.
  disables the unconditional use of the `ocamlc_cflags` and `ocamlc_cppflags`
  from `ocamlc -config` in C compiler calls, these flags will be present in the
  `:standard` set instead; and 2. enables the detection of the C compiler family
  and populates the `:standard` set of flags with common default values when
  building CXX stubs. (ocaml/dune#3875, ocaml/dune#3802, fix ocaml/dune#3718 and ocaml/dune#3528, @voodoos)
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.

2 participants