-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Copy task should support copying a directory easily #5881
Comments
Microsoft.Build.Artifacts can do this, as a workaround. Still agree it would be nice for the copy to be built in and something like Microsoft.Build.Artifacts would just call it. |
Ugh, we should really fix this. Copying some files into a directory and preserving the directory structure is super painful. |
I'm imagining two new properties, SourceRoot and DestinationRoot, that when set, act as the corresponding roots, and the relative path from SourceRoot is preserved for each file. |
This way this won't be a breaking change. |
or maybe just |
When I eventually come back here, here's how to do it:
|
Another issue is that with the current approach you can't copy symbolic links from a source to a destination directory, the link gets resolved and the actual file copied instead of the symlink. This will probably need to wait for BCL support (dotnet/runtime#24271) though. |
@KirillOsenkov did you try Microsoft.Build.Artifacts? |
No, I’m using vanilla MSBuild. |
I just corrected the snippet I posted earlier to expand the globs during execution (inside the target), and not during evaluation. When it ran during evaluation the project may be evaluated too early, and the glob might not pick up the files which are copied later by another target. Expanding the glob from inside the target is more reliable. |
Task batching is idiomatic in MSBuild and the documentation of the Copy task includes an example of copying a directory. Speaking for myself, I don't find it 'gnarly'. But I do understand that MSBuild has a problem where many people have expectations of how MSBuild works and behaves that are very different from how MSBuild actually works and behaves. And I understand that, for various reasons, batching often falls into that gap. This feature request is not about adding functionality that the Instead of extending the Like The |
I personally don't see anything wrong with adding another parameter to A brand new In fact the danger with adding CopyDir is that people might already have their own custom/third-party CopyDir task, so adding the in-the-box one will mess with their builds and is a breaking change we can't afford. Whereas adding SourceFolder to Copy is not a breaking change. |
I disagree that
There is general consensus that a function or class method should be limited in the number of arguments. The same principle applies here. But we can agree that we just see that differently and that, among all the possible trade-offs, it may be minor. Some of your points can be generalized as essentially that no new tasks should ever be added. I understand your comments are about adding a Related to the re-design of the From your description I assume you are envisioning The following would be accepted?
And the following would be errors?
|
@jrdodds You're taking things off-topic. The feedback is that the current behavior feels limited and counterintuitive. There is perhaps unfortunately a very opinionated suggested solution @KirillOsenkov brought up, but that should be fine; maintainers can take the solution or not.
Do you have data and actual numbers on this? This sounds like an opinion being framed as a universal truth. This response is starting to argue about the suggested solution rather than keeping the thread focused on the feedback. |
Hi @mikerochip, Regarding the number of arguments to a function: No, not a universal truth but it is a long-standing coding guideline (at least in my understanding and experience). If you would like a citation that shows this is not a personal opinion of my own invention, here is a SonarQube rule for C#: Methods should not have too many parameters. My apologies if my response came across as argumentative. My intent was to be constructive and to explore the issues. If I decide to take this issue or pass on this issue, I want to be clear about the changes. |
Appreciate the clarification @jrdodds . My only horse in this race is that I have to do this exact thing (copy source folder into target folder, maintain directory structure) about once every year and a half, and I forget how to do it every time, and google searching brings me here, so I watched this issue. I don't have a strong opinion on what the solution is, but I like @KirillOsenkov's solution. Yours would do the trick as well. My main desire is getting to the point where this task is intuitive enough where I can just look at the Copy task docs and be on my way, rather than google search and end up in a years-long unresolved thread; I think I and many other folks on GitHub probably have experienced that more times than we want 😂 . At least for this issue there are options and this issue is effectively just that the current state isn't as ergonomic as it could be. |
@mikerochip It may be a recent change (I haven't looked at the history) but Example 2 in the Copy task documentation is what you need to copy a directory. |
Invoking the
Task batching will invoke the The |
Design ProposalThis issue is currently tagged as 'needs-design'. This is a possible way that the feature request could be satisfied. BackgroundFor source and destination, the
The following are accepted: <Copy SourcesFiles="AFile;BFile" DestinationFolder="BDirectory" />
<Copy SourcesFiles="AFile;BFile" DestinationFiles="CFile;DFile" /> ❌ The following are errors: <Copy SourcesFiles="AFile;BFile" DestinationFolder="BDirectory" DestinationFiles="CFile;DFile" />
<Copy SourcesFiles="AFile;BFile" DestinationFiles="CFile" /> Sources and destinations are resolved into calls to either Enhancement to the Copy Task to copy DirectoriesTask ParametersAdd a
It would be an error if neither A table of which parameters could be used together:
The following would be accepted: <Copy SourceFolders="ADirectory;BDirectory" DestinationFolder="CDirectory" />
<Copy SourceFolders="ADirectory;BDirectory" SourcesFiles="AFile;BFile" DestinationFolder="CDirectory" /> ❌ The following would be errors: <Copy SourceFolders="ADirectory;BDirectory" DestinationFiles="CFile;DFile" />
<Copy SourceFolders="ADirectory;BDirectory" DestinationFiles="CFile;DFile" DestinationFolder="CDirectory" />
<Copy SourceFolders="ADirectory;BDirectory" SourcesFiles="AFile;BFile" DestinationFiles="CFile;DFile" /> Given directories <Copy SourceFolders="c:\example\target" DestinationFolder="c:\example\work" /> This matches the existing behavior of For each directory in The following example shows how files and folders from a 'root' directory could be dynamically 'discovered' and copied without copying the 'root'. <PropertyGroup>
<SourceRootDirectory>c:\example\target</SourceRootDirectory >
</PropertyGroup>
<ItemGroup Condition="Exists('$(SourceDirectory)')">
<SrcDirectories Include="$([System.IO.Directory]::GetDirectories($(SourceRootDirectory)))" />
<SrcFiles Include="$(SourceRootDirectory)\*.*" />
</ItemGroup>
<Copy SourceFolders="@(SrcDirectories)" SourcesFiles="$(SrcFiles)" DestinationFolder="c:\example\work" /> InternalsNew validations and errors would need to be added. When ❓ Should empty directories be copied? With the following code, empty directories are not included and are not copied. <ItemGroup>
<MySourceFiles Include="c:\MySourceTree\**\*.*"/>
</ItemGroup>
<Copy SourceFiles="@(MySourceFiles)" DestinationFolder="$(DestDir)\%(RecursiveDir)" /> But |
MSBuild Team triage: we like the proposal, @jrdodds.
@Forgind and I lean toward "yes, copy empty directories". It feels more intuitive, and if it is desired but not done, it's a pain to recreate the behavior with manual directory creation. This may require a bit more effort in the implementation though so we can revisit if it's horrible. Removing the @jrdodds are you interested in contributing the implementation? |
Let's elaborate on concretely, if I have SourceFolders="C:\A\A1;C:\B\B1" and the DestinationFolder="C:\D", what is the resulting layout on disk? C:\D\A1 and C:\D\B1 or are the contents of A1 and B1 copied directly to D? If it's not immediately clear to me, it is probably a source of confusion to many. Whereas copying from a single source directory to a single destination directory is well defined and understood semantics used by all the tools like cp, copy, xcopy, etc. It means exactly "take all contents of the source and copy it to the destination". Basically I want to work exactly like robocopy would (but no /MIR of course). |
Yes. Thank you. |
@KirillOsenkov Given <Copy SourceFolders="C:\A\A1;C:\B\B1" DestinationFolder="C:\D" /> The result would be C:\D\A1 and C:\D\B1. To quote from the proposal:
The intent is to extend the current The current
|
OK and how do I achieve the semantics that I want? Copy all files and directories from Source to Destination? Again, if I understand your proposal correctly, it’s highly unintuitive and unexpected. |
BTW this is only tangentially related, but I wrote a tool called ContentSync that copies one directory to another one incrementally (similar to robocopy /MIR), but if a file contents are identical it doesn't touch the file (it uses content hash instead of timestamps). |
@JaynieBai Please assign this issue to me. Thanks |
Not an answer or solution to this feature request but here is something to help with the current "super painful" task of copying the content of a folder into another folder. Given two properties, <ItemGroup>
<FilesToCopy Include="$([MSBuild]::EnsureTrailingSlash('$(SourceFolder)'))**\*.*"/>
</ItemGroup>
<Copy SourceFiles="@(FilesToCopy)" DestinationFolder="$([MSBuild]::EnsureTrailingSlash('$(DestinationFolder)'))%(RecursiveDir)"/> Save the above as a code snippet and, when you need to copy a folder, paste this snippet and replace the property names as needed. The above code is essentially Example 2 in the Copy task documentation. You can copy from the example. Either way, if you find it difficult, there is no need to write this from scratch every time. |
I was asked to take a look at this from the design perspective now that we are getting close to an implementation, so here I am. First off, thank you for writing such a detailed spec comment/example @jrdodds - that made it very easy to work through the scenarios you had in mind. I agree with @KirillOsenkov that at first glance the subtleties of |
Fixes #5881 Context Implementation of proposed ITaskItem[] SourceFolders parameter. Changes Made Within Copy.cs: Added ITaskItem[] SourceFolders parameter Modified ValidateInputs() to support the new parameter Modified InitializeDestinationFiles() Previously this method iterated SourceFiles to create DestinationFiles items given the DestinationFolder. The Copy task actually only operates on SourceFiles and DestinationFiles. Extended the method to: Use FileMatcher::GetFiles() for each directory in SourceFolders Create items in both SourceFiles and DestinationFiles for each file found Added additional unit tests in Copy_Tests.cs. Added new error MSB3894 Copy.IncompatibleParameters for the case where SourceFolders and DestinationFiles are both present. This is very close to the existing MSB3022 Copy.ExactlyOneTypeOfDestination but it seemed prudent not to re-use MSB3022. Testing Tested on Windows 11 and macOS 12 Ran full suite of unit tests from the command line Ran some sample project files Notes This implementation conforms to the proposed design with the exception that it does not copy empty directories. This implementation leverages the existing FileMatcher::GetFiles() which recurses the directory tree using multiple threads. GetFiles() returns a list of files. Empty directories are not identified. The use of FileMatcher could be replaced. The Copy task could implement its own support for recursing a directory tree. The wildcard matching logic in FileMatcher is not relevant for the Copy task.
It seems unfortunate that you need to specify folders and files to copy separately, if you want to copy both. E.g., in the example above:
It seems like it would be more uniform to have a single "Sources" list that contains either directories/folders or files, and tree copies the directories or individually copies the files, as appropriate. E.g.,
This way, if you have an Item that contains a mix, you can use:
|
@BruceForstall That would be nice but it's not a feasible option for the existing Internally the |
Well, there's no need to rename |
Sure, a separate As a quick sketch:
|
Copy task doesn't make it easy to copy the entire directory, it forces you to mess with wildcards etc. and it's gnarly
We should consider adding a new SourceFolder parameter on the Copy task that, if specified, pre-populates the SourceFiles with the entire directory. Of course SourceFolder and SourcesFiles should be mutually exclusive.
The text was updated successfully, but these errors were encountered: