Deprecate configure_file(copy : true), and replace it with fs.copyfile #10042

dcbaker · 2022-02-28T23:00:17Z

This is based on #9897

It's a bit of a historical accident that configure_file is used for copying, mainly because it was able to happen because of loose argument handling, and that we didn't yet have an fs module to be a clear, obvious, place to put such a helper. The copyfile method is basically synonymous, but with the added value of not requiring an output keyword argument.

codecov · 2022-02-28T23:10:32Z

Codecov Report

Merging #10042 (867c823) into master (f938861) will decrease coverage by 1.60%.
The diff coverage is n/a.

❗ Current head 867c823 differs from pull request most recent head 59f99dc. Consider uploading reports for the commit 59f99dc to get more accurate results

@@            Coverage Diff             @@
##           master   #10042      +/-   ##
==========================================
- Coverage   68.89%   67.28%   -1.61%     
==========================================
  Files         406      203     -203     
  Lines       88236    44133   -44103     
  Branches    19593     9789    -9804     
==========================================
- Hits        60790    29697   -31093     
+ Misses      22849    12171   -10678     
+ Partials     4597     2265    -2332

Impacted Files	Coverage Δ
modules/cuda.py	`0.00% <0.00%> (-72.64%)`	⬇️
templates/cudatemplates.py	`37.50% <0.00%> (-62.50%)`	⬇️
compilers/cuda.py	`19.63% <0.00%> (-45.40%)`	⬇️
dependencies/cuda.py	`20.19% <0.00%> (-42.79%)`	⬇️
compilers/mixins/clang.py	`56.09% <0.00%> (-14.64%)`	⬇️
scripts/coverage.py	`56.43% <0.00%> (-7.93%)`	⬇️
compilers/detect.py	`41.95% <0.00%> (-5.02%)`	⬇️
linkers/linkers.py	`56.47% <0.00%> (-1.27%)`	⬇️
environment.py	`78.98% <0.00%> (-1.07%)`	⬇️
modules/fs.py	`84.18% <0.00%> (-1.01%)`	⬇️
... and 212 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

eli-schwartz · 2022-02-28T23:22:54Z

I like the idea of this, but I hate the implementation. Actually, I hate the configure_file implementation which this doesn't fix.

configure_file originally assumed that the input file needs to be modified by a configuration_data, for obvious reasons. Also for obvious reasons, this needs to be done at configure time, and any changes to the file template almost certainly entail changes to meson.build logic too. None of this applies to copy: true, it just inherited a weird design.

Could we have this generate a build.ninja rule to do the copying? I really dislike doing file copying at configure time and then having edits to the copied file force a full build system reconfigure. And I'm pretty sure I've seen people just make custom_target with Unix cp as the command because they didn't like this either.

docs/markdown/Fs-module.md

jpakkane · 2022-03-01T10:29:53Z

The design reason for configure_file was that the output can be used in any target without needing to add it to the list of sources manually. This is how config.h works and is generally what people expect.

Maybe this could have a kwarg specifying whether it should be done at build time or configure time (if the former is something we really want to support)?

tristan957 · 2022-03-01T17:58:54Z

I like fs.copy() better than fs.copyfile() since it is a more futureproof name in my opinion although on UNIX everything is a file :).

eli-schwartz · 2022-03-02T00:33:21Z

The design reason for configure_file was that the output can be used in any target without needing to add it to the list of sources manually. This is how config.h works and is generally what people expect.

Right, so copy: true inherited that design limitation because it would be somewhat unfortunate to have configure_file sometimes return a File and sometimes return a CustomTarget.

Maybe this could have a kwarg specifying whether it should be done at build time or configure time (if the former is something we really want to support)?

... I don't really think it makes a lot of sense to make this optional? We can just make it return a CustomTarget. It is trivially added as a source to any build target, or to a declare_dependency, so we can guarantee it is up to date. Considering a fairly common use case of copy: true is "fix cmake projects which have headers in the wrong directory layout" I think people will always want the latter, never the former, because it's hardly unlikely for headers to update in a regular git pull.

xclaesse · 2022-03-02T00:50:26Z

If we copy at configure time it means we have to reconfigure the whole project each time the file is touched, instead of just rebuilding some targets.

