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

Breaking change inside v7.0.200 - MSBuild Preview version used !? #30624

Closed
WernerMairl opened this issue Feb 15, 2023 · 83 comments
Closed

Breaking change inside v7.0.200 - MSBuild Preview version used !? #30624

WernerMairl opened this issue Feb 15, 2023 · 83 comments

Comments

@WernerMairl
Copy link

Describe the bug

"dotnet build -o " feature (output folder for entire solution build)

image

To Reproduce

use "dotnet build -o " with sdk 7.0.200 from today

Exceptions (if any)

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

Further technical details

You can see in the screenshot that a preview version of MSBuild is used/delivered (7.5.0-preview....".
Maybe this is the root cause of the issue!
Because with the SDK from last week (7.0.100) i can workaround the exception!

I hope that helps - assuming this issue may cause a lot of problems...

BR
Werner

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-NetSDK untriaged Request triage from a team member labels Feb 15, 2023
@KalleOlaviNiemitalo
Copy link
Contributor

The NETSDK1194 error was intentionally added in #29065, to fix #15607.

@WernerMairl WernerMairl changed the title Breaking change inside v7.0.200 ? Breaking change inside v7.0.200 - MSBuild Preview version used !? Feb 15, 2023
@WernerMairl
Copy link
Author

@KalleOlaviNiemitalo
Nice fix. but following semantic versioning policy, this may be the wrong delivery :-(

@PeterLehmann
Copy link

still a breaking change you have not write in the release note

@KalleOlaviNiemitalo
Copy link
Contributor

I'm not sure whether there even are any release notes for differences between .NET SDK feature bands.

If this breaking change were listed in https://github.com/dotnet/core/blob/main/release-notes/7.0/7.0.3/7.0.3.md#notable-changes, developers might notice it from there. But you can also target .NET 7.0.3 using .NET SDK 7.0.103, which does not include this breaking change.

@WernerMairl
Copy link
Author

WernerMairl commented Feb 15, 2023

Sorry Kalle,

dotnet 7.0 is a released product.

Yes: you CAN implement breaking changes, but then they must be:

a) documented (release notes)
b) more important: the major version number part must be increased (assuming we are doing semver.

We can bet, that this change may be reverted ASAP a soon as Microsoft recognizes the mistake ;-)

This fix can be a nice improvement on .Net 8

@KalleOlaviNiemitalo
Copy link
Contributor

.NET SDK is not doing semver -- see https://learn.microsoft.com/en-us/dotnet/core/versions/#versioning-details.

@dave-yotta
Copy link

dave-yotta commented Feb 15, 2023

No Semver? That's crazy! Do I have to pin to a patch version and avoid security patches just so that I can avoid breaking the build?

All the heat the sdk team are going to get for this, and it's only been out for a few hours, is a direct consequence of all consumers of the SDK (for me it is my reference to 7.0 of the dotnet docker image: mcr.microsoft.com/dotnet/sdk:7.0) expecting the patch version to not indicate any breaking change.

SemVer aside: which versioning system actually specifies a breaking change as the most minor possible change?

@WernerMairl
Copy link
Author

all in all...
not a good idea to deliver this in this way....

we will see how many build pipelines worldwide are broken...

🍿 🍿 🍿

@dave-yotta
Copy link

dave-yotta commented Feb 15, 2023

So if my runtime is dotnet/aspnet:7.0 I need to somehow mirror that in an sdk version dotnet/sdk:7.0.1**?? There are no valid version strings for this supported in any tools I can think of!

@GregorLamche
Copy link

So if my runtime is dotnet/aspnet:7.0 I need to somehow mirror that in an sdk version dotnet/sdk:7.0.1**?? There are no valid version strings for this supported in any tools I can think of!

One reason why my team stopped using Microsoft docker images (MS is too slow to role out security updates of the base image).

As a work around you can switch to a OS base image, and install the sdk/runtime using its package manager.
With Ubuntu it would look like this:

FROM ubuntu:22.04 as base
RUN apt-get update \
    && apt-get install -y dotnet-sdk-7.0=7.0.102-1

@dave-yotta
Copy link

As a work around you can switch to a OS base image, and install the sdk/runtime using its package manager. With Ubuntu it would look like this:

