-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Support common output directory root in SDK out of the box #867
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
Comments
Very much agree. This is pushing developers to have a correct build by default. By correct I mean a build where by a given output path is only written to exactly once. Should make this as easy as possible for developers to opt into. Note: Why use |
@jaredpar |
I don't really see a difference between project and solution level settings. The only real difference is whether MSBuild is targeting a solution or a project during a build. Concrete example: Assuming Util.sln contains Util.csproj I would expect the following commands to produce the same output: > msbuild Util.sln /p:BaseOutputPath=c:\test
> msbuild Util.csproj /p:BaseOutputPath=c:\test |
The difference is that the solution level path ( |
That doesnt line up with the recomendations we came away with when meeting with various teams last week. In that meeting |
Are you saying that the project name currently does get automatically added to it, or that that's what should happen? |
What should happen. The discussion we had centered around Concretely: suppose my code exists at > msbuild Roslyn.sln If my output was all under > msbuild Roslyn.sln /p:BaseOutputPath=e:\example Should put all of my output under |
We need to pick a different name and I like Tomas' choice of RootOutputPath. BaseOutputPath is patterned after BaseIntermediateOutputPath and we can't change the latter's long-existing meaning. There should also be RootIntermediateOutputPath for parity. The logic could be something like:
Ditto for s/Output/IntermediateOutput/;s/bin/obj/ So you can /p:BaseIntermediateOutputPath and /p:BaseOutputPath do what they do now, and /p:RootOutputPath and /p:RootIntermediateOutputPath exhibit the behavior desired here. |
What is the long term existing meaning? When discussed with MSBuild they were receptive of the semantics I outlined. |
@nguerrera Agreed. In the proposal above I actually took it one step further and used a single variable I'd be fine with either. |
BaseIntermediateOutputPath never gets a project name appended to it to produce IntermediateOutputPath |
@jaredpar All the properties that are set currently are per-project: either they include the project name, or are relative to the project directory. |
Again I don't see a difference between per project and per solution. From the perspective of build it's impossible to distinguish between the two of them.
Again opposite of what we discussed the other day. |
I wasn't there. Regardless, existing behavior cannot be up for discussion ;) |
There is nothing to do with solutions here. The question is whether a property implies appending a project-name on the way to the final per-project output. They would behave identically when invoked by building individual projects or traversing to them from sln. |
Then I do not understand the significance of pointing out items are per project. |
In this context: Per project means: the path is the path, there is no requirement for the sdk or msbuild to append project name to it to prevent collisions. Either user arranges to have project name in it or to keep it relative to csproj as it is by default. (Also GIGO: if you don't arrange accordingly, you can get incorrect build that stomps on itself). This is BaseXxx. Not per project means: I can safely specify this property with a shared value and msbuild or sdk will arrange to append a project disambiguator to it when it is used in the context of any given project. This is new and we can invent RootXxx for it. |
This is how I would set per-project variables based on root variables: Set by the user globally: <RepoRoot>$([System.IO.Path]::GetFullPath('$(MSBuildThisFileDirectory)'))</RepoRoot>
<ArtifactsDir>$(RepoRoot)artifacts\</ArtifactsDir>
<RootOutputPath>$(ArtifactsDir)$(Configuration)\bin\</RootOutputPath>
<RootIntermediateOutputPath>$(ArtifactsDir)$(Configuration)\obj\</RootIntermediateOutputPath> In the SDK: <PropertyGroup Condition="'$(RootOutputPath)' != ''" >
<BaseOutputPath Condition="'$(BaseOutputPath)' == ''">$(RootOutputPath)$(MSBuildProjectName)\</BaseOutputPath>
<OutputPath>$(BaseOutputPath)</OutputPath>
</PropertyGroup>
<PropertyGroup Condition="'$(RootIntermediateOutputPath)' != ''" >
<BaseIntermediateOutputPath Condition="'$(BaseIntermediateOutputPath)' == ''">$(RootIntermediateOutputPath)$(MSBuildProjectName)\</BaseIntermediateOutputPath>
<IntermediateOutputPath>$(BaseIntermediateOutputPath)</IntermediateOutputPath>
</PropertyGroup> |
I believe the above is consistent with the conclusions of our meeting. Note that the properties are set conditionally, so setting Base* paths on per-project basis overrides the default settings. This makes it compatible with LUT and other systems that would want to redirect output paths. Perhaps we could set RootOutputPath also conditionally if we want easy redirection on per-repo basis. |
I still think we need to start by stepping back and ...
Need to come to an agreement here before I can really comment on this proposal. |
Sorry I don't. Let's take some time, write it down, then we have a shared place to go back to for understanding. |
Here is a concrete example of what I don't understand: This PR was created directly as a result of our previous meeting. Didn't need any new variables here to get this running. Everything is based off of I don't know how we got from that PR and meeting to needing new variables. |
You can do this yourself using only BaseXxx, but it's tricky. Feature here is to make it easier. RootXxx would be a new feature that does the right thing on your behalf when you just specify the top-level dir. You would not need to ever say $(MSBuildProjectName) yourself in a csproj or targets file to get a sane build. |
+1 for what @nguerrera says. In addition the goal is for users to not need to touch any of the (Base)(Intermediate)OutputPath. OutDir properties. |
Roslyn currently defines the variables like so: <RepoRoot>$([System.IO.Path]::GetFullPath('$(MSBuildThisFileDirectory)..\..\'))</RepoRoot>
<BaseOutputPath Condition="'$(BaseOutputPath)' == ''">$(RepoRoot)Binaries\</BaseOutputPath>
<OutDir>$(BaseOutputPath)$(Configuration)\</OutDir>
<BaseIntermediateOutputPath Condition="'$(BaseIntermediateOutputPath)' == ''">$(RepoRoot)Binaries\Obj\</BaseIntermediateOutputPath>
<IntermediateOutputPath>$(BaseIntermediateOutputPath)$(Configuration)\$(MSBuildProjectName)\</IntermediateOutputPath> It's slightly different from the proposal above which includes the project name in the Base* paths as well. It turns out this is needed to be compatible with dotnet SDK. Once we move Roslyn to the SDK we will need to include the project name not just in (Intermediate)OutputPath but also in the Base variables. Doing so brings it on par with this proposal. |
I think this is what is getting to me. This issue is jumping to a conclusion without fully explaining the problem. It may make sense to the others on this who are more familiar with the SDK. For others though I just don't see the justification. I think it would help to step back and ...
|
@rainersigwald this relates to the PR you have in msbuild. Will believes this will break c++/CLI and so we may want to consider special casing there. |
…810.2 (dotnet#867) - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19410.2
Here's my take on this via MSBuild's side: Nirmal4G/MSBuild@a9563c0
Thus, freeing up
|
@marcpopMSFT is there any work to do after that MSBuild change was merged? |
We'll have to look into it, but I think that the SDK copied some of MSBuilds output path calculation logic, because it needed the paths to be calculated earlier in the evaluation or something. |
What's the status of this issue, is it considered fully implemented by the previously mentioned MSBuild change? If so, what is the required configuration that I should use, is there any documentation I can refer to? |
@dsplaisted the original feature of BaseOutputPath is complete and you can set that. Daniel is investigating additional changes though I don't know if we have details on that to share. This is what our documentation has: |
@stijnherreman The latest update is that we are considering some output path changes in .NET 8, which would include support for a In the meantime, you can put the following in a <Project>
<PropertyGroup>
<RootOutputPath>artifacts</RootOutputPath>
<RootOutputPath>$([System.IO.Path]::GetFullPath('$(MSBuildThisFileDirectory)\$(RootOutputPath)\'))</RootOutputPath>
<BaseOutputPath>$(RootOutputPath)bin\$(MSBuildProjectName)\</BaseOutputPath>
<BaseIntermediateOutputPath>$(RootOutputPath)obj\$(MSBuildProjectName)\</BaseIntermediateOutputPath>
</PropertyGroup>
</Project> |
Thank you for this solution. I've been juggling a mish-mash of options in If I may add a slight modification for those who want to use a full path for the output directory:
Update: The downside is that creating an APPX package will break with this error:
|
Use [SDK guidance][1] for BaseOutputPath, BaseIntermediateOutputPath to establish read-only source tree. [1]: dotnet/sdk#867 (comment)
Use [SDK guidance][1] for BaseOutputPath, BaseIntermediateOutputPath to establish read-only source tree. [1]: dotnet/sdk#867 (comment)
I'm glad some progress has been made on this, but what I really need is to be able to specify the full path to the artifacts' location. The
|
@skst that looks like an issue specific to the Xamarin iOS targets, would you mind opening an issue in https://github.com/xamarin/xamarin-macios ? |
ArtifactsPath now covers the original ask of this issue. If there are additional asks or feedback, please file new issues on the feature. |
Currently, new projects using SDK build into per-project bin and obj directories. That's good for simple projects that don't participate in CI. For repos that build using CI servers it is often a requirement to produce all outputs under a single root directory with following (or similar) layout:
We have seen countless repos with build systems that are customized to do so, each in a different way. Such build customization is usually hard to get right.
I propose to add an out of the box option to the SDK that allows customers to create such repo layouts trivially.
The
$(RootOutputPath)
could perhaps be specified via implicitDirectory.Build.props
import feature: dotnet/msbuild#222.Perhaps we could also consider
$(RepositoryRootPath)
a well-known property that has a documented meaning. It's a generally useful property to have, imo.To summarize I propose the user has the option to set the following properties in
Directory.Build.props
:And the SDK uses these variables to set the output paths like so:
The text was updated successfully, but these errors were encountered: