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 check that all files are present before installing #2230

Merged
merged 1 commit into from
Jun 4, 2019
Merged

Add check that all files are present before installing #2230

merged 1 commit into from
Jun 4, 2019

Conversation

rgrinberg
Copy link
Member

We can go even further and make sure that all .install files are valid before installing anything, but this PR at least makes it so that we don't install a partial package.

@rgrinberg rgrinberg requested review from emillon and a user June 2, 2019 07:06
@rgrinberg
Copy link
Member Author

@emillon I'm not sure what's the correct behavior wrt --dry-run. I feel like the check should still occur, but it makes the tests fail. Shall we change the tests?

@emillon
Copy link
Collaborator

emillon commented Jun 3, 2019

@rgrinberg I think that you can fix that by moving more things into the ops: instead of adding exists : _ -> bool, you can add check_files that take entries and to the checks in real ops, and do nothing in dry run. Would that work?

@rgrinberg
Copy link
Member Author

It would work, but I'm not sure it makes sense. Shouldn't the dry run warn us about files being missing?

@emillon
Copy link
Collaborator

emillon commented Jun 3, 2019

I see. Does this need to go through Ops/--dry-run at all? I think that this mechanism is useful when touching external files, but for inspecting in-tree *.install files, a plain exists sounds good enough.

@rgrinberg
Copy link
Member Author

Okay. So I will indeed modify this PR so that --dry-run doesn't affect the behavior of this sanity check.

One last point: shall I do the sanity check on all the .install files before installing any package? Currently, I'm doing it per package.

@emillon
Copy link
Collaborator

emillon commented Jun 3, 2019

Per-package seems fine. There may be room for an alias that does more thorough tests, but for now I think it's ok to only check what's going to be installed.

@rgrinberg
Copy link
Member Author

I think I might have been unclear about this. What I meant was that if you have two packages like this:

$ ls *.install
bar.install # this one works
foo.install # this one is faulty

Running $ dune install will install the package bar but will fail on foo. This can be bad - especially in the case where there are artifacts in bar that depend on foo.

@emillon
Copy link
Collaborator

emillon commented Jun 3, 2019

Ah ok! in that case, yes, a more global check would be great (but can be done in a separate PR if you prefer).

@rgrinberg rgrinberg force-pushed the install-uninstall-error branch 4 times, most recently from dce2745 to 12e4d9b Compare June 3, 2019 16:45
@rgrinberg
Copy link
Member Author

@emillon could you have another look? I've now:

  • Made the sanity check work on all files
  • Changed the installation order of packages to be alphabetical. This probably doesn't make much of a difference, but it's nice to keep things deterministic.

@rgrinberg rgrinberg mentioned this pull request Jun 4, 2019
Closed
emillon
emillon approved these changes Jun 4, 2019
View reviewed changes
Copy link
Collaborator

@emillon emillon left a comment

Choose a reason for hiding this comment

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

LGTM with minor remarks!

bin/install_uninstall.ml Show resolved Hide resolved
bin/install_uninstall.ml Show resolved Hide resolved
@rgrinberg rgrinberg force-pushed the install-uninstall-error branch 2 times, most recently from 7c0b021 to ff1ad63 Compare June 4, 2019 08:42
@rgrinberg
Add check that all files are present before installing
3e08a88 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg rgrinberg force-pushed the install-uninstall-error branch from ff1ad63 to 3e08a88 Compare June 4, 2019 10:32
@rgrinberg rgrinberg merged commit 3538ae7 into ocaml:master Jun 4, 2019
@rgrinberg rgrinberg deleted the install-uninstall-error branch June 4, 2019 10:34
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jun 4, 2019
@rgrinberg
[new release] dune (1.10.0)
cfa54d7 
CHANGES:

