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

Tests for relative dependencies outside the workspace #4035

Closed
wants to merge 6 commits into from
Closed

Tests for relative dependencies outside the workspace #4035

wants to merge 6 commits into from

Conversation

bobot
Copy link
Collaborator

@bobot bobot commented Dec 14, 2020

Wait for decisions in #4034

@bobot bobot force-pushed the fix_user_dependencies_outside_workspace branch from 6e1c5ed to d3eb289 Compare December 14, 2020 10:30
Base automatically changed from master to main January 14, 2021 17:09
@bobot bobot force-pushed the fix_user_dependencies_outside_workspace branch 3 times, most recently from dda38e1 to ccee62b Compare January 22, 2022 13:22
@bobot bobot mentioned this pull request Jan 22, 2022
Closed
@bobot
Copy link
Collaborator Author

bobot commented Jan 22, 2022

@snowleopard it solves one CR you added in a tests. But the current fix is perhaps not in the right way. The goal is for dependencies outside the source_tree to be considered external.

@rgrinberg It would be better if some fix for this problem can go in 3.0 but it is not a regression so it could wait for 3.0.1

@bobot bobot force-pushed the fix_user_dependencies_outside_workspace branch from ccee62b to 2878f76 Compare January 22, 2022 15:39
@snowleopard
Copy link
Collaborator

@snowleopard it solves one CR you added in a tests. But the current fix is perhaps not in the right way. The goal is for dependencies outside the source_tree to be considered external.

Nice! I'll have a look in more detail when I'm back from vacation.

@bobot
Copy link
Collaborator Author

bobot commented Apr 6, 2022

@snowleopard can you have a look?

@bobot bobot mentioned this pull request Apr 14, 2022
Closed
@bobot
Copy link
Collaborator Author

bobot commented Apr 14, 2022 •
edited
Loading

TODO: Perhaps add a test for dune exec -- ../.

@bobot bobot requested a review from snowleopard April 14, 2022 07:42
snowleopard
snowleopard reviewed Apr 14, 2022
View reviewed changes
src/dune_util/value.ml Outdated Show resolved Hide resolved
snowleopard
snowleopard reviewed Apr 14, 2022
View reviewed changes
test/blackbox-tests/test-cases/rule/dependency-external.t/run.t Outdated
Error: path outside the workspace: ../external.txt from .
[1]

$ dune build --root=a/b @test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like a duplicate of the above command.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer to test that when nothing change nothing is rebuilt

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment above this command that states this is what you expected?

@snowleopard
Copy link
Collaborator

@bobot Apologies for taking so long. I've left some comments.

@rgrinberg Could you have a look as well since this touches the rules?

@rgrinberg
Copy link
Member

I did. Will review you again after @bobot addresses your comments.

@bobot bobot force-pushed the fix_user_dependencies_outside_workspace branch 4 times, most recently from fca7b70 to 175dbae Compare April 22, 2022 16:46
@bobot
Copy link
Collaborator Author

bobot commented Apr 22, 2022

@snowleopard I think all your comments are taken into account. Additionally dune exec -- ../run.exe is also fixed, and one case of copy_rules. Perhaps other instance of Path.relative should be converted.

@rgrinberg do you want to review?

rgrinberg
rgrinberg reviewed Apr 22, 2022
View reviewed changes
src/dune_util/value.mli Outdated Show resolved Hide resolved
rgrinberg
rgrinberg reviewed Apr 22, 2022
View reviewed changes
src/dune_util/value.ml Outdated
let to_path_in_build ?error_loc t ~dir =
match t with
| String s -> Path.relative_to_source_in_build ?error_loc ~dir s
| Dir p | Path p -> p
Copy link
Member

Choose a reason for hiding this comment

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

How do we know that Dir p or Path p are in build?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I considered the path where created by another dune internal function, so it should be in build or external. The problem would be if it can be in source.

Copy link
Member

Choose a reason for hiding this comment

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

Could we an assert for this then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is now asserted, you can merge if its ok for you.

@rgrinberg rgrinberg added this to the 3.2.0 milestone Apr 22, 2022
@rgrinberg rgrinberg linked an issue Apr 25, 2022 that may be closed by this pull request
dune exec -- ../ causes dune to crash #5572
Closed
@rgrinberg rgrinberg removed a link to an issue Apr 25, 2022
dune exec -- ../ causes dune to crash #5572
Closed
@rgrinberg rgrinberg linked an issue Apr 25, 2022 that may be closed by this pull request
dune exec -- ../ causes dune to crash #5572
Closed
@bobot bobot force-pushed the fix_user_dependencies_outside_workspace branch 2 times, most recently from f13f818 to e0c71d1 Compare May 5, 2022 10:02
@bobot bobot force-pushed the fix_user_dependencies_outside_workspace branch from e0c71d1 to 5d77345 Compare May 5, 2022 10:04
bobot and others added 4 commits May 5, 2022 12:06
@bobot
Consider relative dependencies outside source_tree as external
09cb194 
Signed-off-by: François Bobot <[email protected]>
@bobot
Fix dune exec -- ../run.exe
2a55940 
Signed-off-by: François Bobot <[email protected]>
@rgrinberg @bobot
test: regression for 5572
b23ed52 
Signed-off-by: Rudi Grinberg <[email protected]>
@bobot
Check that deps are not in source
b0cb202 
  but in build or external.