Also I think it would make sense to take a list of files that all goes to the same destination. Useful for the headers use case.

xclaesse · 2022-03-02T00:51:53Z

Note that if we copy at build time, maybe we could keep configure_file() not deprecated for doing it at configure time.

ilayn · 2022-05-12T17:25:47Z

I am not sure if this is the right place to ask this but currently, due to reasons beyond me, we have to copy certain files to build dir because Cython is running on gears that are not fully greased as far as I can tell. Hence I used the advice of using custom_target with cp however that is not flying on Windows hence I resorted back to

configure_file(input : '_cythonized_array_utils.pxd',
               output : '_cythonized_array_utils.pxd',
               configuration : configuration_data())

after reading #860. I don't know why this even works for copying but apparently it does. However now I get this warning, but I don't know how to use it

 WARNING: Got an empty configuration_data() object and found no substitutions in the input file '_cythonized_array_utils.pxd'. If you want to copy a file to the build dir, use the 'copy:' keyword argument added in 0.47.0

is it supposed to be used as the following ?

configure_file('_cythonized_array_utils.pxd'
                copy: True)

But now, this issue says it might be deprecated. What is the canonical way to copy files ?

tristan957 · 2022-05-12T17:54:44Z

@ilayn instead of configuration kwarg, use the copy kwarg in your original code block

dcbaker · 2022-08-11T18:05:46Z

@eli-schwartz, the only case I can come up with for when configure_time is necessary, is if you want to use the output for a run_command

dcbaker · 2022-08-11T18:43:25Z

I've reworked the fs.copyfile to be a CustomTarget as requested by @eli-schwartz, and addressed @jpakkane's concerns about outputs having path seperators (both in the docs and in the code). I'm still not sure what to do about configure_time copying, because I did run into a case where you want that, and it's run_commnad.

mesonbuild/modules/fs.py

docs/markdown/Fs-module.md

mesonbuild/modules/fs.py

eli-schwartz

⚔️ conflicts

docs/markdown/Fs-module.md

`configure_file` is both an extremely complicated implementation, and a strange place for copying. It's a bit of a historical artifact, since the fs module didn't yet exist. It makes more sense to move this to the fs module and deprecate this `configure_file` version. This new version works at build time rather than configure time, which has the disadvantage it can't be passed to `run_command`, but with the advantage that changes to the input don't require a full reconfigure.

In our meson scripts, we use configure_file(copy:true) to copy files from srcdir into builddir. However, as of meson-0.64.0, this is deprecated [1] in favor of using: fs = import('fs') fs.copyfile(in, out) Except, the submodule's new method wasn't introduced until 0.64.0. And since we can't bump the minimal meson version we require, we have to work with both: new and old versions. Now, the fun part: fs.copyfile() is not a drop in replacement as it returns different type (a custom_target object). This is incompatible with places where we store the configure_file() retval in a variable to process it further. While we could just replace 'copy:true' with a dummy 'configuration:...' (say 'configuration: configmake_conf') we can't do that for binary files (like src/fonts/ or src/images/). Therefore, places where we are not interested in the retval can be switched to fs.copyfile() and places where we are interested in the retval will just use a dummy 'configuration:'. Except, src/network/meson.build. In here we not just copy the file but also specify alternative install dir and that's not something that fs.copyfile() can handle. Yet, using 'copy: true' is viewed wrong [2]. 1: https://mesonbuild.com/Release-notes-for-0-64-0.html#fscopyfile-to-replace-configure_filecopy-true 2: mesonbuild/meson#10042 Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

In our meson scripts, we use configure_file(copy:true) to copy files from srcdir into builddir. However, as of meson-0.64.0, this is deprecated [1] in favor of using: fs = import('fs') fs.copyfile(in, out) Except, the submodule's new method wasn't introduced until 0.64.0. And since we can't bump the minimal meson version we require, we have to work with both: new and old versions. Now, the fun part: fs.copyfile() is not a drop in replacement as it returns different type (a custom_target object). This is incompatible with places where we store the configure_file() retval in a variable to process it further. While we could just replace 'copy:true' with a dummy 'configuration:...' (say 'configuration: configmake_conf') we can't do that for binary files (like src/fonts/ or src/images/). Therefore, places where we are not interested in the retval can be switched to fs.copyfile() and places where we are interested in the retval will just use a dummy 'configuration:'. Except, src/network/meson.build. In here we not just copy the file but also specify alternative install dir and that's not something that fs.copyfile() can handle. Yet, using 'copy: true' is viewed wrong [2]. 1: https://mesonbuild.com/Release-notes-for-0-64-0.html#fscopyfile-to-replace-configure_filecopy-true 2: mesonbuild/meson#10042 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

