Raise an error when trying to mark the current directory as vendored or data-only by voodoos · Pull Request #3056 · ocaml/dune

voodoos · 2020-01-24T08:46:15Z

This PR add new error messages triggered when:

Trying to declare the current directory as data_only_dirs
Trying to declare the current directory as vendored_dirs

The check is quite naive and tricky globs like ../name-of-the-current-dir will not trigger it.

Fixes #3019

rgrinberg

Looks good apart from some small nits. A change entry is appropriate as well.

src/dune/sub_dirs.ml

voodoos · 2020-01-24T10:35:52Z

Do you think it would be worth it to have a smarter algorithm in the is_dot helper function line 176 ?

I was considering using Build_system.eval_pred to resolve the glob and then check if the current folder is in the list.

rgrinberg · 2020-01-24T10:51:36Z

Do you think it would be worth it to have a smarter algorithm in the is_dot helper function line 176 ?

We could also disallow all path components if you'd like. In fact, I'm pretty sure we do this elsewhere when parsing globs.

I was considering using Build_system.eval_pred to resolve the glob and then check if the current folder is in the list.

Yeah, that might be a lot harder than you think. It will certainly introduce circular dependencies for example.

voodoos · 2020-01-24T10:58:42Z

Ok, the current PR is probably an acceptable compromise between complexity and helpfulness.

rgrinberg · 2020-01-24T11:10:41Z

Ok, the current PR is probably an acceptable compromise between complexity and helpfulness.

Yeah, however what about sharing the validation that we have we for ignored sub dirs:

    let ignored =
      let+ l =
        enter
          (repeat
             (plain_string (fun ~loc dn ->
                  if
                    Filename.dirname dn <> Filename.current_dir_name
                    ||
                    match dn with
                    | ""
                    | "."
                    | ".." ->
                      true
                    | _ -> false
                  then
                    User_error.raise ~loc
                      [ Pp.textf "Invalid sub-directory name %S" dn ]
                  else
                    dn)))
      in
      Predicate_lang.Glob.of_string_set (String.Set.of_list l)

This prevents globs that are outside the current dir.

voodoos · 2020-01-27T10:09:39Z

I have the impression that this validation is too restrictive for data_only_dirs or vendored_dirs:

If I am not mistaken we want to allow:

Direct subdirectories: ./dataonlydirs
Deeper subdirectories: ./somedir/dalaonlydir

And disallow:

Current directory: .

Should also be disallowed (but it may not be worth the trouble):

