Delay opening redirected files until execing cmd by jonludlam · Pull Request #1635 · ocaml/dune

jonludlam · 2018-12-11T09:46:14Z

No description provided.

rgrinberg

Looks good. A CHANGES entry is appropriate as well.

rgrinberg · 2018-12-11T10:18:33Z

src/action_exec.ml

    (match stdout_to with
     | None -> print_string str; flush stdout
-     | Some (_, oc) -> output_string oc str)
+     | Some fn -> Io.with_file_out fn ~f:(fun oc -> output_string oc str))


This can be simplified to use Io.write_file now.

rgrinberg · 2018-12-11T10:21:06Z

src/action_exec.ml

 and redirect outputs fn t ~ectx ~dir ~env ~stdout_to ~stderr_to =
-  let oc = Io.open_out fn in
-  let out = Some (fn, oc) in
+  let abs = Path.to_absolute fn in


Why this is necessary deserves a comment.

Mostly just paranoia in the presence of 'Chdir' as an action. Dune itself still builds if we don't resolve the path at this point, but I didn't want to assume that everything else will. If you think it's unnecessary I'll remove it, or we can keep it in and stick a comment there.

A comment would be useful here I think. Thanks.

This avoids an issue where the redirected output would be opened before there was a slot available to execute a command, consuming fds unnecessarily. Issue ocaml#1633 was an instance of this hitting the default limit on MacOS of 256 fds. Fixes: ocaml#1633 Signed-off-by: Jon Ludlam <jon@recoil.org>

Signed-off-by: Jon Ludlam <jon@recoil.org>

ghost · 2018-12-11T22:47:51Z

This doesn't quite work, for instance this action would now not produce the expected result:

(with-stdout-to file
 (progn
  (echo foo)
  (echo bar)))

This reverts commit 59c4daa. Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

Revert #1635 + implement another fix for #1633. When executing actions: - open files as late as possible - close them as soon as possible This ensures that fds stay open for the least amount of time and helps reduce the maximum number of fds opened by Dune. Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com> Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

@bobot

