search

brandedoutcast / publish-nuget

📦 GitHub action to automate publishing NuGet packages when project version changes
https://github.com/marketplace/actions/publish-nuget
MIT License
197 stars 101 forks source link

BUGFIX: fixes #58 where pushing package symbols produced false negatives #62

Open waldosax opened 3 years ago

waldosax commented 3 years ago

BUGFIX: fixes #58 where pushing package symbols produced false negatives. (Parameter cannot be null: path2)

Apparently, the errant space in the arguments makes the _executeCommand function think there's one more empty paramater at the end. This makes dotnet nuget push the package, the symbols, and a third empty package.

Simply transposing where the space appears in the ternary solves this problem.

AraHaan commented 3 years ago

Tbh the input in the action INCLUDE_SYMBOLS I found out can be nuked:

Just that this would be needed to be explained in the readme:

  • When wanting to include symbols in an symbols package set these as well in the csproj or an Directory.Build.props file of the project being packaged:
    • <IncludeSymbols>true</IncludeSymbols>
    • <SymbolPackageFormat>snupkg</SymbolPackageFormat>

Then the dotnet pack command would not need to specify --include-symbols=true /p:SymbolPackageFormat=snupkg and instead retrieve it from the actual project itself if they are set by the user.

This then simplifies the action with less inputs required / needed and easier setup and maintainability.

I will look at making an PR that will fix this.

akamud commented 3 years ago

I see value in completing this PR first. Anything else needed for this to be merged?

AraHaan commented 3 years ago

I see value in completing this PR first. Anything else needed for this to be merged?

You can try my fork that applies all of the PRs open that I know of here to it in a way that actually works (even the GPR one) a few of the other PRs combined with mine broke it until I spent an entire day debugging it until it worked thanks to running it in an action that I kept repeating it because I pinned it to the tip of main.

https://github.com/Elskom/publish-nuget/

Also if interested try the build-dotnet action that I made as well.

waldosax commented 3 years ago

I see value in completing this PR first. Anything else needed for this to be merged?

Not a thing just merge it.

pl-aknight commented 3 years ago

Yes, please merge!

cbenard commented 2 years ago

I'm sad this Action is abandoned. 😢 I used @waldosax's fixed version thanks to @rwasef1830 (I didn't know how to use unpublished Actions via commit hashes before).

AraHaan commented 2 years ago

I replaced this github action anyway

check out: https://github.com/Elskom/setup-latest-dotnet (replaces setup-dotnet to always give the absolute latest .NET SDK) https://github.com/Elskom/build-dotnet (replaces this action and restores, builds, tests, packs, and pushes projects to nuget.org (or any other feed))

tihomir-kit commented 2 years ago

@cbenard how do you know it's abandoned? Lack of work on it and just a guess or did the maintainer state that somewhere?

Does anyone know if there's a good, active alternative to switch to? I see this Elskom one has been linked, but it only has 2 stars, which makes it look not so promising for now.

alirezanet commented 2 years ago

Any update on this? I have the same problem not sure what to do...

alirezanet commented 2 years ago

ok, I did not find any other trustable repo so I used @waldosax fix and created my own nuget-push 

uses: alirezanet/publish-nuget@v3.0.0

thanks waldosax

jzabroski commented 2 years ago

@alirezanet What was wrong with @AraHaan 's repository? Why can't you trust that one? Kind of confused.

jzabroski commented 2 years ago

I see. One immediate problem with @AraHaan 's action is he didn't publish it to the GitHub Action marketplace, but you published yours.

The other, weird, thing I noticed is the Publish Nuget marketplace page for the original project has code that is actually referencing... a different action than the one in the marketplace. https://github.com/marketplace/actions/publish-nuget - I wonder if this should be flagged with GitHub ? Seems very risky that the package is purporting to be one thing, but then recommending you use a different package.

image

alirezanet commented 2 years ago

@alirezanet What was wrong with @AraHaan 's repository? Why can't you trust that one? Kind of confused.

nothing is wrong with that repo, because he did merge all open pull requests that repo for me had a few breaking changes, for example, in that repo you don't have INCLUDE_SYMBOLS: option ... etc

my version is almost the same as the original one + this PL

alirezanet commented 2 years ago

I see. One immediate problem with @AraHaan 's action is he didn't publish it to the GitHub Action marketplace, but you published yours.

The other, weird, thing I noticed is the Publish Nuget marketplace page for the original project has code that is actually referencing... a different action than the one in the marketplace. https://github.com/marketplace/actions/publish-nuget - I wonder if this should be flagged with GitHub ? Seems very risky that the package is purporting to be one thing, but then recommending you use a different package.

image

yeah, that's another reason I created my own version :) I was confused like you

jzabroski commented 2 years ago

@alirezanet Does yours contain all the hotfixes, or just this one you needed?

waldosax commented 2 years ago

It's the same package. Guy just changed his name.

On Wed, Dec 8, 2021 at 12:58 PM John Zabroski @.***> wrote:

@alirezanet https://github.com/alirezanet Does yours contain all the hotfixes, or just this one you needed?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/brandedoutcast/publish-nuget/pull/62#issuecomment-989043022, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOK53C6UUAVJ2TDEGGILPVLUP6MDVANCNFSM46BIMATQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

alirezanet commented 2 years ago

@alirezanet Does yours contain all the hotfixes, or just this one you needed?

No, just this one.

waldosax commented 2 years ago

Mine only has those since I forked it and fixed it. Haven’t merged in anything since then.

Sent from my iPhone

On Dec 8, 2021, at 1:19 PM, AliReZa Sabouri @.***> wrote:

 @alirezanet Does yours contain all the hotfixes, or just this one you needed?

No, just this one.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.