Sneaky globs containing the current directory: ./somedir/../ or ../*
Or those pointing to a directory parent of the current directory: ..

May also be allowed:

Non-sub directories which are not parents of the current dir: ../dataonly

rgrinberg · 2020-01-27T10:24:26Z

Deeper subdirectories: ./somedir/dalaonlydir

We want to allow this but I don't think this currently works. We might as well reflect this in the validation.

Should also be disallowed (but it may not be worth the trouble):

Agreed with disallowing all of this.

May also be allowed:

I don't think we should allow this either. Directories shouldn't be able to affect how rules are setup in parent or sibling directories.

voodoos · 2020-01-27T11:38:43Z

Deeper subdirectories: ./somedir/dalaonlydir

We want to allow this but I don't think this currently works. We might as well reflect this in the validation.

I just checked and indeed it doesn't work. It should indeed be reflected by the validation, I will start reworking the PR.

rgrinberg · 2020-01-27T11:40:44Z

Would be good to have an explicit test for this as well. It’s something that we’d like to fix in the future of course, so feel free on investigating what it would take to extend this functionality

…

On Jan 27, 2020, 7:38 PM +0800, Ulysse ***@***.***>, wrote: > > Deeper subdirectories: ./somedir/dalaonlydir > We want to allow this but I don't think this currently works. We might as well reflect this in the validation. I just checked and indeed it doesn't work. It should indeed be reflected by the validation, I will start reworking the PR. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or unsubscribe.

voodoos · 2020-02-05T15:00:45Z

Yeah, however what about sharing the validation that we have we for ignored sub dirs:
    let ignored =
     let+ l =
       enter
         (repeat
            (plain_string (fun ~loc dn -> some_validation)))
     in
      Predicate_lang.Glob.of_string_set (String.Set.of_list l)
This prevents globs that are outside the current dir.

It also prevent * to function, in fact, if I understand well, the same result could be obtained with a simple string. Thus I cannot use it for vendored_dirs or data_only_dirs because we want to be able to vendor all subfolders with *.

I am unsure of how I could adapt the present validation to work as intended with *.

voodoos · 2020-02-05T15:57:56Z

I guess I can use something like that instead:

Predicate_lang.Glob.of_glob (Glob.of_string_exn loc some_string)

Will give it a try.

test/blackbox-tests/test-cases/vendor/run.t

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

@shonfeder

…lugin, dune-private-libs and dune-glob (2.3.0) CHANGES: - Improve validation and error handling of arguments to `dune init` (ocaml/dune#3103, fixes ocaml/dune#3046, @shonfeder) - `dune init exec NAME` now uses the `NAME` argument for private modules (ocaml/dune#3103, fixes ocaml/dune#3088, @shonfeder) - Avoid linear walk to detect children, this should greatly improve performance when a target has a large number of dependencies (ocaml/dune#2959, @ejgallego, @aalekseyev, @Armael) - [coq] Add `(boot)` option to `(coq.theories)` to enable bootstrap of Coq's stdlib (ocaml/dune#3096, @ejgallego) - [coq] Deprecate `public_name` field in favour of `package` (ocaml/dune#2087, @ejgallego) - Better error reporting for "data only" and "vendored" dirs. Using these with anything else than a strict subdirectory or `*` will raise an error. The previous behavior was to just do nothing (ocaml/dune#3056, fixes ocaml/dune#3019, @voodoos) - Fix bootstrap on bytecode only switches on windows or where `-j1` is set. (ocaml/dune#3112, @xclerc, @rgrinberg) - Allow `enabled_if` fields in `executable(s)` stanzas (ocaml/dune#3137, fixes ocaml/dune#1690 @voodoos) - Do not fail if `ocamldep`, `ocamlmklib`, or `ocaml` are absent. Wait for them to be used to fail (ocaml/dune#3138, @rgrinberg) - Introduce a `strict_package_deps` mode that verifies that dependencies between packages in the workspace are specified correctly. (@rgrinberg, ocaml/dune#3117)

@shonfeder

…lugin, dune-private-libs and dune-glob (2.3.0) CHANGES: - Improve validation and error handling of arguments to `dune init` (ocaml/dune#3103, fixes ocaml/dune#3046, @shonfeder) - `dune init exec NAME` now uses the `NAME` argument for private modules (ocaml/dune#3103, fixes ocaml/dune#3088, @shonfeder) - Avoid linear walk to detect children, this should greatly improve performance when a target has a large number of dependencies (ocaml/dune#2959, @ejgallego, @aalekseyev, @Armael) - [coq] Add `(boot)` option to `(coq.theories)` to enable bootstrap of Coq's stdlib (ocaml/dune#3096, @ejgallego) - [coq] Deprecate `public_name` field in favour of `package` (ocaml/dune#2087, @ejgallego) - Better error reporting for "data only" and "vendored" dirs. Using these with anything else than a strict subdirectory or `*` will raise an error. The previous behavior was to just do nothing (ocaml/dune#3056, fixes ocaml/dune#3019, @voodoos) - Fix bootstrap on bytecode only switches on windows or where `-j1` is set. (ocaml/dune#3112, @xclerc, @rgrinberg) - Allow `enabled_if` fields in `executable(s)` stanzas (ocaml/dune#3137, fixes ocaml/dune#1690 @voodoos) - Do not fail if `ocamldep`, `ocamlmklib`, or `ocaml` are absent. Wait for them to be used to fail (ocaml/dune#3138, @rgrinberg) - Introduce a `strict_package_deps` mode that verifies that dependencies between packages in the workspace are specified correctly. (@rgrinberg, ocaml/dune#3117) - Make sure the `@all` alias is defined when no `dune` file is present in a directory (ocaml/dune#2946, fix ocaml/dune#2927, @diml)

@shonfeder

…lugin, dune-private-libs and dune-glob (2.3.0) CHANGES: - Improve validation and error handling of arguments to `dune init` (ocaml/dune#3103, fixes ocaml/dune#3046, @shonfeder) - `dune init exec NAME` now uses the `NAME` argument for private modules (ocaml/dune#3103, fixes ocaml/dune#3088, @shonfeder) - Avoid linear walk to detect children, this should greatly improve performance when a target has a large number of dependencies (ocaml/dune#2959, @ejgallego, @aalekseyev, @Armael) - [coq] Add `(boot)` option to `(coq.theories)` to enable bootstrap of Coq's stdlib (ocaml/dune#3096, @ejgallego) - [coq] Deprecate `public_name` field in favour of `package` (ocaml/dune#2087, @ejgallego) - Better error reporting for "data only" and "vendored" dirs. Using these with anything else than a strict subdirectory or `*` will raise an error. The previous behavior was to just do nothing (ocaml/dune#3056, fixes ocaml/dune#3019, @voodoos) - Fix bootstrap on bytecode only switches on windows or where `-j1` is set. (ocaml/dune#3112, @xclerc, @rgrinberg) - Allow `enabled_if` fields in `executable(s)` stanzas (ocaml/dune#3137, fixes ocaml/dune#1690 @voodoos) - Do not fail if `ocamldep`, `ocamlmklib`, or `ocaml` are absent. Wait for them to be used to fail (ocaml/dune#3138, @rgrinberg) - Introduce a `strict_package_deps` mode that verifies that dependencies between packages in the workspace are specified correctly. (@rgrinberg, ocaml/dune#3117) - Make sure the `@all` alias is defined when no `dune` file is present in a directory (ocaml/dune#2946, fix ocaml/dune#2927, @diml)

voodoos requested a review from a user January 24, 2020 08:46

voodoos requested a review from rgrinberg as a code owner January 24, 2020 08:46

rgrinberg approved these changes Jan 24, 2020

View reviewed changes

src/dune/sub_dirs.ml Outdated Show resolved Hide resolved

src/dune/sub_dirs.ml Outdated Show resolved Hide resolved

voodoos changed the title ~~Vendors issue~~ Raise an error when trying to describe the current directory as vendored or data-only Jan 24, 2020

voodoos changed the title ~~Raise an error when trying to describe the current directory as vendored or data-only~~ Raise an error when trying to mark the current directory as vendored or data-only Jan 24, 2020

voodoos requested a review from rgrinberg February 6, 2020 13:43

rgrinberg reviewed Feb 6, 2020

View reviewed changes

test/blackbox-tests/test-cases/vendor/run.t Outdated Show resolved Hide resolved

voodoos force-pushed the vendors-issue branch from 3b1be6c to 4116130 Compare February 7, 2020 10:20

Raise error when trying to vendor current dir.

71ce09a

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>

rgrinberg force-pushed the vendors-issue branch from 4116130 to 71ce09a Compare February 9, 2020 22:02

rgrinberg merged commit 035c93a into ocaml:master Feb 9, 2020

voodoos mentioned this pull request Feb 12, 2020

Fix data only regression from PR#3056 #3126

Merged

rgrinberg mentioned this pull request Feb 15, 2020

[new release] dune-build-info, dune, dune-configurator, dune-action-plugin, dune-private-libs and dune-glob (2.3.0) ocaml/opam-repository#15845

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise an error when trying to mark the current directory as vendored or data-only#3056

Raise an error when trying to mark the current directory as vendored or data-only#3056
rgrinberg merged 1 commit intoocaml:masterfrom
voodoos:vendors-issue

voodoos commented Jan 24, 2020

Uh oh!

rgrinberg left a comment

Uh oh!

Uh oh!

Uh oh!

voodoos commented Jan 24, 2020

Uh oh!

rgrinberg commented Jan 24, 2020 •

edited

Loading

Uh oh!

voodoos commented Jan 24, 2020

Uh oh!

rgrinberg commented Jan 24, 2020

Uh oh!

voodoos commented Jan 27, 2020

Uh oh!

rgrinberg commented Jan 27, 2020

Uh oh!

voodoos commented Jan 27, 2020

Uh oh!

rgrinberg commented Jan 27, 2020 via email

Uh oh!

voodoos commented Feb 5, 2020 •

edited

Loading

Uh oh!

voodoos commented Feb 5, 2020

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

voodoos commented Jan 24, 2020

Uh oh!

rgrinberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

voodoos commented Jan 24, 2020

Uh oh!

rgrinberg commented Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

voodoos commented Jan 24, 2020

Uh oh!

rgrinberg commented Jan 24, 2020

Uh oh!

voodoos commented Jan 27, 2020

Uh oh!

rgrinberg commented Jan 27, 2020

Uh oh!

voodoos commented Jan 27, 2020

Uh oh!

rgrinberg commented Jan 27, 2020 via email

Uh oh!

voodoos commented Feb 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

voodoos commented Feb 5, 2020

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rgrinberg commented Jan 24, 2020 •

edited

Loading

voodoos commented Feb 5, 2020 •

edited

Loading