I was thinking about sdk security patches, not OS, but that's a new sadness. Pinning dotnet-sdk-7.0=7.0.102-1 means I won't get any critical patches from 7.0; there's no way around this if breaking changes are released on the patch version - I guess this is because the sdk team want maj/min to match the .NET version.

If the sdk and runtime are truly a monorepo, then breaking changes should update the minor version for both of them.

@KalleOlaviNiemitalo
Copy link
Contributor

Ubuntu's own dotnet-sdk-7.0 package is still at version 7.0.102-0ubuntu1~22.10.1, doesn't have 7.0.103 yet. I think it's using source-build so it will remain in 7.0.1xx and never upgrade to any higher feature band. .NET SDK 7.0.200 is not part of the source-build release for .NET 7.0.3 (dotnet/source-build#3247).

@dave-yotta
Copy link

dave-yotta commented Feb 15, 2023

No, it seems it is the install-scripts. I couldn't repro directly on sdk:7.0.

Here's what leads to the error for me:

FROM mcr.microsoft.com/dotnet/aspnet:7.0-bullseye-slim as base
RUN apt-get update 
RUN apt install -y jq moreutils curl
RUN curl -sSL https://dot.net/v1/dotnet-install.sh > dotnet-install.sh
RUN chmod u+x dotnet-install.sh
RUN ./dotnet-install.sh --channel 7.0 --install-dir /usr/share/dotnet

FROM base
RUN dotnet pack -c release -o packages-out
 ./dotnet-install.sh --channel 7.0 --install-dir /usr/share/dotnet
dotnet-install: Note that the intended use of this script is for Continuous Integration (CI) scenarios, where:
dotnet-install: - The SDK needs to be installed without user interaction and without admin rights.
dotnet-install: - The SDK installation doesn't need to persist across multiple CI runs.
dotnet-install: To set up a development environment or to run apps, use installers rather than this script. Visit https://dotnet.microsoft.com/download to get the installer.

dotnet-install: Attempting to download using aka.ms link https://dotnetcli.azureedge.net/dotnet/Sdk/7.0.200/dotnet-sdk-7.0.200-linux-x64.tar.gz

@WernerMairl
Copy link
Author

In the meantime I'm working on some "forward" fix for my projects.

UseCase

build + pack + push (Github-Action)

Old version:

Option "-o" used to locate the resulting *.nupkg files for push:

image

New version

for SDK 7.0.200:

  • removed all the "-o" usages
  • modified dotnet nuget push for recursive folder search:

image

@MichaelSimons
Copy link
Member

So if my runtime is dotnet/aspnet:7.0 I need to somehow mirror that in an sdk version dotnet/sdk:7.0.1**?? There are no valid version strings for this supported in any tools I can think of!

One reason why my team stopped using Microsoft docker images (MS is too slow to role out security updates of the base image).

As a work around you can switch to a OS base image, and install the sdk/runtime using its package manager. With Ubuntu it would look like this:

FROM ubuntu:22.04 as base
RUN apt-get update \
    && apt-get install -y dotnet-sdk-7.0=7.0.102-1

I don't want to derail this topic here, @GregorLamche, would you care to start a discussion topic on your experience? I'd like to know details and try to improve our .NET Docker offering if we are not meeting your needs. TIA.

@baronfel baronfel added the needs team triage Requires a full team discussion label Feb 15, 2023
@amis92
Copy link

amis92 commented Feb 15, 2023

It's very disturbing to see -preview label when running a simple dotnet build using the this SDK.

$ dotnet build --no-restore
MSBuild version 17.5.0-preview-23061-01+040e2a90e for .NET

And, I was also hit by the breaking change, quite involutarily, as my global.json says 7.0.100 but actions/setup-dotnet installed the new one anyway.

@baronfel
Copy link
Member

baronfel commented Feb 15, 2023

We've been discussing the introduction of the new error and will be taking the following actions based on everyone's feedback in this (and other!) threads - THANK YOU for making your feedback known, please keep it up in the future.

a) We'll downgrade the error to a warning - we think this usage pattern is harmful and can lead to builds that are harder to debug, especially when projects are multi-targeted or have differing sets of PackageReference versions.
b) We'll skip the logic for the pack command entirely - pack has stable and correct semantics when the -o flag is specified (because it sets PackageOutputPath instead of OutputPath directly)

