Unblock having non-anycpu configurations by always setting output path #351
Conversation
Also, set PlatformTarget to match Platform for x86 and x64 as VS would do for classic projects. Finally, fix nuget pack output path for single-targeting project.
ScenarioAdd configurations or platforms to project in VS and have it build without having to make manual modifications to project. Also, get a consistent package output path regardless of whether cross-targeting is used or not Bugs#328 WorkaroundsSet OutputPath and PackageOutputPath manually in csproj RiskLow Performance ImpactNone Regression AnalysisNot a regression |
|
@srivatsn @dsplaisted @dotnet/project-system Please review |
| <IntermediateOutputPath Condition="!HasTrailingSlash('$(IntermediateOutputPath)')">$(IntermediateOutputPath)\</IntermediateOutputPath> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- Set the package output path (for nuget pack target) now, before the TargetFramework is appended --> |
There was a problem hiding this comment.
I don't think there's a way to solve this right now, but it seems to me that you wouldn't want the NuGet package generated in an x86 or x64 folder.
There was a problem hiding this comment.
I filed #329 to make sure we have settled on default output paths. There is definitely tension between traditional VS concept of
| --> | ||
| <PropertyGroup> | ||
| <Configuration Condition="'$(Configuration)'==''">Debug</Configuration> | ||
| <Platform Condition="'$(Platform)'==''">AnyCPU</Platform> |
There was a problem hiding this comment.
You also have the Configuration and Platform defaults in Microsoft.NET.Sdk.props, is there a reason to also put it in this targets file?
There was a problem hiding this comment.
Just being defensive in case the props aren't imported, though you probably have other problems. Regardless, the duplication pre-exists this change, so I'll leave de-duping as a separate exercise.
| @@ -60,6 +80,7 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
|
|
|||
| <!-- Temp Hack: https://github.com/Microsoft/msbuild/issues/1045: This is the only before common targets hook available to us, but using it is hijacking a hook that should belong to the user. --> | |||
There was a problem hiding this comment.
Hack -> workaround (or did you already change this in another PR?)
There was a problem hiding this comment.
Yes, this is already fixed in another PR.
| .Should() | ||
| .Pass(); | ||
|
|
||
| buildCommand.GetOutputDirectory("netcoreapp1.0", Path.Combine("x64", "Debug")) |
There was a problem hiding this comment.
We should probably rename this parameter or otherwise refactor so that we're not passing in x64\Debug as the configuration parameter.
There was a problem hiding this comment.
I'll follow up with a separate test-only change to clean this up.
|
|
||
| namespace Microsoft.NET.Pack.Tests | ||
| { | ||
| public class GivenThatWeWantToPackASimpleLibrary |
There was a problem hiding this comment.
Is this test related to the OutputPath changes or is it just filling in more general test coverage?
There was a problem hiding this comment.
Yes, it is related to the package output path being correct. There are different import paths for x-targeted and single-targeted cases and we only tested x-targeted. My first attempt at fixing this broke the x-targeted test and then I found it prudent to test both cases to prevent fixing one while breaking the other.
|
@srivatsn Please review as well. @MattGertz for approval |
|
Approved. |
* Fixed error messages * Removed some unnecessary period characters and one unused string resource. * Improved MissingLinkToRegistry string resource
Also, set PlatformTarget to match Platform for x86 and x64 as VS would
do for classic projects.
This is enough to close all of the following:
But there is remaining work to manage (add/remove/rename) configurations and platforms, which is now tracked by: