Support Common output via BuildDir and PublishDir
#6105
Conversation
|
Note for reviewers: |
There was a problem hiding this comment.
I'd like to avoid changing any defaults in making this, but I think this is a good start, though I haven't looked to see if I like all the whitespace changes. (I did see it was just a draft, but I wanted to try to do a little better on feedback than last time 😊)
@dsplaisted as the issue author.
There was a problem hiding this comment.
Why this change? Seems like it would confuse people. Did you mean to make it $(BuildDir)app.publish?
There was a problem hiding this comment.
No. This was intentional. Making publish a first-class citizen of the MSBuild process. So, I placed it in the root instead.
I've wanted to ask, why the app. prefix? why not just publish?
Is it because of the explorer's sorting that you wanted to make publish folder come first?
|
@Nirmal4G Can you remove the whitespace / formatting changes from this PR and submit them separately? That will make it a lot easier to review the changes related to the output directory. |
|
@Nirmal4G Thanks for your work on this. We try hard to avoid breaking changes in MSBuild. That means we can't change the output paths used for projects that haven't set We will also need to decide whether |
|
Actually, understanding this more, I don't think it really addresses #3497. What that issue tracks is a way to set a common output path for multiple projects (ie in a solution/repo). Then the project name would be automatically appended to that common output path (along with other components, such as I think that's more useful than having a single property that allows you to override both the output and intermediate paths for a single project. |
There was a problem hiding this comment.
My big concern so far is nailing down exactly what BuildDir should be. It sounds like BuildDir should just default to the root of the project? So no matter where it is, BaseOutputPath (bin) and BaseIntermediateOutputPath (obj) just plant themselves at (builddir)/bin or obj respectively.
Also, changing the pre-existing outputpath properties sounds pretty fishy to me. How realistic is it a dev would override these?
Thanks @Forgind for pointing out the 6th commit as the main one to look at.
|
Before I forget: please retarget to master. |
I agree and I do have a solution, same as I did in #5238, we can use a property to switch to a new behaviour when
This, I wanted to ask. Do we want to append to
Which directory structure do you prefer as the default?
I very much prefer if we remove the Besides, the change I made just relaxes BIOP and not constrain it. So, there'll be no-breaking change on this part but we do have previews to check and confirm it. |
|
To be clear, I'm against changing the default that If we're talking about simply allowing a user to set I'm not sure what the benefit of adding an
Where would these outputs go previously? in obj? |
#MeToo but yes there's a compelling argument to me is that having a sensible and standard output paths across different types of projects for variety of different use cases. I'll create a proposal based on what I learn from this PR. We can have the discussion there.
Yes, currently but I do propose we move it to a separate folder only when under build directory. All of this doesn't mean I want to remove existing behavior. |
Nirmal4G
changed the title
BuildDir and PublishDir
This reverts commit 0e4b0c2.
Make Common props, targets and tasks easier to read and understand.
Ensure they follow consistent formatting
E.g.: 2-space indent
```xml
<!-- Single Line Comment Text -->
<!--
Multi Line Comment Text
Another Comment Text
Indented Comment Text
-->
```
Make Common props, targets and tasks easier to read and understand. in all files: - Fix improper leading and trailing spacing of strings within quotes. in 'Microsoft.Common.props': - Move 'BaseIntermediateOutputPath' logic out of 'MSBuildProjectExtensionsPath' block. in 'Microsoft.Common.CrossTargeting.targets': - Remove temporary import depending on 'CoreCrossTargetingTargetsPath' property. in 'Microsoft.Common.CurrentVersion.targets': - Add single quotes to property names in text. - Set 'ProjectPath' to the now available 'MSBuildProjectDirectory'. - Simplified condition logic wherever based on 'OutputType' property. - Use 'ConfigurationName' property instead of 'Configuration' property.
Promote PublishDir to BuildDir status Use BuildDir to initialize MSBuildProjectExtensionsPath Use BuildDir for mismatch warning instead of BaseIntermediateOutputPath
When pointing build and publish outside of project directory, we need to check the same and append the project name to them so that the projects' outputs doesn't clash. There are no property functions available to check if a path is a parent of another path. So, we resort to using this hack until a suitable alternative is made available.
We don't need to split them but this will become crowded as we add output path features later. So, it is best to split them now. The order of checking the OutputPath properties is now reversed, meaning the early paths come first instead of the final path properties. This leads to fail early for the path properties in the order of importance.
Setting custom build and publish folder names is now enabled via 'BuildDirName' and 'PublishDirName'. The defaults are 'build' for build directory and 'publish' for publish directory.
1. *BaseIntermediateOutputPath* -> *BuildDir* 2. *ConfigurationAndPlatform* -> *OutputPaths* 3. ProjectExtensionsPath: *obj* -> *ext*
|
Closing this since I re-forked the repo, the branch ref to this PR was removed as well. I'll open a new PR soon. |
4 participants
Fixes #3497
Context
We as devs want simplified singular root output path across build and publish.
Changes Made
Promote
PublishDirtoBuildDirstatusUse
BuildDirto initializeMSBuildProjectExtensionsPathUse
BuildDirfor mismatch warning instead ofBaseIntermediateOutputPathTesting
TBD
Notes
This is just a concept of the new output paths behaviour using a new common directory to hold all the temporary build outputs. This is not the final implementation as this break previous behaviour. I'm still experimenting, whether to introduce a new property to split the behaviour, either within the targets or to a new targets file with an import switch. If we do favour the spilt, we can do it with #1686. The Sdk way easier since it also reduces duplicating the logic between .NET SDK and the Common targets.
Please hold up your reviews until it's out of draft.