Signed-off-by: François Bobot <[email protected]>
@bobot bobot force-pushed the fix_user_dependencies_outside_workspace branch from 5d77345 to b0cb202 Compare May 5, 2022 10:07
@rgrinberg rgrinberg closed this May 5, 2022
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request May 17, 2022
@rgrinberg
[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, du…
a6c2fc7 
…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.2.0)

CHANGES:

- Fixed ``dune describe workspace --with-deps`` so that it correctly
  handles Reason files, as well as files any other dialect. (ocaml/dune#5701, @esope)

- Disable alerts when compiling code in vendored directories (ocaml/dune#5683,
  @NathanReb)

- Fixed ``dune describe --with-deps``, that crashed when some
  preprocessing was required in a dune file using ``per_module``.
  (ocaml/dune#5682, fixes ocaml/dune#5680, @esope)

- Add `$ dune describe pp` to print the preprocssed ast of sources. (ocaml/dune#5615,
  fixes ocaml/dune#4470, @cannorin)

- Report dune file evaluation errors concurrently. In the same way we report
  build errors. (ocaml/dune#5655, @rgrinberg)

- Watch mode now default to clearing the terminal on rebuild (ocaml/dune#5636, fixes,
  ocaml/dune#5216, @rgrinberg)

- The output of jobs that finished but were cancelled is now omitted. (ocaml/dune#5631,
  fixes ocaml/dune#5482, @rgrinberg)

- Allows to configure all the default destination directories with `./configure`
  (adds `bin`, `sbin`, `data`, `libexec`). Use `OPAM_SWITCH_PREFIX` instead of
  calling the `opam` binaries in `dune install`. Fix handling of multiple
  `libdir` in `./configure` for handling `/usr/lib/ocaml/` and
  `/usr/local/lib/ocaml`. In `dune install` forbid relative directories in
  `libdir`, `docdir` and others specific directory setting because their handling
  was inconsistent (ocaml/dune#5516, fixes ocaml/dune#3978 and ocaml/dune#5455, @bobot)

- `--terminal-persistence=clear-on-rebuild` will no longer destroy scrollback
  on some terminals (ocaml/dune#5646, @rgrinberg)

- Add a fmt command as a shortcut of `dune build @fmt --auto-promote` (ocaml/dune#5574,
  @tmattio)

- Watch mode now tracks copied external files, external directories for
  dependencies, dune files in OCaml syntax, files used by `include` stanzas,
  dune-project, opam files, libraries builtin with compiler, and foreign
  sources (ocaml/dune#5627, ocaml/dune#5645, ocaml/dune#5652, ocaml/dune#5656, ocaml/dune#5672, ocaml/dune#5691, ocaml/dune#5722, fixes ocaml/dune#5331,
  @rgrinberg)

- Improve metrics for cram tests. Include test names in the event and add a
  category for cram tests (ocaml/dune#5626, @rgrinberg)

- Allow specifying multiple licenses in project file (ocaml/dune#5579, fixes ocaml/dune#5574,
  @liyishuai)

- Match `glob_files` only against files in external directories (ocaml/dune#5614, fixes
  ocaml/dune#5540, @rgrinberg)

- Add pid's to chrome trace output (ocaml/dune#5617, @rgrinberg)

- Fix race when creating local cache directory (ocaml/dune#5613, fixes ocaml/dune#5461, @rgrinberg)

- Add `not` to boolean expressions (ocaml/dune#5610, fix ocaml/dune#5503, @rgrinberg)

- Fix relative dependencies outside the workspace (ocaml/dune#4035, fixes ocaml/dune#5572, @bobot)

- Allow to specify `--prefix` via the environment variable
  `DUNE_INSTALL_PREFIX` (ocaml/dune#5589, @vapourismo)

- Dune-site.plugin: add support for `archive(native|byte, plugin)` used in the
  wild before findlib documented `plugin(native|byte)` in 2015 (ocaml/dune#5518, @bobot)

- Fix a bug where Dune would not correctly interpret `META` files in alternative
  layout (ie when the META file is named `META.$pkg`). The `Llvm` bindings were
  affected by this issue. (ocaml/dune#5619, fixes ocaml/dune#5616, @nojb)

- Support `(binaries)` in `(env)` in dune-workspace files (ocaml/dune#5560, fix ocaml/dune#5555,
  @emillon)

- (mdx) stanza: add support for (locks). (ocaml/dune#5628, fixes ocaml/dune#5489, @emillon)

- (mdx) stanza: support including files in different directories using relative
  paths, and provide better error messages when paths are invalid (ocaml/dune#5703, ocaml/dune#5704,
  fixes ocaml/dune#5596, @emillon)

- Fix ctypes rules for external lib names which aren't valid ocaml names
  (ocaml/dune#5667, fixes ocaml/dune#5511, @Khady)
@rgrinberg rgrinberg mentioned this pull request May 17, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg left review comments

@snowleopard snowleopard snowleopard left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.

dune exec -- ../ causes dune to crash
3 participants
@bobot @snowleopard @rgrinberg