We will put this up for inclusion into next month's servicing release of the .NET SDK, which is the soonest possible ship vehicle we have for this kind of change. In the meantime, you can restore the previous behavior by changing the use of -o to a specific property. If you want to maintain the existing behavior exactly, then you can use the --property flag to set a MSBuild property to the desired directory. The property to use varies based on the command:

Command Property Example
build OutputPath dotnet build --property:OutputPath=DESIRED_PATH
clean OutputPath dotnet clean --property:OutputPath=DESIRED_PATH
pack PackageOutputPath dotnet pack --property:PackageOutputPath=DESIRED_PATH
publish PublishDir dotnet publish --property:PublishDir=DESIRED_PATH
store OutputPath dotnet store --property:OutputPath=DESIRED_PATH
test TestResultsDirectory dotnet test --property:OutputPath=DESIRED_PATH

Note
DESIRED_PATH must be a fully-qualified path for best results.

@ernestopye
Copy link

We just hit this today as well. It appears to be an issue with 6.0.406 too (using MSBuild 17.5.0-preview-23061-01+040e2a90e). We were targeting 6.0.x and went to this version after it was released automatically.

We've now pinned our version to 6.0.405 and we're back to using a non-prerelease version of MSBuild on our build agents.

We have dozens of pipelines that will be affected by this and will need to update all of them.

We had no errors at all. It was just that ViewComponents were simply not compiling, and that's how it was brought to our attention.

@baronfel
Copy link
Member

Hello folks, wanted to give you all an update on the situation.

Last night, 7.0.201 was released through all the various channels. It should already be available on GitHub Actions and AzDO runners, and of course for local installation. This hotfix contained the changes to the --output parameter mentioned above: it is downgraded to a warning, and the check no longer applies to the pack command.

The other problem logged in this issue, MSBuild logging a prerelease version string, will be handled in the normal servicing schedule - likely with next month's expected release of 7.0.202.

Thank you all for the feedback and discussion on this issue.

@Astrophizz
Copy link

Astrophizz commented Mar 1, 2023

@baronfel Just a heads up, 7.0.200 was pushed to our Microsoft Hosted Azure Dev Ops build agents yesterday and broke dotnet restore --locked-mode. The restored packages no longer adhere to our packages.lock.json. We made need to pin to an SDK version to preserve the integrity of our build artifacts, even with a successful build 🫤

@baronfel
Copy link
Member

baronfel commented Mar 1, 2023

@Astrophizz do you have any details on that? maybe a binlog? paging @nkolev92 in case he's seen this reported elsewhere already.

@nkolev92
Copy link
Contributor

nkolev92 commented Mar 1, 2023

@Astrophizz Are you using CPM?

We haven't heard anything about lock file discrepancies yet. It'd be great if you can get us a repro and file an issue at https://github.com/nuget/home/issues

@Astrophizz
Copy link

@baronfel It's difficult to provide more info right now because my employer doesn't allow me to sign in to GitHub (posting from phone!) but I'll see if I can get a minimal reproduction. The issue was that Microsoft.Identity.Web 1.8.2 would resolve a descendant Microsoft.IdentityModel.Logging 6.10.0 with 7.0.103 but with 7.0.200 it resolved to 6.25.0 (in spite of the package lock explicitly spicing 6.10.0). Version 6.10.0 of that package has no dependencies but 6.25.0 depends on Microsoft.IdenityModel.Abstractions which led to a runtime file io exception when it couldn't find the dll of the new dependency.

@Astrophizz
Copy link

@nkolev92 What is CPM? I'll follow up with any reproduction at the NuGet GitHub.

@nkolev92
Copy link
Contributor

nkolev92 commented Mar 1, 2023

CPM is central package management, https://learn.microsoft.com/en-us/nuget/consume-packages/central-package-management.
There's active development there, so I was thinking it's more likely that you're hitting problems there thna without it.

Looking forward to your repro!

@Astrophizz
Copy link

@nkolev92 No, we aren't using that. It sounds cool though! It occurred to me that we are using Azure pipeline caching on the NuGet package cache (keyed to a hash of the lock file) and I suppose that could introduce an extra wrinkle, though I'm not sure how or why it would fail when no changes were made to package refs between the good and bad build... 🤔