- Restricted the set of variables available for expansion in the destination
  filename of `install` stanza to simplify implementation and avoid dependency
  cycles. (ocaml/dune#2073, @aalekseyev, @diml)

- [menhir] call menhir from context root build_dir (ocaml/dune#2067, @ejgallego,
  review by @diml, @rgrinberg)

- [coq] Add `coq.pp` stanza to help with pre-processing of grammar
  files (ocaml/dune#2054, @ejgallego, review by @rgrinberg)

- Add a new more generic form for the *promote* mode: `(promote
  (until-clean) (into <dir>))` (ocaml/dune#2068, @diml)

- Allow to promote only a subset of the targets via `(promote (only
  <pred>))`. For instance: `(promote (only *.mli))` (ocaml/dune#2068, @diml)

- Improve the behavior when a strict subset of the targets of a rule is already
  in the source tree for projects using the dune language < 1.10 (ocaml/dune#2068, fixes
  ocaml/dune#2061, @diml)

- With lang dune >= 1.10, rules in standard mode are no longer allowed to
  produce targets that are present in the source tree. This has been a warning
  for long enough (ocaml/dune#2068, @diml)

- Allow %{...} variables in pps flags (ocaml/dune#2076, @mlasson review by @diml and
  @aalekseyev).

- Add a 'cookies' option to ppx_rewriter/deriver flags in library stanzas. This
  allow to specify cookie requests from variables expanded at each invocation of
  the preprocessor. (ocaml/dune#2106, @mlasson @diml)

- Add more opam metadata and use it to generate `.opam` files. In particular, a
  `package` field has been added to specify package specific information.
  (ocaml/dune#2017, ocaml/dune#2091, @avsm, @jonludlam, @rgrinberg)

- Clean up the special support for `findlib.dynload`. Before, Dune would simply
  match on the library name. Now, we only match on the findlib package name when
  the library doesn't come from Dune. Someone writing a library called
  `findlib.dynload` with Dune would have to add `(special_builtin_support
  findlib_dynload)` to trigger the special behavior. (ocaml/dune#2115, @diml)

- Install the `future_syntax` preprocessor as `ocaml-syntax-shims.exe` (ocaml/dune#2125,
  @rgrinberg)

- Hide full command on errors and warnings in development and show them in CI.
  (detected using the `CI` environment variable). Commands for which the
  invocation might be omitted must output an error prefixed with `File `. Add an
  `--always-show-command-line` option to disable this behavior and always show
  the full command. (ocaml/dune#2120, fixes ocaml/dune#1733, @rgrinberg)

- In `dune-workspace` files, add the ability to choose the host context and to
  create duplicates of the default context with different settings. (ocaml/dune#2098,
  @TheLortex, review by @diml, @rgrinberg and @aalekseyev)

- Add support for hg in `dune subst` (ocaml/dune#2135, @diml)

- Don't build documentation for implementations of virtual libraries (ocaml/dune#2141,
  fixes ocaml/dune#2138, @jonludlam)

- Fix generation of the `-pp` flag in .merlin (ocaml/dune#2142, @rgrinberg)

- Make `dune subst` add a `(version ...)` field to the `dune-project`
  file (ocaml/dune#2148, @diml)

- Add the `%{os_type}` variable, which is a short-hand for
  `%{ocaml-config:os_type}` (ocaml/dune#1764, @diml)

- Allow `enabled_if` fields in `library` stanzas, restricted to the
  `%{os_type}`, `%{model}`, `%{architecture}`, `%{system}` variables (ocaml/dune#1764,
  ocaml/dune#2164 @diml, @rgrinberg)

- Fix `chdir` on external and source paths. Dune will also fail gracefully if
  the external or source path does not exist (ocaml/dune#2165, fixes ocaml/dune#2158, @rgrinberg)

- Support the `.cc` extension fro C++ sources (ocaml/dune#2195, fixes ocaml/dune#83, @rgrinberg)

- Run `ocamlformat` relative to the context root. This improves the locations of
  errors. (ocaml/dune#2196, fixes ocaml/dune#1370, @rgrinberg)

- Fix detection of `README`, `LICENSE`, `CHANGE`, and `HISTORY` files. These
  would be undetected whenever the project was nested in another workspace.
  (ocaml/dune#2194, @rgrinberg)

- Fix generation of `.merlin` whenever there's more than one stanza with the
  same ppx preprocessing specification (ocaml/dune#2209 ,fixes ocaml/dune#2206, @rgrinberg)

- Fix generation of `.merlin` in the presence of the `copy_files` stanza and
  preprocessing specifications of other stanazs. (ocaml/dune#2211, fixes ocaml/dune#2206,
  @rgrinberg)

- Run `refmt` from the context's root directory. This improves error messages in
  case of syntax errors. (ocaml/dune#2223, @rgrinberg)

- In .merlin files, don't pass `-dump-ast` to the `future_syntax` preprocessor.
  Merlin doesn't seem to like it when binary AST is generated by a `-pp`
  preprocessor. (ocaml/dune#2236, @aalekseyev)

- `dune install` will verify that all files mentioned in all .install files
  exist before trying to install anything. This prevents partial installation of
  packages (ocaml/dune#2230, @rgrinberg)
@rgrinberg rgrinberg mentioned this pull request Jun 4, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emillon emillon emillon approved these changes

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 @emillon