Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ArPow stage 1: local source-build infrastructure #53294
ArPow stage 1: local source-build infrastructure #53294
Changes from 44 commits
1054536
7d0c956
376c10c
8a4c258
9d474ce
06f1d5e
0e4d155
cc4124f
59fb50e
ad4201b
1c87b39
c4f711d
479c95a
2565431
cd313cd
2b5307d
d7546c5
d6602eb
843caa7
a7d8b95
18d9616
f8e8475
aa13d3c
38cea15
f5445ee
4d94fd4
0a8059d
9b2c5b3
1b2d9b1
23e4735
5a12eaf
e0c3357
658dea4
de586df
e9b7fe4
469f6a1
6c2e477
1c97495
e6f5d8f
f026627
371a532
58bb935
309421d
7230fa1
972035a
f905b0e
0124b4f
cf48c62
4211c03
88a1715
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - I logged an issue for having the ArPOW infrastructure handle large packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing this with @dseefeld, we believe this patch is obsolete because
dotnet-pgo.csproj
is now excluded when building source-build.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This usage can be deleted if this project just uses the package's props to define the task: https://github.com/dotnet/arcade/blob/13bb7c843fac9ce3652b5f418ca27b9bf3b83754/src/Microsoft.DotNet.Build.Tasks.Packaging/src/build/Packaging.common.targets#L44
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently necessary because of dotnet/arcade#7413
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, should be simple enough to write those targets with a template task? At the very least, why isn't this patch in Arcade instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you patch in Arcade? For whatever reason, the
Microsoft.DotNet.Build.Tasks.Packaging
package doesn't expose a property that points to the msbuild task assembly.I wouldn't want to invest any resources in fixing this aside from inlining the patch into main as this code-path will go away when @Anipik finished the pkgproj migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The project can't use the package's props or targets files as it contributes to the project's restore. This runs as part of restore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My impression based on your comment
This is currently necessary because of dotnet/arcade#7413
was that the props/targets didn't have the right path.If the props have the right path, then this proj file can just import the props/targets. Since all this proj does is define a task which those targets already define.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be downloaded before that, I think, as part of
runtime/eng/Tools.props
Line 9 in 9199eb6
I'd prefer to make things less hacky if possible though, was hoping your conversion of this to a traversal project would just let it naturally consume the package and let us delete stuff. If that's not the case then I'm fine leaving this since it'll go away with PkgProj removal, as you said.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that's how the package is available before the project's restore to calculate the PackageDownload items. Libraries-packages.proj itself would need to manually import the props and targets files which IMO isn't great either. Let's just keep it as-is for now and apply the patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dseefeld why is this necessary? Unfortunately the comment or the commit message doesn't provide any details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that since the project was only supported on Windows it wasn't necessary for source-build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it hurt anything to build it? We have plenty of other things that are the same and they don't get excluded from source build. https://github.com/dotnet/runtime/tree/main/src/libraries/System.ServiceProcess.ServiceController for example.
I'd recommend just deleting this patch if all it is doing is producing one less package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More than likely it was introducing prebuilts. Until we get the prebuilt report cleaned up during this ArPow effort it is difficult to detect. @dseefeld, are you alright with removing the patch until we can prove it is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integrating this patch with #54222.