@nkolev92
Copy link
Contributor

nkolev92 commented Mar 1, 2023

The error message, NU1004 should hopefully contain some details about what changed.

@Astrophizz
Copy link

Astrophizz commented Mar 1, 2023

@nkolev92 It doesn't have a NuGet error. It completes the package restore but with different packages than those specified by the lock file.

@nkolev92
Copy link
Contributor

nkolev92 commented Mar 1, 2023

Are you running with --locked-mode on the CI?

If you do that, it'll enforce the lock file. Without it, changes in references will be processed.

@Astrophizz
Copy link

Astrophizz commented Mar 1, 2023

@nkolev92 Yes, I'm running it with --locked-mode. It appears as though that no longer enforces the lock file with 7.0.200, or at least not completely.

@Astrophizz
Copy link

@nkolev92 After some further investigation, this looks like a matter of an Azure DevOps task restoring multiple projects ( **/*.csproj style with some being sibling "root" projects) and then another task building those projects. One of the root projects uses a package lock but that doesn't seem to matter much in the end as all of the built projects stomp on each other. I don't know why the result of the dll stomping changed with 7.0.200, but I suppose it could be a side effect of CPM work.

It sounds like CPM might be the solution to my problem at the same time since my reading of the docs is that I can get consistent transitive package versions across multiple solutions/root projects 😁👍. Does it support a package lock file? I'm guessing I would have a lock for each root project.

@nkolev92
Copy link
Contributor

nkolev92 commented Mar 2, 2023

One of the root projects uses a package lock but that doesn't seem to matter much in the end as all of the built projects stomp on each other. I don't know why the result of the dll stomping changed with 7.0.200, but I suppose it could be a side effect of CPM work

I wouldn't expect that to be the case. Most of the work in CPM is on transitive pinning, so stomping shouldn't be something that happens.

It sounds like CPM might be the solution to my problem at the same time since my reading of the docs is that I can get consistent transitive package versions across multiple solutions/root projects

Note that you'll only get that for transitive versions you decide to pin and if you enable transitive pinning.
Depending on how many or what kind of runnable/deployable projects you have, you may not need pinning though.

CPM is excellent for large repos, as you'll never accidentally direct reference different versions of packages.

Does it support a package lock file? I'm guessing I would have a lock for each root project

Yes, they're separate features that can work together.

@ericsampson
Copy link

@baronfel re this:

b) We'll skip the logic for the pack command entirely - pack has stable and correct semantics when the -o flag is specified (because it sets PackageOutputPath instead of OutputPath directly)

Can't we do that for the publish command as well? Since it also turns the specified -o into the PublishDir internally. Right now it still gives a scary warning for no reason AFAICT. Thanks!!!
(btw hi Chet!!)

@baronfel
Copy link
Member

@ericsampson the problem is how that shared PublishDir property is used - when set as a global property each project gets the same value for PublishDir and the outputs of all projects are copied to that same directory in a nondeterministic manner. This differs from PackageOutputPath because pack produces non-clobbering outputs to that shared path. The nondeterminism is the root of the warning here, because too many things are using that one property directly instead of namespacing or deriving a value from it.

I'd love to change the meaning of -o for publish to something explicitly not PublishDir, and then let PublishDir be computed based off of that root, but we need to do a bunch of user development before making any breaking changes here. I'm hopeful that approaches like the proposed new output paths will provide the kind of clean break this scenario needs, while keeping the mechanism opt-in.

@ericsampson
Copy link

thanks @baronfel.
one thing re that linked proposal, from a user perspective please don't make it enabled by default in .NET 8
It really sucks when CI breaks just by bumping .NET version, I spent a few weeks fixing that for hundreds of builds back when there was a breaking path change in the CLI behavior back in the Core 2 -> 3 transition.

I'd much much rather it to be opt-in for .NET 8, and then defaulted on for .NET 9
That allows eager people to opt in, and gives time for people to learn about the change and adjust pipelines at their leisure.

@baronfel
Copy link
Member

@ericsampson don't worry - on by default is not in the cards currently. Details in this section, but everything will be opt in at least for .NET 8 - we recognize that there are huge compat concerns with silently changing output paths!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests