Add only_sources field to copy_files stanza

[jchavarri]

Fixes #9709.

Adds new stanzas copy_files_src and copy_files_src# so that only source files and folders are considered when matching the pattern passed to the stanza.

About the implementation, I tried to keep it as minimal as possible, building on top of existing copy_files rules handling. This means there are parts of the rules generation function that are useless in the copy_files_src case, like the if not exists_or_generated check. But I thought this is better than adding a whole function that will share a lot of code with the original one.

About the tests, I am not sure how to proceed, maybe it's worth duplicating all existing copy_files tests with the new stanza, to see how it behaves?

[jchavarri]

Another random thought for the future, is if it'd be desirable to deprecate copy_files so it gets renamed to copy_files_bld or copy_files_build, so that it's clearer for users the behavior of each stanza.

[emillon]

About syntax: could the distinction be expressed as a new field in copy_files? such as:

(copy_files
 (only_sources)
 (files subdir/*.ml))

Or even:

(copy_files (source_files subdir/*.ml))

[jchavarri]

About syntax: could the distinction be expressed as a new field in copy_files?

Sure, added in 8434970.

[rgrinberg]

Could you change the test not to rely on libraries? It should be easy enough to just use the files being copied as targets.

[jchavarri]

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?

[rgrinberg]

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

[jchavarri]

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]

[rgrinberg]

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.

[jchavarri]

Thanks, I updated the test in f81d57f.

[rgrinberg]

This field should be available starting 3.14

[jchavarri]

12cd755

[jchavarri]

n/m this was a larger change that I expected, created #9840 for it, should be merged first.

[nojb]

About syntax: could the distinction be expressed as a new field in copy_files? such as:

(copy_files
 (only_sources)
 (files subdir/*.ml))

Or even:

(copy_files (source_files subdir/*.ml))

Sorry for coming late to the discussion, but I think (source_files ...) is somewhat better than (only_sources) which feels less compositional (eg in the future we could support multiple globs in a single copy_files stanza, both from source and build directory, etc).

[jchavarri]

@nojb I am not sure about the suggested approach. Note that copy_files also support a short form: (copy_files <glob>). What would be the behavior in that case if we add a source_files field to the stanza?

With the current approach using only_sources boolean field, there is no ambiguity, as there is only a single field where the glob is defined.

in the future we could support multiple globs in a single copy_files stanza, both from source and build directory

As this is already achievable using two copy_files stanzas, it is not a much needed feature (for me / imo).

[jchavarri]

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.

[jchavarri]

It's a bit surprising that this version is stored in dune-rpc library 🤔

[nojb]

@nojb I am not sure about the suggested approach. Note that copy_files also support a short form: (copy_files <glob>). What would be the behavior in that case if we add a source_files field to the stanza?

Generally speaking, the short form is legacy; it dates from when the (copy_files) stanza had only one field; I don't think we should support new additions to the short form.

For the question at hand ((copy_files (only_sources) (files ...)) vs (copy_files (source_files ...))): even though I have a slight preference for the latter one, I don't have a strong argument for it, so feel free to go with your intuition and what the other devs think. Just a remark: the name only_sources suggests that the files copied with (only_sources) are a subset of those copied without it, but if I'm not wrong, this is not quite what the implementation is doing, rather, we are expanding the glob in the source directory rather than in the build directory, and this could result in extra files (which are never copied to the build directory) being considered. Is this correct, or am I missing something?

[jchavarri]

Just a remark: the name only_sources suggests that the files copied with (only_sources) are a subset of those copied without it, but if I'm not wrong, this is not quite what the implementation is doing, rather, we are expanding the glob in the source directory rather than in the build directory, and this could result in extra files (which are never copied to the build directory) being considered. Is this correct, or am I missing something?

Your assumption is correct, I hadn't thought of that scenario. Maybe we could rename it to from_source? Assuming we don't go with source_files... I don't have a strong opinion either, and I didn't know the short form was legacy. Curious to see what @emillon and @rgrinberg think.

[rgrinberg]

I wouldn't say the short form is legacy, but it certainly doesn't need to support all the features of the full stanza.

rather, we are expanding the glob in the source directory rather than in the build directory, and this could result in extra files (which are never copied to the build directory) being considered. Is this correct, or am I missing something?

When could this make a difference? Sources files in a directory are always a subset of the targets in that directory. Note that copying things to the build directory is nothing more than setting up a copy rule from the source to the build dir.

[rgrinberg]

LGTM apart from the usual boilerplate:

• Reference docs
• CHANGES entry
  No preference for whether to use a field or a constructor. I see no issues with using multiple copy_files stanzas if it's not possible to express everything in a single stanza.

[jchavarri]

LGTM apart from the usual boilerplate:

Added in 6166c80.

[jchavarri]

@emillon Thanks. I think sources is better, updated the PR to it in dd2bd16.

[rgrinberg]

The previous form actually had a concrete advantage of the current, you could extend it to enable/disable it with pforms. E.g.

(copy_files
 (only_sources (= %{os} macosx)
 (glob *))

From my experience, the above ends up being useful for many fields. So I would recommend to not forbid it.

[jchavarri]

The previous form

@rgrinberg I understand you are referring to the field rename from only_sources to sources. Reverted in d1ee4b7.

[emillon]

(only_sources (= %{os} macosx)) is only possible if this field has type Blang.t and is expanded later. Is there a motivating use case for this?

[rgrinberg]

There's no motivating use case, but usually what ends up happening is that somebody requires the blang functionality eventually and then we end up having to add more versioning code to add blang support. Adding such versioning code is usually about as much work as just allowing blang. So yes I'd prefer to keep the language more uniform and allow toggles to take blangs

[jchavarri]

you could extend it to enable/disable it with pforms

@rgrinberg Just a clarification, are you expecting this extension to be part of this PR as well? Or were you referring purely to the naming (only_sources seems to fit better the pform usage than sources).

[emillon]

I just added the blang part, with a short syntax for true. @rgrinberg how does it look?