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

Add only_sources field to copy_files stanza #9827

Merged
merged 20 commits into from
Feb 9, 2024
Merged

Add only_sources field to copy_files stanza #9827

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
a4c3678
copy_files: add test to show behavior with build files
jchavarri Jan 24, 2024
a266d14
add copy_files_src stanza
jchavarri Jan 24, 2024
4b0a038
format
jchavarri Jan 24, 2024
25b58fe
Merge branch 'main' into source-files
jchavarri Jan 24, 2024
8434970
copy_files: add only_sources field
jchavarri Jan 24, 2024
12cd755
copy_files: guard new field against version check
jchavarri Jan 25, 2024
26ae93e
Merge branch 'main' into source-files
jchavarri Jan 25, 2024
f81d57f
copy_files: don't rely on library for the test
jchavarri Jan 25, 2024
df71976
Merge branch 'main' into source-files
jchavarri Jan 26, 2024
6166c80
add changelog + docs
jchavarri Jan 26, 2024
dd2bd16
rename only_sources to sources
jchavarri Jan 26, 2024
4bd6b3e
simpler test
jchavarri Jan 26, 2024
ef07616
format
jchavarri Jan 26, 2024
5802eae
replace sources bool with variant
jchavarri Jan 26, 2024
d1ee4b7
Revert "rename only_sources to sources"
jchavarri Jan 29, 2024
1834f9b
Merge branch 'main' into source-files
jchavarri Jan 29, 2024
87a5ffc
format
jchavarri Jan 29, 2024
fc022d2
Merge branch 'main' into source-files
emillon Feb 8, 2024
236ea2c
Allow blang expressions
emillon Feb 8, 2024
6914cea
Merge branch 'main' into source-files
emillon Feb 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion otherlibs/dune-rpc/private/types.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ end
module Version = struct
type t = int * int

let latest = 3, 13
let latest = 3, 14
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note I had to change this to be able to run the test. Not sure if bumping this version involves more work / should be done in a different PR.

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's a bit surprising that this version is stored in dune-rpc library 🤔


let sexp : t Conv.value =
let open Conv in
Expand Down
5 changes: 4 additions & 1 deletion src/dune_rules/simple_rules.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,10 @@ let copy_files sctx ~dir ~expander ~src_dir (def : Copy_files.t) =
not the current directory."
];
(* add rules *)
let* files = Build_system.eval_pred (File_selector.of_glob ~dir:src_in_build glob) in
let* files =
let dir = if def.only_sources then src_in_src else src_in_build in
Build_system.eval_pred (File_selector.of_glob ~dir glob)
in
(* CR-someday amokhov: We currently traverse the set [files] twice: first, to
add the corresponding rules, and then to convert the files to [targets]. To
do only one traversal we need [Memo.parallel_map_set]. *)
Expand Down
13 changes: 12 additions & 1 deletion src/dune_rules/stanzas/copy_files.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ type t =
; mode : Rule.Mode.t
; enabled_if : Blang.t
; files : String_with_vars.t
; only_sources : bool
; syntax_version : Dune_lang.Syntax.Version.t
}

Expand All @@ -22,8 +23,17 @@ let long_form =
and+ mode = field "mode" ~default:Rule.Mode.Standard (check >>> Rule_mode_decoder.decode)
and+ enabled_if = Enabled_if.decode ~allowed_vars:Any ~since:(Some (2, 8)) ()
and+ files = field "files" (check >>> String_with_vars.decode)
and+ only_sources =
field_b "only_sources" ~check:(Dune_lang.Syntax.since Stanza.syntax (3, 14))
and+ syntax_version = Dune_lang.Syntax.get_exn Stanza.syntax in
{ add_line_directive = false; alias; mode; enabled_if; files; syntax_version }
{ add_line_directive = false
; alias
; mode
; enabled_if
; files
; only_sources
; syntax_version
}
;;

let decode =
Expand All @@ -38,6 +48,7 @@ let decode =
; mode = Standard
; enabled_if = Blang.true_
; files
; only_sources = false
; syntax_version
}
;;
1 change: 1 addition & 0 deletions src/dune_rules/stanzas/copy_files.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ type t =
; mode : Rule.Mode.t
; enabled_if : Blang.t
; files : String_with_vars.t
; only_sources : bool
; syntax_version : Dune_lang.Syntax.Version.t
}

Expand Down
27 changes: 27 additions & 0 deletions test/blackbox-tests/test-cases/copy_files/test6.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
Show that copy_files operates on the build folder, copying over e.g. .re.ml files

$ mkdir -p subdir
$ cat >dune-project <<EOF
> (lang dune 3.14)
> EOF
$ cat >subdir/dune <<EOF
> (library (name foo))
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 change the test not to rely on libraries? It should be easy enough to just use the files being copied as targets.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, i don't understand what you mean. The rule that has foo.re.ml as target is created by the library, afaiu. If I remove the library and try to do dune build subdir/foo.re.ml (which is what I assume the suggestion is about), dune will show Error: Don't know how to build subdir/foo.re.ml.

If I target the copied file foo.re directly, then I can't test the difference between regular copy_files behavior and the new behavior, because in both cases the target will be generated, because it's in the source.

Could you share a snippet of what you have in mind please?

Copy link
Member

@rgrinberg rgrinberg Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Something like this:

$ cat >target/dune <EOF
> (copy_files
>  (only_sources true)
>  (files foo/*))
> EOF
 
$ cat >foo/dune <<EOF
> (with-stdout-to in-build (echo ""))
> EOF

$ touch foo/in-source

$ dune build target/in-source # should pass
$ dune build target/in-build # should fail with unknown target
$  # now modify target/dune to set `(only_sources false)`
$ dune build target/in-build # should pass

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 tried this but get an error due to multiple rules generated:

  $ mkdir -p target foo
  $ cat >dune-project <<EOF
  > (lang dune 3.14)
  > EOF
  $ cat >target/dune <<EOF
  > (copy_files
  >  (files ../foo/*))
  > EOF
  $ cat >foo/dune <<EOF
  > (rule
  >  (with-stdout-to in-build
  >  (echo "")))
  > EOF

  $ touch foo/in-source

  $ dune build target/in-source # should pass
  Error: Multiple rules generated for _build/default/target/dune:
  - target/dune:2
  - file present in source tree
  Hint: rm -f target/dune
  [1]
  $ dune build target/in-build # should fail with unknown target
  Error: Multiple rules generated for _build/default/target/dune:
  - target/dune:2
  - file present in source tree
  Hint: rm -f target/dune
  [1]

Show the difference when `only_sources` is used

  $ cat >target/dune <<EOF
  > (copy_files
  >  (only_sources true)
  >  (files ../foo/*))
  > EOF

  $ dune build target/in-source # should pass
  Error: Multiple rules generated for _build/default/target/dune:
  - target/dune:3
  - file present in source tree
  Hint: rm -f target/dune
  [1]
  $ dune build target/in-build # should fail with unknown target
  Error: Multiple rules generated for _build/default/target/dune:
  - target/dune:3
  - file present in source tree
  Hint: rm -f target/dune
  [1]

Copy link
Member

Choose a reason for hiding this comment

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

I think you can just modify the glob to exclude dune. One simple way is to use an extension for your files and then select them with *.txt.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I updated the test in f81d57f.

> EOF
$ cat >subdir/foo.re <<EOF
> let t = 1
> EOF
$ cat >dune <<EOF
> (copy_files (files subdir/*.ml))
> EOF
$ dune build
$ ls _build/default | grep foo.re.ml
foo.re.ml

Show the difference when `only_sources` is used

$ cat >dune <<EOF
> (copy_files (only_sources true) (files subdir/*.ml))
> EOF
$ dune build
$ ls _build/default | grep foo.re.ml
[1]
Loading