dcbaker requested a review from jpakkane as a code owner February 28, 2022 23:00

jpakkane reviewed Mar 1, 2022

View reviewed changes

docs/markdown/Fs-module.md Outdated Show resolved Hide resolved

eli-schwartz mentioned this pull request Jun 10, 2022

Wip/jehan/special case set4glib #6267

Open

dcbaker force-pushed the submit/fs-copy-file branch from 4474301 to 06e8eae Compare August 11, 2022 18:41

eli-schwartz requested changes Aug 11, 2022

View reviewed changes

mesonbuild/modules/fs.py Outdated Show resolved Hide resolved

dcbaker force-pushed the submit/fs-copy-file branch 2 times, most recently from 142d8b0 to 2838ec7 Compare August 11, 2022 19:22

dcbaker added this to the 0.64.0 milestone Aug 15, 2022

eli-schwartz reviewed Aug 16, 2022

View reviewed changes

docs/markdown/Fs-module.md Outdated Show resolved Hide resolved

eli-schwartz reviewed Aug 16, 2022

View reviewed changes

mesonbuild/modules/fs.py Outdated Show resolved Hide resolved

eli-schwartz reviewed Aug 16, 2022

View reviewed changes

mesonbuild/modules/fs.py Outdated Show resolved Hide resolved

dcbaker force-pushed the submit/fs-copy-file branch from 2838ec7 to ecf1547 Compare August 17, 2022 17:03

eli-schwartz requested changes Aug 17, 2022

View reviewed changes

docs/markdown/Fs-module.md Outdated Show resolved Hide resolved

dcbaker force-pushed the submit/fs-copy-file branch from ecf1547 to 151bbce Compare August 18, 2022 16:45

dcbaker force-pushed the submit/fs-copy-file branch from 151bbce to 59f99dc Compare August 18, 2022 16:47

eli-schwartz approved these changes Aug 18, 2022

View reviewed changes

eli-schwartz merged commit 991baf5 into mesonbuild:master Aug 18, 2022

dcbaker deleted the submit/fs-copy-file branch August 18, 2022 21:29

ibc mentioned this pull request Mar 4, 2024

Branch libuv-v1.48.0 fails in MSVC due to 'spawn_no_ext' test failing ibc/wrapdb#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate configure_file(copy : true), and replace it with fs.copyfile #10042

Deprecate configure_file(copy : true), and replace it with fs.copyfile #10042

dcbaker commented Feb 28, 2022

codecov bot commented Feb 28, 2022 •

edited

eli-schwartz commented Feb 28, 2022

jpakkane commented Mar 1, 2022

tristan957 commented Mar 1, 2022

eli-schwartz commented Mar 2, 2022 •

edited

xclaesse commented Mar 2, 2022

xclaesse commented Mar 2, 2022

ilayn commented May 12, 2022 •

edited

tristan957 commented May 12, 2022

dcbaker commented Aug 11, 2022

dcbaker commented Aug 11, 2022

eli-schwartz left a comment

Deprecate configure_file(copy : true), and replace it with fs.copyfile #10042

Deprecate configure_file(copy : true), and replace it with fs.copyfile #10042

Conversation

dcbaker commented Feb 28, 2022

codecov bot commented Feb 28, 2022 • edited

Codecov Report

eli-schwartz commented Feb 28, 2022

jpakkane commented Mar 1, 2022

tristan957 commented Mar 1, 2022

eli-schwartz commented Mar 2, 2022 • edited

xclaesse commented Mar 2, 2022

xclaesse commented Mar 2, 2022

ilayn commented May 12, 2022 • edited

tristan957 commented May 12, 2022

dcbaker commented Aug 11, 2022

dcbaker commented Aug 11, 2022

eli-schwartz left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 28, 2022 •

edited

eli-schwartz commented Mar 2, 2022 •

edited

ilayn commented May 12, 2022 •

edited