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

Delay opening redirected files until execing cmd #1635

Merged
merged 3 commits into from
Dec 11, 2018
Merged

Delay opening redirected files until execing cmd #1635

merged 3 commits into from
Dec 11, 2018

Conversation

jonludlam
Copy link
Member

No description provided.

@jonludlam jonludlam requested a review from a user December 11, 2018 09:46
rgrinberg
rgrinberg requested changes Dec 11, 2018
View reviewed changes
Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

Looks good. A CHANGES entry is appropriate as well.

src/action_exec.ml Outdated
@@ -44,7 +40,7 @@ let exec_echo stdout_to str =
Fiber.return
(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))
Copy link
Member

Choose a reason for hiding this comment

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

This can be simplified to use Io.write_file now.

src/action_exec.ml Outdated
@@ -195,16 +179,15 @@ let rec exec t ~ectx ~dir ~env ~stdout_to ~stderr_to =
Fiber.return ()

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
Copy link
Member

@rgrinberg rgrinberg Dec 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Why this is necessary deserves a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

A comment would be useful here I think. Thanks.

@jonludlam
Delay opening redirected files until execing cmd
991a3ed 
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>
@jonludlam
Remove dead code
e049dcb 
Signed-off-by: Jon Ludlam <jon@recoil.org>
@jonludlam
Update CHANGES.md
595777b 
Signed-off-by: Jon Ludlam <jon@recoil.org>
@rgrinberg rgrinberg merged commit 59c4daa into ocaml:master Dec 11, 2018
@ghost ghost mentioned this pull request Dec 11, 2018
Closed
@ghost
Copy link

ghost commented Dec 11, 2018

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)))

rgrinberg added a commit to rgrinberg/jbuilder that referenced this pull request Dec 11, 2018
@rgrinberg
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
@rgrinberg
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
@rgrinberg
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
@rgrinberg @jeremiedimino
Revert "Delay opening redirected files until execing cmd (ocaml#1635)"
5286904 
This reverts commit 59c4daa.

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
ghost pushed a commit that referenced this pull request Dec 12, 2018
@rgrinberg @jeremiedimino
Open files lazily + close them ASAP (#1643)
1b139df 
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>
rgrinberg pushed a commit that referenced this pull request Jan 8, 2019
@jonludlam @rgrinberg
Delay opening redirected files until execing cmd (#1635)
4c98233
rgrinberg added a commit that referenced this pull request Jan 8, 2019
@rgrinberg
Open files lazily + close them ASAP (#1643)
f41e27b 
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>
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jan 8, 2019
@rgrinberg
[new release] dune (1.6.3)
34777bb 
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)
@rgrinberg rgrinberg mentioned this pull request Jan 8, 2019
Merged
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jan 8, 2019
@rgrinberg
[new release] dune (1.6.3)
cd59a09 
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)
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jan 9, 2019
@rgrinberg
[new release] dune (1.6.3)
6c853e7 
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)
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Feb 12, 2019
@rgrinberg
[new release] dune (1.7.0)
674b1ed 
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)
@rgrinberg rgrinberg mentioned this pull request Feb 12, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg approved these changes

@emillon emillon Awaiting requested review from emillon

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
@jonludlam @rgrinberg