Skip to content

Downgrade errors for specifying --output for solutions to warnings#30660

Merged
dsplaisted merged 4 commits intodotnet:release/7.0.2xxfrom
dsplaisted:fix-solution-pack
Feb 16, 2023
Merged

Downgrade errors for specifying --output for solutions to warnings#30660
dsplaisted merged 4 commits intodotnet:release/7.0.2xxfrom
dsplaisted:fix-solution-pack

Conversation

@dsplaisted
Copy link
Copy Markdown
Member

Downgrade errors for specifying --output for solutions to warnings

Description

In 7.0.200, we added an error when the --output option was specified for a solution. This was because this causes multiple projects to use the same output path, which can lead to hard-to-diagnose issues. It can also result in build flakiness if there are different projects copying the same files to the output path.

However, it appears that this change broke a lot more people than expected, so we will downgrade this error to a warning.

Additionally, we are going to remove the error/warning entirely from the dotnet pack command. The --output parameter for dotnet pack only affects the package output path, and since each project should be generating a different .nupkg file, there shouldn't be a risk of file collisions when multiple projects share the same package output path.

Customer impact

We have multiple reports of this breaking CI builds.

Regression?

  • Yes
  • No

Risk

Low

Verification

  • Manual (required)
  • Automated

@ghost
Copy link
Copy Markdown

ghost commented Feb 15, 2023

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.

@dsplaisted dsplaisted requested review from a team, baronfel and marcpopMSFT February 15, 2023 23:45
</data>
<data name="CannotHaveSolutionLevelOutputPath" xml:space="preserve">
<value>NETSDK1194: The "--output" option isn't supported when building a solution.</value>
<value>NETSDK1194: The "--output" option isn't supported when building a solution. Specifying a solution-level output path results in all projects copying outputs to the same directory, which can lead to inconsistent builds.</value>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming it's ok to ship a servicing fix with error message clarifications without accompanying localization? Do we just merge those when they come in later?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe so.

Comment thread src/Cli/dotnet/commands/dotnet-pack/PackCommandParser.cs
@dsplaisted dsplaisted merged commit 9d1eea6 into dotnet:release/7.0.2xx Feb 16, 2023
@xt0rted
Copy link
Copy Markdown
Contributor

xt0rted commented Feb 17, 2023

Nice to see this reverted and by the looks of it my scenario will be supported again, but will print out a warning.

For context I was using dotnet publish App.sln --no-build --output ./artifacts so I could publish all of our projects at once which runs a lot faster than doing each project individually. In a quick test earlier dotnet publish App.sln --no-build ran in 2 seconds, while running dotnet publish /src/*.csproj --no-build for each project took 20 seconds.

The way I've been publishing our projects is as follows:

Directory.Build.props

<Project>

  <PropertyGroup>
    <IsPublishable>False</IsPublishable>
  </PropertyGroup>

</Project>

Directory.Build.targets

<Project>

  <Target Name="_PrepareForPublish" BeforeTargets="PrepareForPublish">
    <!--
      Adjust publish output so it appends the PublishTo to the PublishDir.

      This makes running `dotnet publish -o some/folder` easier since each project
      will be published to its own named folder inside of the PublishDir location.

      The default behavior of publishing to the project's bin folder should be
      preserved if a custom output folder isn't provided.
      -->
    <PropertyGroup Condition="!$(PublishDir.StartsWith('bin'))">
      <!-- Ensure any PublishDir has a trailing slash, so it can be concatenated -->
      <PublishDir Condition="!HasTrailingSlash('$(PublishDir)')">$(PublishDir)\</PublishDir>

      <!--
        Default to the project name which can be overridden inside each project file.

        We're using a custom property instead of the built-in PublishDirName because that
        affects the output location when running `dotnet publish` without an output folder.
        -->
      <PublishTo Condition="'$(PublishTo)' == ''">$(MSBuildProjectName)</PublishTo>

      <PublishDir>$(PublishDir)$(PublishTo)</PublishDir>
    </PropertyGroup>
  </Target>

</Project>

Then inside each .csproj that I want to publish artifacts for (5 websites, 3 worker services, and 4 console apps) I put the following properties:

<Project Sdk="Microsoft.NET.Sdk.Web">

  <PropertyGroup>
    <IsPublishable>True</IsPublishable>
    <PublishTo>account-web</PublishTo>
  </PropertyGroup>

</Project>

In our ci workflow we'd then call dotnet build, dotnet test --no-build, and finally dotnet publish --no-build --output ./artifacts. Each published project would be published into the artifacts folder, inside of it's own folder. So the structure would be /artifacts/account-web, /artifacts/public-api, etc.

For now we can work around it with dotnet publish --no-build -p:PublishDir=../../artifacts but it'd be nice if dotnet publish --output ... continued to work with solutions as long as it was also using --no-build.

@baronfel
Copy link
Copy Markdown
Member

@xt0rted nice work - It appears to me that you might be a prime candidate for the root output path property we're discussing introducing as part of dotnet/designs#281 - would you mind reviewing that when you have time and seeing if it would allow you to remove this build customization? one of our goals is to remove the need for folks to do complex build manipulation to get reasonable layouts, and reduce the need to use --output overall.

@TheConstructor
Copy link
Copy Markdown

#30624 (comment) describes a work-around until the new SDK is released

denis417 pushed a commit to JetBrains/sdk that referenced this pull request Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants