Skip to content
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 error when specifying --output option for a solution #29065

Merged

Conversation

dsplaisted
Copy link
Member

Fixes #15607

@dsplaisted dsplaisted requested a review from a team as a code owner November 14, 2022 21:27
@dsplaisted dsplaisted requested review from a team and removed request for a team November 14, 2022 21:27
@@ -18,6 +18,25 @@ public static class OptionForwardingExtensions

public static ForwardedOption<T> ForwardAsSingle<T>(this ForwardedOption<T> option, Func<T, string> format) => option.SetForwardingFunction(format);

public static ForwardedOption<string> ForwardAsOutputPath(this ForwardedOption<string> option, string outputPropertyName, bool surroundWithDoubleQuotes = false)
Copy link
Member

Choose a reason for hiding this comment

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

VS appears to create a child folder for each project so this doesn't happen: https://learn.microsoft.com/en-us/visualstudio/ide/how-to-change-the-build-output-directory?view=vs-2022#build-to-a-common-output-directory

If Visual Studio invokes the sdk with -o, It seems very possible that this breaks. We should verify that this doesn't break following the above document

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you mean. Visual Studio doesn't pass the -o option. This PR is about what happens if you build a solution from the command line and specify that option.

Copy link
Member

@nagilson nagilson Nov 18, 2022

Choose a reason for hiding this comment

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

Sorry, I was thinking of some hypothetical implementation of VS that invoked an SDK CLI process with -o ./project-name/, in which case this would break that. But that doesn't happen.

If appending projectName to the -o values would be all it takes to implement the feature from the SDK side though, that would be pretty awesome

src/Cli/dotnet/OptionForwardingExtensions.cs Show resolved Hide resolved
src/Tasks/Common/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Tasks/Common/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Tasks/Common/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Tasks/Common/Resources/Strings.resx Outdated Show resolved Hide resolved
src/Tests/dotnet.Tests/OutputPathOptionTests.cs Outdated Show resolved Hide resolved
@dsplaisted dsplaisted merged commit cd16c47 into dotnet:release/7.0.2xx Nov 18, 2022
radical added a commit to radical/performance that referenced this pull request Dec 7, 2022
.. with .sln files. Instead, pass `/p:PublishDir=..` to the build
command.

This changed in dotnet/sdk#29065, and broke
`dotnet-runtime-perf` pipeline's blazor scenarios.

```
$ dotnet publish /home/helixbot/work/A8850905/p/performance/src/scenarios/blazorpizza/app/BlazingPizza.sln --configuration Release --output pub /p:NuGetPackageRoot=/home/helixbot/work/A8850905/w/A9A608EA/u/artifacts/packages/ /p:UseSharedCompilation=false /p:BuildInParallel=false /m:1 --framework net7.0 /p:_TrimmerDumpDependencies=true -bl:./traces/blazor_publish.binlog -p:WasmNativeWorkload=false
MSBuild version 17.5.0-preview-22601-03+a2490dd3f for .NET

/home/helixbot/work/A8850905/p/dotnet/sdk/8.0.100-alpha.1.22606.3/Current/SolutionFile/ImportAfter/Microsoft.NET.Sdk.Solution.targets(36,5):
    error NETSDK1194: The "--output" option isn't supported when building a solution. [/home/helixbot/work/A8850905/p/performance/src/scenarios/blazorpizza/app/BlazingPizza.sln]
```
radical added a commit to radical/performance that referenced this pull request Dec 7, 2022
.. with .sln files. Instead, pass `/p:PublishDir=..` to the build
command.

This changed in dotnet/sdk#29065, and broke
`dotnet-runtime-perf` pipeline's blazor scenarios.

```
$ dotnet publish /home/helixbot/work/A8850905/p/performance/src/scenarios/blazorpizza/app/BlazingPizza.sln --configuration Release --output pub /p:NuGetPackageRoot=/home/helixbot/work/A8850905/w/A9A608EA/u/artifacts/packages/ /p:UseSharedCompilation=false /p:BuildInParallel=false /m:1 --framework net7.0 /p:_TrimmerDumpDependencies=true -bl:./traces/blazor_publish.binlog -p:WasmNativeWorkload=false
MSBuild version 17.5.0-preview-22601-03+a2490dd3f for .NET

/home/helixbot/work/A8850905/p/dotnet/sdk/8.0.100-alpha.1.22606.3/Current/SolutionFile/ImportAfter/Microsoft.NET.Sdk.Solution.targets(36,5):
    error NETSDK1194: The "--output" option isn't supported when building a solution. [/home/helixbot/work/A8850905/p/performance/src/scenarios/blazorpizza/app/BlazingPizza.sln]
```
radical added a commit to radical/performance that referenced this pull request Dec 7, 2022
.. with .sln files. Instead, pass `/p:PublishDir=..` to the build
command.

This changed in dotnet/sdk#29065, and broke
`dotnet-runtime-perf` pipeline's blazor scenarios.

```
$ dotnet publish /home/helixbot/work/A8850905/p/performance/src/scenarios/blazorpizza/app/BlazingPizza.sln --configuration Release --output pub /p:NuGetPackageRoot=/home/helixbot/work/A8850905/w/A9A608EA/u/artifacts/packages/ /p:UseSharedCompilation=false /p:BuildInParallel=false /m:1 --framework net7.0 /p:_TrimmerDumpDependencies=true -bl:./traces/blazor_publish.binlog -p:WasmNativeWorkload=false
MSBuild version 17.5.0-preview-22601-03+a2490dd3f for .NET

/home/helixbot/work/A8850905/p/dotnet/sdk/8.0.100-alpha.1.22606.3/Current/SolutionFile/ImportAfter/Microsoft.NET.Sdk.Solution.targets(36,5):
    error NETSDK1194: The "--output" option isn't supported when building a solution. [/home/helixbot/work/A8850905/p/performance/src/scenarios/blazorpizza/app/BlazingPizza.sln]
```

Details:

When building a solution, passing a relative path to `PublishDir` gets
evaluated per-project. So, if we pass `-p:PublishDir=pub` then we get
`pub/` sub-directories for each of the projects.

But when used with `--output pub`, output for all the projects goes to
the same directory. To have the same behavior use an absolute path.
radical added a commit to dotnet/performance that referenced this pull request Dec 7, 2022
…sh` (#2774)

.. with .sln files. Instead, pass `/p:PublishDir=..` to the build
command.

This changed in dotnet/sdk#29065, and broke
`dotnet-runtime-perf` pipeline's blazor scenarios.

```
$ dotnet publish /home/helixbot/work/A8850905/p/performance/src/scenarios/blazorpizza/app/BlazingPizza.sln --configuration Release --output pub /p:NuGetPackageRoot=/home/helixbot/work/A8850905/w/A9A608EA/u/artifacts/packages/ /p:UseSharedCompilation=false /p:BuildInParallel=false /m:1 --framework net7.0 /p:_TrimmerDumpDependencies=true -bl:./traces/blazor_publish.binlog -p:WasmNativeWorkload=false
MSBuild version 17.5.0-preview-22601-03+a2490dd3f for .NET

/home/helixbot/work/A8850905/p/dotnet/sdk/8.0.100-alpha.1.22606.3/Current/SolutionFile/ImportAfter/Microsoft.NET.Sdk.Solution.targets(36,5):
    error NETSDK1194: The "--output" option isn't supported when building a solution. [/home/helixbot/work/A8850905/p/performance/src/scenarios/blazorpizza/app/BlazingPizza.sln]
```

Details:

When building a solution, passing a relative path to `PublishDir` gets
evaluated per-project. So, if we pass `-p:PublishDir=pub` then we get
`pub/` sub-directories for each of the projects.

But when used with `--output pub`, output for all the projects goes to
the same directory. To have the same behavior use an absolute path.
@baronfel baronfel added the breaking-change Using this label will notify dotnet/compat and trigger a request to file a compat bug label Jan 20, 2023
@aaronasmith
Copy link

This seems to have broken dotnet pack for solution files when specifying an output. Even when called with nobuild.

@ctyar
Copy link

ctyar commented Jan 28, 2023

Yeah, you have to specify a project:
dotnet pack path\to\project.csproj -o artifacts

@kekyo
Copy link

kekyo commented Feb 15, 2023

Too many my projects are broken when building nuget packaging with dotnet pack -o <path> usage...

@dave-yotta
Copy link

dave-yotta commented Feb 15, 2023

This has broken our build, thanks. @ctyar we're packing the whole solution not a single nupkg. Not sure why this has happened overnight but probabbly dotnet-install.sh has just started pointing to it or the dotnet buster images have just updated to .200?

This is a breaking change it's not ok to release these on a patch version.

@GregorLamche
Copy link

Can confirmed --output works with 102 (pulled from repos locally), and is now broken with 200 (pulled by our Azure pipeline). (eventual consistency, ho!)

@dave-yotta
Copy link

That didn't even work, specifying the folder(s) I just get #20 3.922 /usr/share/dotnet/sdk/7.0.200/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets(221,5): error NU5026: The file '...' to be packed was not found on disk.

@baronfel
Copy link
Member

This has broken our build, thanks. @ctyar we're packing the whole solution not a single nupkg. Not sure why this has happened overnight but probabbly dotnet-install.sh has just started pointing to it or the dotnet buster images have just updated to .200?

This is a breaking change it's not ok to release these on a patch version.

@dave-yotta for the install-scriptsnspecifically you can limit them to the 100 series if that's a requirement for you.

For clarity, the 200 series is not a patch release from the 100-series of SDKs, it is a 'feature band' release where we do often introduce new features. The .NET SDK, and .Net overall, do not use SemVer for versions. While we do strive to make feature band' updates as painless as possible, we decided this work was worth doing, as we had seen many users end up with inconsistent builds due to the unclear semantics of publish/pack at the solution level.

Now having said that, we should have had a breaking change notice up on the docs and missed that - that's 100% our fault and we'll get that updated today.

@dave-yotta
Copy link

dave-yotta commented Feb 15, 2023

I'm using net 7.0, I need the sdk to match that, without pulling any breaking changes. I would not have spotted anything in the docs because I'm referencing 7.0.* for both runtime and sdk.

I have no way to "limit" to the 100 series, versioning tools do not support this - nor do your own install scripts.

The only possible solution is to pick for example 7.0.1 for the runtime and (checks the reference table) 7.0.102 (or something) for the sdk.

While I might be able to configure our dependency monitoring tooling to pick up docker FROM directives, there is no chance it'll pick up ./dotnet-install.sh --channel 7.0.102 --install-dir /usr/share/dotnet inside a dockerfile or other script. I'm not even sure install scripts support this type of pinning; I've asked them for what it's worth.

That leaves me in a situation where I will be lacking critical updates from at least the sdk install, which is likely worse than being left open to breaking changes all the time.

see #30624 (comment)

@dmelinosky
Copy link

dmelinosky commented Feb 15, 2023

Can confirm this is breaking one of our builds as well.

@bbrandt-plx
Copy link

This is also breaking our build all of the sudden within Github Actions. We had a successful build a few days ago under MSBuild 17.4.1+9a89d02ff but now fails under MSBuild 17.5.0-preview-23061-01+040e2a90e

We have a solution with multiple projects that all produce nuget packages.

Here's the basic format of the dotnet Pack command we're running:

dotnet pack /pathTo/ourSolution.sln --configuration Release --no-build --no-restore --output /output/artifacts/nuget --no-dependencies

The build fails with: "error NETSDK1194: The "--output" option isn't supported when building a solution."

Any recommendations on how we can change our build commands to build a solution and publish all the nuget packages to one place?

@baronfel
Copy link
Member

For pack specifically you should be able to do a combination of dotnet pack <sln> <other options except --output> followed by dotnet nuget push **/bin/**/*.nupkg - the push command handles globbing natively. That said, this does feel like a change that had too much unintended impact and the team is discussing mitigations at the moment.

@ascott18
Copy link

ascott18 commented Feb 16, 2023

To keep old behavior for dotnet pack if you actually need all your packages to be outputted into one place (e.g. to publish as a build artifact rather than feeding straight to nuget push:

-o out/packages becomes -p:PackageOutputPath="$(pwd)/out/packages"

@xt0rted
Copy link
Contributor

xt0rted commented Feb 16, 2023

This is a pretty disappointing change. We've been publishing all of our projects in one go using dotnet publish ./project.sln --no-build --output ./artifacts and that would publish each website and console app into its own artifacts folder which was passed to DevOps for deploying. This would take 2 seconds to run and required no extra effort when adding new projects that needed publishing. Now we have to publish each project individually which bumps the time to 20+ seconds. If we take --no-build off it takes minutes to build and publish artifacts.

Update: When using dotnet publish you can switch -o/--output to -p:PublishDir=. They don't function the same though with how they handle your current working directory. So dotnet publish --output ./artifacts becomes dotnet publish -p:PublishDir=../../artifacts.

@jozefizso
Copy link

jozefizso commented Mar 4, 2023

This change randomly broke our build system. Apparently the default SDK for GHA was .NET 6 where this option was working correctly and it is broken in .NET 7 which is default now.

Why we cannot use the --output option when there is directly replacement with /p:PublishDir?

Update: As as read more about it, it's not even .NET 6 vs 7, it is 7.0.1 vs 7.0.200 - you cannot just break build system for a minor update.

@jozefizso
Copy link

For clarity, the 200 series is not a patch release from the 100-series of SDKs, it is a 'feature band' release where we do often introduce new features.

This is a breaking change and not a feature.

@jozefizso
Copy link

For pack specifically you should be able to do a combination of dotnet pack <sln> <other options except --output> followed by dotnet nuget push **/bin/**/*.nupkg - the push command handles globbing natively. That said, this does feel like a change that had too much unintended impact and the team is discussing mitigations at the moment.

This is really bad workaround. Pushing random nupkgs from all over the place to nuget?

@baronfel
Copy link
Member

baronfel commented Mar 4, 2023

Hi @jozefizso - it should be possible to update to 7.0.201, where we've pushed out a hotfix that addresses this regression. Especially removing it from pack, where it never should have been applied. There's a more in-depth example of the reasoning for the new diagnostic on the change notification if you'd like some more background on why setting properties like PublishDir at the solution level isn't something we recommend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CLI breaking-change Using this label will notify dotnet/compat and trigger a request to file a compat bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.