-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add support for new output path format #29599
Add support for new output path format #29599
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
0dfec36
to
2b20308
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
2b20308
to
459c758
Compare
bdee25b
to
fc0df06
Compare
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.
Thank you for wrangling up all of those tests... wow that was a lot of hard-coded paths.
It looks like there are still a lot of test failures, including a few of the newer tests, so I won't look at the MSBuild code and try it out until those are fixed. Also hoping to get a little more engagement on the design spec
UseStandardOutputPaths
seems like it has a similar-ish problem in what is "standard" might change... (I am thinking of netstandard rids.)
Some brainstormed ideas:
UseSimplifiedOutputPaths
UseExperimentalOutputPaths
I like RootOutputPath
. Maybe also: FlatOutputPath
?
src/Tests/Microsoft.NET.TestFramework/ProjectConstruction/TestProject.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.NET.TestFramework/Commands/GetValuesCommand.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Sdk.BlazorWebAssembly.Tests/WasmBuildIntegrationTest.cs
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAFrameworkDependentApp.cs
Outdated
Show resolved
Hide resolved
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.
Very excited to see this land for previews and user feedback!
Re: the name of the opt-in property, I agree with @nagilson that Standard
's meaning can change over time, but I don't have a compelling alternative. Maybe UseUnifiedOutputPaths
? This is something we can potentially update across previews.
|
||
|
||
<!-- We need to put the UseStandardOutputPaths logic after the import of Directory.Build.props, but before the MSBuild Project Extensions .props import. | ||
However, both of these things happen in Microsoft.Common.props with no opportunity to insert logic in between them. |
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.
Do you think this is a gap in the layering that we should try to change for .NET 8? I see a few lines below that you suggest this - I think it would be worth doing for a later preview!
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.DefaultOutputPaths.targets
Outdated
Show resolved
Hide resolved
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.
Why not BuildDir
?
My proposals:
- Introduce
BuildDir
as a top-level directory for all things build - Fix output path customization limitations
I personally think we should start from existing paths and modify it to cover all the goals presented in the new proposal. I'm not good at explaining things over words, so I could create a PR on both MSBuild repo and here to explain the changes better!
For the |
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.DefaultOutputPaths.targets
Show resolved
Hide resolved
Can you add an example to your initial comment what default and opt-in will look like? That would help a lot. |
caf9af8
to
ebcbb81
Compare
@joeloff @nagilson @baronfel @rainersigwald I've updated the implementation here to (I believe) match the latest version of the design proposal. I still need to update tests and probably add some more test coverage, but the implementation is ready to review, I believe. |
030e11f
to
de35c57
Compare
@dsplaisted fyi, we had a bad branch creation for preview 3 so we deleted it and repushed with the right commit. I had to close and reopen your PR during that activity (and fixed a merge conflict) |
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 didn't look at all the tests, but have a few small comments on the MSBuild logic.
<PropertyGroup Condition="'$(UseArtifactsOutput)' == 'true' And '$(ArtifactsPath)' == '' And '$(_DirectoryBuildPropsBasePath)' != ''"> | ||
<!-- Default ArtifactsPath to be in the directory where Directory.Build.props is found | ||
Note that we do not append a backslash to the ArtifactsPath as we do with most paths, because it may be a global property passed in on the command-line which we can't easily change --> | ||
<ArtifactsPath>$(_DirectoryBuildPropsBasePath)\.artifacts</ArtifactsPath> |
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.
If you stick with .artifacts
I think you should add it to the .gitignore
template
sdk/template_feed/Microsoft.DotNet.Common.ItemTemplates/content/Gitignore/.gitignore
Line 19 in 09d8fec
# Build results |
<UseArtifactsOutput Condition="'$(UseArtifactsOutput)' == ''">true</UseArtifactsOutput> | ||
</PropertyGroup> | ||
|
||
<!-- Repeat ArtifactsPath logic from Sdk.props here, in case UseArtifactsOutput was set in the project file --> |
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.
Could we error in that case instead? I don't like the idea of accidentally diverging the implementation over time.
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.
So ArtifactsPath
just wouldn't be allowed in the project file at all?
We go through some hoops to make BaseIntermediateOutputPath
work whether it's set in the project file or beforehand. If it's set in the project file then it doesn't affect MSBuildProjectExtensionsPath
. This seemed analogous.
I just had a crazy idea: Maybe we could de-duplicate some of the logic by having a .props file that's conditionally imported in two different places depending on where ArtifactsPath
is defined?
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.
Doesn't sound crazy to me, sounds pretty smart 😄
@joeloff I think the main scenario is to put it in Directory.Build.props or to specify it on the command-line. If If you only specify the artifacts path on the command-line, then Visual Studio would build using the default (old) paths. The different output paths shouldn't interfere with each other though (the test that verifies that you can switch between output path formats helps verify that). Also, I think specifying the artifacts path on the command line may be more of a CI scenario so it isn't as likely to happen together with Visual Studio. |
Followup items:
|
It would also be good to de-duplicate the code for |
Add support for simplified output paths and
RootOutputPath
property.Based on this design spec: dotnet/designs#281. The design doc needs to be updated. The main changes from the current version:
bin
instead. This is discussed in this commentI needed a name for a property that would enable the new output paths. I went with
UseStandardOutputPaths
, which was the best I could think of. Better name suggestions are welcome.The property that puts all output for a solution under a single folder is
RootOutputPath
, which I think is a reasonably good name. Still open to suggestions though.