CHANGES: - Fix merlin handling of private module visibility (ocaml/dune#1653 @bobot) - skip directories when looking up programs in the PATH (ocaml/dune#1628, fixes ocaml/dune#1616, @diml) - Fix preprocessing for libraries with `(include_subdirs ..)` (ocaml/dune#1624, fix ocaml/dune#1626, @nojb, @rgrinberg) - Do not generate targets for archive that don't match the `modes` field. (ocaml/dune#1632, fix ocaml/dune#1617, @rgrinberg) - When executing actions, open files lazily and close them as soon as possible in order to reduce the maximum number of file descriptors opened by Dune (ocaml/dune#1635, ocaml/dune#1643, fixes ocaml/dune#1633, @jonludlam, @rgrinberg, @diml) - Do not generate targets for archive that don't match the `modes` field. (ocaml/dune#1632, fix ocaml/dune#1617, @rgrinberg) - Get the correct environment node for multi project workspaces (ocaml/dune#1648, @rgrinberg)

@bobot

CHANGES: - Fix merlin handling of private module visibility (ocaml/dune#1653 @bobot) - skip directories when looking up programs in the PATH (ocaml/dune#1628, fixes ocaml/dune#1616, @diml) - Fix preprocessing for libraries with `(include_subdirs ..)` (ocaml/dune#1624, fix ocaml/dune#1626, @nojb, @rgrinberg) - Do not generate targets for archive that don't match the `modes` field. (ocaml/dune#1632, fix ocaml/dune#1617, @rgrinberg) - When executing actions, open files lazily and close them as soon as possible in order to reduce the maximum number of file descriptors opened by Dune (ocaml/dune#1635, ocaml/dune#1643, fixes ocaml/dune#1633, @jonludlam, @rgrinberg, @diml) - Do not generate targets for archive that don't match the `modes` field. (ocaml/dune#1632, fix ocaml/dune#1617, @rgrinberg) - Get the correct environment node for multi project workspaces (ocaml/dune#1648, @rgrinberg) - Fix an issue causing menhir generated parsers to fail to build in some cases. The fix is to systematically use `-short-paths` when calling `ocamlc -i` (ocaml/dune#1743, fix ocaml/dune#1504, @diml)

@bobot

CHANGES: - Fix merlin handling of private module visibility (ocaml/dune#1653 @bobot) - skip directories when looking up programs in the PATH (ocaml/dune#1628, fixes ocaml/dune#1616, @diml) - Fix preprocessing for libraries with `(include_subdirs ..)` (ocaml/dune#1624, fix ocaml/dune#1626, @nojb, @rgrinberg) - Do not generate targets for archive that don't match the `modes` field. (ocaml/dune#1632, fix ocaml/dune#1617, @rgrinberg) - When executing actions, open files lazily and close them as soon as possible in order to reduce the maximum number of file descriptors opened by Dune (ocaml/dune#1635, ocaml/dune#1643, fixes ocaml/dune#1633, @jonludlam, @rgrinberg, @diml) - Do not generate targets for archive that don't match the `modes` field. (ocaml/dune#1632, fix ocaml/dune#1617, @rgrinberg) - Get the correct environment node for multi project workspaces (ocaml/dune#1648, @rgrinberg) - Fix an issue causing menhir generated parsers to fail to build in some cases. The fix is to systematically use `-short-paths` when calling `ocamlc -i` (ocaml/dune#1743, fix ocaml/dune#1504, @diml)

@diml

CHANGES: - Second step of the deprecation of jbuilder: the `jbuilder` binary now emits a warning on every startup and both `jbuilder` and `dune` emit warnings when encountering `jbuild` files (ocaml/dune#1752, @diml) - Change the layout of build artifacts inside _build. The new layout enables optimizations that depend on the presence of `.cmx` files of private modules (ocaml/dune#1676, @bobot) - Fix merlin handling of private module visibility (ocaml/dune#1653 @bobot) - unstable-fmt: use boxes to wrap some lists (ocaml/dune#1608, fix ocaml/dune#1153, @emillon, thanks to @rgrinberg) - skip directories when looking up programs in the PATH (ocaml/dune#1628, fixes ocaml/dune#1616, @diml) - Use `lsof` on macOS to implement `--stats` (ocaml/dune#1636, fixes ocaml/dune#1634, @xclerc) - Generate `dune-package` files for every package. These files are installed and read instead of `META` files whenever they are available (ocaml/dune#1329, @rgrinberg) - Fix preprocessing for libraries with `(include_subdirs ..)` (ocaml/dune#1624, fix ocaml/dune#1626, @nojb, @rgrinberg) - Do not generate targets for archive that don't match the `modes` field. (ocaml/dune#1632, fix ocaml/dune#1617, @rgrinberg) - When executing actions, open files lazily and close them as soon as possible in order to reduce the maximum number of file descriptors opened by Dune (ocaml/dune#1635, ocaml/dune#1643, fixes ocaml/dune#1633, @jonludlam, @rgrinberg, @diml) - Reimplement the core of Dune using a new generic memoization system (ocaml/dune#1489, @rudihorn, @diml) - Replace the broken cycle detection algorithm by a state of the art one from [this paper](https://doi.org/10.1145/2756553) (ocaml/dune#1489, @rudihorn) - Get the correct environment node for multi project workspaces (ocaml/dune#1648, @rgrinberg) - Add `dune compute` to call internal memoized functions (ocaml/dune#1528, @rudihorn, @diml) - Add `--trace-file` option to trace dune internals (ocaml/dune#1639, fix ocaml/dune#1180, @emillon) - Add `--no-print-directory` (borrowed from GNU make) to suppress `Entering directory` messages. (ocaml/dune#1668, @dra27) - Remove `--stats` and track fd usage in `--trace-file` (ocaml/dune#1667, @emillon) - Add virtual libraries feature and enable it by default (ocaml/dune#1430 fixes ocaml/dune#921, @rgrinberg) - Fix handling of Control+C in watch mode (ocaml/dune#1678, fixes ocaml/dune#1671, @diml) - Look for jsoo runtime in the same dir as the `js_of_ocaml` binary when the ocamlfind package is not available (ocaml/dune#1467, @nojb) - Make the `seq` package available for OCaml >= 4.07 (ocaml/dune#1714, @rgrinberg) - Add locations to error messages where a rule fails to generate targets and rules that require files outside the build/source directory. (ocaml/dune#1708, fixes ocaml/dune#848, @rgrinberg) - Let `Configurator` handle `sizeof` (in addition to negative numbers). (ocaml/dune#1726, fixes ocaml/dune#1723, @Chris00) - Fix an issue causing menhir generated parsers to fail to build in some cases. The fix is to systematically use `-short-paths` when calling `ocamlc -i` (ocaml/dune#1743, fix ocaml/dune#1504, @diml) - Never raise when printing located errors. The code that would print the location excerpts was prone to raising. (ocaml/dune#1744, fix ocaml/dune#1736, @rgrinberg) - Add a `dune upgrade` command for upgrading jbuilder projects to Dune (ocaml/dune#1749, @diml) - When automatically creating a `dune-project` file, insert the detected name in it (ocaml/dune#1749, @diml) - Add `(implicit_transitive_deps <bool>)` mode to dune projects. When this mode is turned off, transitive dependencies are not accessible. Only listed dependencies are directly accessible. (ocaml/dune#1734, ocaml/dune#430, @rgrinberg, @hnrgrgr) - Add `toplevel` stanza. This stanza is used to define toplevels with libraries already preloaded. (ocaml/dune#1713, @rgrinberg) - Generate `.merlin` files that account for normal preprocessors defined using a subset of the `action` language. (ocaml/dune#1768, @rgrinberg) - Emit `(orig_src_dir <path>)` metadata in `dune-package` for dune packages built with `--store-orig-source-dir` command line flag (also controlled by `DUNE_STORE_ORIG_SOURCE_DIR` env variable). This is later used to generate `.merlin` with `S`-directives pointed to original source locations and thus allowing merlin to see those. (ocaml/dune#1750, @andreypopp) - Improve the behavior of `dune promote` when the files to be promoted have been deleted. (ocaml/dune#1775, fixes ocaml/dune#1772, @diml) - unstable-fmt: preserve comments (ocaml/dune#1766, @emillon) - Pass flags correctly when using `staged_pps` (ocaml/dune#1779, fixes ocaml/dune#1774, @diml) - Fix an issue with the use of `(mode promote)` in the menhir stanza. It was previously causing intermediate *mock* files to be promoted (ocaml/dune#1783, fixes ocaml/dune#1781, @diml) - unstable-fmt: ignore files using OCaml syntax (ocaml/dune#1784, @emillon) - Configurator: Add `which` function to replace the `which` command line utility in a cross platform way. (ocaml/dune#1773, fixes ocaml/dune#1705, @Chris00) - Make configurator append paths to `$PKG_CONFIG_PATH` on macOS. Previously it was prepending paths and thus `$PKG_CONFIG_PATH` set by users could have been overridden by homebrew installed libraries (ocaml/dune#1785, @andreypopp) - Disallow c/cxx sources that share an object file in the same stubs archive. This means that `foo.c` and `foo.cpp` can no longer exist in the same library. (ocaml/dune#1788, @rgrinberg) - Forbid use of `%{targets}` (or `${@}` in jbuild files) inside preprocessing actions (ocaml/dune#1812, fixes ocaml/dune#1811, @diml) - Add `DUNE_PROFILE` environment variable to easily set the profile. (ocaml/dune#1806, @rgrinberg) - Deprecate the undocumented `(no_keep_locs)` field. It was only necessary until virtual libraries were supported (ocaml/dune#1822, fix ocaml/dune#1816, @diml) - Rename `unstable-fmt` to `format-dune-file` and remove its `--inplace` option. (ocaml/dune#1821, @emillon). - Autoformatting: `(using fmt 1.1)` will also format dune files (ocaml/dune#1821, @emillon). - Autoformatting: record dependencies on `.ocamlformat-ignore` files (ocaml/dune#1824, fixes ocaml/dune#1793, @emillon)

jonludlam requested a review from a user December 11, 2018 09:46

jonludlam requested review from emillon and rgrinberg as code owners December 11, 2018 09:46

rgrinberg requested changes Dec 11, 2018

View reviewed changes

jonludlam force-pushed the fix-1633 branch from f13da85 to 83602ed Compare December 11, 2018 10:50

jonludlam added 3 commits December 11, 2018 12:33

Remove dead code

e049dcb

Signed-off-by: Jon Ludlam <jon@recoil.org>

Update CHANGES.md

595777b

Signed-off-by: Jon Ludlam <jon@recoil.org>

jonludlam force-pushed the fix-1633 branch from b03ea37 to 595777b Compare December 11, 2018 11:34

rgrinberg approved these changes Dec 11, 2018

View reviewed changes

rgrinberg merged commit 59c4daa into ocaml:master Dec 11, 2018

ghost mentioned this pull request Dec 11, 2018

Build failure on macOS with Dune master #1633

Closed

rgrinberg added a commit to rgrinberg/jbuilder that referenced this pull request Dec 11, 2018

Revert "Delay opening redirected files until execing cmd (ocaml#1635)"

f528a30

This reverts commit 59c4daa. Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg added a commit to rgrinberg/jbuilder that referenced this pull request Dec 11, 2018

Revert "Delay opening redirected files until execing cmd (ocaml#1635)"

95b3235

This reverts commit 59c4daa. Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg added a commit to rgrinberg/jbuilder that referenced this pull request Dec 11, 2018

Revert "Delay opening redirected files until execing cmd (ocaml#1635)"

3f824a4

This reverts commit 59c4daa. Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

ghost pushed a commit to rgrinberg/jbuilder that referenced this pull request Dec 12, 2018

Revert "Delay opening redirected files until execing cmd (ocaml#1635)"

5286904

This reverts commit 59c4daa. Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

rgrinberg pushed a commit that referenced this pull request Jan 8, 2019

Delay opening redirected files until execing cmd (#1635)

4c98233

rgrinberg mentioned this pull request Jan 8, 2019

[new release] dune (1.6.3) ocaml/opam-repository#13235

Merged

rgrinberg mentioned this pull request Feb 12, 2019

[new release] dune (1.7.0) ocaml/opam-repository#13444

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay opening redirected files until execing cmd#1635

Delay opening redirected files until execing cmd#1635
rgrinberg merged 3 commits intoocaml:masterfrom
jonludlam:fix-1633

jonludlam commented Dec 11, 2018

Uh oh!

rgrinberg left a comment

Uh oh!

rgrinberg Dec 11, 2018

Uh oh!

rgrinberg Dec 11, 2018 •

edited

Loading

Uh oh!

jonludlam Dec 11, 2018

Uh oh!

rgrinberg Dec 11, 2018

Uh oh!

ghost commented Dec 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jonludlam commented Dec 11, 2018

Uh oh!

rgrinberg left a comment

Choose a reason for hiding this comment

Uh oh!

rgrinberg Dec 11, 2018

Choose a reason for hiding this comment

Uh oh!

rgrinberg Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonludlam Dec 11, 2018

Choose a reason for hiding this comment

Uh oh!

rgrinberg Dec 11, 2018

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rgrinberg Dec 11, 2018 •

edited

Loading