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.

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 SourceFolders #8843

Merged
merged 42 commits into from
Dec 14, 2023
Merged

Copy task SourceFolders #8843

merged 42 commits into from
Dec 14, 2023

Conversation

jrdodds
Copy link
Contributor

@jrdodds jrdodds commented Jun 6, 2023

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.

@AR-May AR-May self-assigned this Jun 6, 2023
@jrdodds
Copy link
Contributor Author

jrdodds commented Jun 6, 2023

Can the build be re-run? Or is it necessary to push a new commit?

Update: Pushed a new commit to address a TODO.

The question remains. When the PR build fails for causes in the build infrastructure and not the repo, is there a way to re-run the build or request that the build be re-run?

In the backing Azure Pipelines, I can only view. In the GitHub desktop client, there is a "Re-run Checks" button. Clicking the button for the dotnet/msbuild repo does nothing.

@AR-May
Copy link
Member

AR-May commented Jun 8, 2023

Can the build be re-run? Or is it necessary to push a new commit?

You can write "/azp run" in the comment to the PR to re-run checks.

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 8843 in repo dotnet/msbuild

@rainersigwald
Copy link
Member

Yeah, only committers have permission for /azp run--and committers should use the finer-grained rerun capabilities in AzDO or GitHub to rerun only failed jobs that failed for known, tracked-by-bugs issues.

@rainersigwald rainersigwald added this to the VS 17.8 milestone Jun 22, 2023
@jrdodds
Copy link
Contributor Author

jrdodds commented Oct 27, 2023

Just a note that a unit test unrelated to the commit(s) failed and then on the next build passed.

build: Windows Core
test run: Microsoft.Build.Engine.UnitTests_net8.0_x64
unit test: Microsoft.Build.UnitTests.BackEnd.BuildManager_Tests.OutOfProcProjectInstanceBasedBuildDoesNotReloadFromDisk(shouldSerializeEntireState: True)

The log shows

error MSB4223: A node of the required type OutOfProc could not be created.

@jrdodds jrdodds requested a review from AR-May October 28, 2023 00:46
src/Tasks.UnitTests/Copy_Tests.cs Show resolved Hide resolved
src/Tasks/Copy.cs Outdated Show resolved Hide resolved
…onFiles list as TwoVectorsMustHaveSameLength error
@MichalPavlik
Copy link
Contributor

LGTM Merge after Alina's comments are addressed.

@jrdodds
Copy link
Contributor Author

jrdodds commented Nov 9, 2023

Just pushed a commit to address both comments

Copy link
Member

@AR-May AR-May left a comment

Choose a reason for hiding this comment

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

LGTM

@AR-May
Copy link
Member

AR-May commented Nov 9, 2023

Our main currently cannot take PRs with new features. We will merge this as soon as our main is open.

@AR-May AR-May merged commit 909ca6b into dotnet:main Dec 14, 2023
8 checks passed
@KirillOsenkov
Copy link
Member

Is the new capability documented anywhere? I’d like to see a couple of examples.

@AR-May
Copy link
Member

AR-May commented Dec 14, 2023

Feature is fully described in the issue with feature request:
#5881 (comment)
But it would be good idea to add documentation about it somewhere else too.

@KirillOsenkov
Copy link
Member

does the description in the issue match the actual implementation? I’m afraid documenting a feature in an issue is insufficient. Ideally if there’s existing documentation for the Copy task, we should augment that.

@AR-May
Copy link
Member

AR-May commented Dec 14, 2023

Yes, I agree, documenting in an issue is insufficient, we should add this to https://learn.microsoft.com/en-us/visualstudio/msbuild/copy-task.
Yes, the implementation matches the suggested design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy task should support copying a directory easily
5 participants