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

When nupkg exists on push --skip-duplicate, don't automatically push snupkg #9647

Closed
tidyui opened this issue Jun 4, 2020 · 21 comments · Fixed by NuGet/NuGet.Client#4373
Closed
Assignees
Labels

Comments

@tidyui
Copy link

tidyui commented Jun 4, 2020

When uploading packages with --skip-duplicate duplicate nupkg packages are skipped, but symbol packages are uploaded again.

You can take a look at this script from GitHub Actions which causes this behavior.

https://github.com/PiranhaCMS/piranha.core/blob/master/.github/workflows/publish_to_nuget.yml

Best regards

@donnie-msft
Copy link
Contributor

Which version of dotnet are you using?
I recently worked in this area #8148

@tidyui
Copy link
Author

tidyui commented Jun 4, 2020

Hi there! I’m using dotnet nuget push on .net core 3.1.300 on Linux.

@nkolev92
Copy link
Member

nkolev92 commented Jun 4, 2020

Isn't this by design from a NuGet.org perspective? NuGet/NuGetGallery#7681

The client works as designed, it's the server that's defined a contract for snupkgs different from nupkgs.

@tidyui
Copy link
Author

tidyui commented Jun 4, 2020

It's not very functional from a build-script point of view.

  1. When the build script is triggered all projects/packages are built through the pipeline
  2. When everything is done packages are pushed to NuGet.org with the request to skip the ones that hasn't changed (i.e version number already exists).

However since symbol packages are accepted all packages that was skipped goes into this state when logged into NuGet.org.

Skärmavbild 2020-06-04 kl  20 45 49

It's not visible for the person consuming the packages, but it's not optimal.

Best regards

@donnie-msft
Copy link
Contributor

The documented behavior of --skip-duplicate doesn't indicate that it skips packages that haven't changed:
When pushing multiple packages to an HTTP(S) server, treats any 409 Conflict response as a warning so that the push can continue. Available since .NET Core 3.1 SDK.
https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-nuget-push#options

If you disagree with the nuget.org implementation, please contribute to an issue I opened here: NuGet/NuGetGallery#7681

@nkolev92
Copy link
Member

nkolev92 commented Jun 4, 2020

It's not very functional from a build-script point of view.

I buy that, I just want the conversation to reach the right audience (which is why I mentioned the issue on NuGetGallery side. There were good reasons for that behavior.).

On a different note, different builds, from different sources, should not create the same version of a package. So ensure you are not doing something like that.

Deterministic builds are enabled by default on newer versions of the compiler, so ideally it should generate the same dlls and pdbs, so nuget.org maybe wouldn't complain? That's beyond my expertise though.

@tidyui
Copy link
Author

tidyui commented Jun 4, 2020

@donnie-msft I'm trying to understand what I've misunderstood here 😀 I was certain the --skip-duplicate was intended to just skip uploading of duplicate packages, are you telling me it's the other way around?

@nkolev92 Yes we use deterministic builds, but they point to a certain commit (if I'm not mistaken). Since we have 1 repository, containing projects that will eventually result in 22 different NuGet-packages, and the fact that we might release a hot fix for package x, but not y, this means the entire repository with all packages will be build but only some packages will be updated. However the commit the packages are built from will not be the same for any of the packages.

@donnie-msft
Copy link
Contributor

donnie-msft commented Jun 4, 2020

I was certain the --skip-duplicate was intended to just skip uploading of duplicate packages

No, this option is about ignoring responses from a server.

Here's the test I wrote which illustrates the nupkg/snupkg behavior being discussed.
https://github.com/NuGet/NuGet.Client/blob/dev/test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/PushCommandTest.cs#L615
My comment explains it best:
/// When pushing *.Nupkg with SkipDuplicate, a 409 Conflict is ignored and the secondary symbols push proceeds.

@tidyui
Copy link
Author

tidyui commented Jun 4, 2020

@donnie-msft Just by looking at your comment in the code:

When pushing *.Nupkg with SkipDuplicate, a 409 Conflict is ignored and the secondary symbols push proceeds.

I can see that I can most likely get the behavior I expect just by removing the --skip-duplicate.

As a user I must say the naming of the switch is rather confusing, it would've been clearer if it would've been called --skip-duplicate-warnings or something like that 😀

I'll make sure I update my action workflow and hopefully it will work as expected on next build.

Thanks a lot!

@donnie-msft donnie-msft added Resolution:ByDesign This issue appears to be ByDesign and removed WaitingForCustomer Applied when a NuGet triage person needs more info from the OP labels Jun 4, 2020
@aortiz-msft
Copy link
Contributor

@tidyui That makes sense.

@donnie-msft , @nkolev92 - Would we be willing to accept a PR to change the flag? :)

@nkolev92
Copy link
Member

nkolev92 commented Jun 5, 2020

@donnie-msft , @nkolev92 - Would we be willing to accept a PR to change the flag? :)

To -skip-duplicate-warnings?

The concept of duplicate is owned by the server rather than the client. It's the server that's supposed to ensure that duplicate aren't allowed and reply with with a documented protocol response to indicate that there's been a clash.
Certain 3rd party servers (not nuget.org) can be configured in way that they always accept duplicates.

Prior to the --skip-duplicate flag, a 409 response from the server (the protocol defined code for duplicate) was treated as a failure, an error. If the push was handling multiple packages, this would fail.

The --skip-duplicate switch was added as a response to a common customer ask about issues with scripting, similar to the OP one.

As such, I'm not sure --skip-duplicate-warnings communicates a different idea. I'd argue that it communicate a slightly incorrect message.
Finally when talking about option name, we try to keep it short and as clear as possible.

Personally, I think it might be a good idea to look into improving the description of the option if at all possible.

@donnie-msft
Copy link
Contributor

@anangaur can you comment on this? You and I worked on this feature's naming. I recall this being the result of a Twitter poll...

@tidyui
Copy link
Author

tidyui commented Jun 6, 2020

One final question as I'm not still 100% sure what the option does. If I'm uploading over 20 packages with:

dotnet nuget push '**/Piranha*.nupkg' -k ${{ secrets.NugetAuthToken }}

By omitting --skip-duplicates, is the only difference that .snupkg are not uploaded if there is a 409 for the .nupkg or will the rest of package uploads be skipped if I get a 409 on the first upload?

Regards

@tidyui
Copy link
Author

tidyui commented Jun 6, 2020

So I just tried and when run in a build script, omitting --skip-duplicate actually terminates the deploy first time it encounters a duplicate. So how on earth do I go about skipping duplicate packages without uploading new versions of the symbol packages?

/ Regards

@donnie-msft
Copy link
Contributor

@tidyui It sounds like you want --no-symbols option...

Doesn't push symbols (even if present) (docs)

Any questions about this option, please open a new issue with repro steps, or point to the confusion in existing docs so we can improve those.

@tidyui
Copy link
Author

tidyui commented Jun 8, 2020

@donnie-msft No I don’t want to use —no-symbols, I want to:

  1. Build 20+ packages with my deploy script
  2. Push all packages but skip duplicate packages without breaking the upload.
  3. Push the symbol packages for the packages that were pushed in step 2.

What I don’t want is to push new symbol packages for the ones that got a 409.

Regards

@nkolev92
Copy link
Member

nkolev92 commented Jun 8, 2020

@tidyui

That's not quite possible without some extra work.

Refer to the issue @donnie-msft linked: NuGet/NuGetGallery#7681.
The design for snupkg pushing on nuget.org is different from the design for nupkg pushing.

The NuGetGallery issue would attract the right audience.

@donnie-msft donnie-msft reopened this Jun 8, 2020
@donnie-msft donnie-msft changed the title Pushing packages with --skip-duplicate still updates snupkg When nupkg exists on push --skip-duplicate, don't automatically push snupkg Jun 8, 2020
@donnie-msft
Copy link
Contributor

Reopening this under the context that it's acceptable to break our existing behavior of automatically pushing snupkgs when --skip-duplicate encounters a duplicate nupkg.

@donnie-msft donnie-msft added Product:NuGet.exe NuGet.exe and removed Resolution:ByDesign This issue appears to be ByDesign labels Jun 8, 2020
@donnie-msft donnie-msft added the Type:DCR Design Change Request label Jun 8, 2020
@donnie-msft
Copy link
Contributor

donnie-msft commented Jun 8, 2020

Another comment in support of this change...

In addition to this problem, if a symbol package is available, it is always pushed even if the SkipDuplicate option has been added. We have the following error on the nuget.org side because the package and symbols no longer match.

Symbols package publishing failed. The associated symbols package could not be published due to the following reason(s):
The uploaded symbols package contains one or more pdbs that are not portable.
Once you've fixed the issue with your symbols package, you can re-upload it.

Please note: The last successfully published symbols package is still available for debugging and download.

If both packages are pushed at the same time (e. g. due to a version change), the problem does not occur.

Originally posted by @Aarklendoia in #8148 (comment)

@donnie-msft
Copy link
Contributor

Another comment in support of this change #11013

@sebastienros
Copy link

I also favor a change here. It's very common for me (multiple repositories) to push several packages with symbols. Now one package may fail, or even dotnet push may fail in the middle. If I run the publish script again, it skips the packages, but the symbols are updated and screw everything up because they don't match anymore.

Had the issue last month with Orchard (200 packages, now half of them have wrong symbols) and this morning with another set of package where one package was wrong, so publishing it with the same script broke all other symbols. Only solution is to do a full new set of packages, and hope that all go well in a single step.

At least, if --skip-duplicates could also skip the symbols on a 409 that would fix most problems for me.

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