Skip to content
This repository has been archived by the owner on Oct 26, 2022. It is now read-only.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

waldosax
Copy link

@waldosax waldosax commented Jun 3, 2021

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.

…d false negatives. (Parameter cannot be null: path2)
@AraHaan
Copy link
Contributor

AraHaan commented Jun 14, 2021

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
Copy link

akamud commented Jun 25, 2021

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

@AraHaan
Copy link
Contributor

AraHaan commented Jun 25, 2021

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
Copy link
Author

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

Not a thing just merge it.

@pl-aknight
Copy link

Yes, please merge!

@cbenard
Copy link

cbenard commented Oct 14, 2021

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
Copy link
Contributor

AraHaan commented Oct 14, 2021

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
Copy link

tihomir-kit commented Oct 20, 2021

@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
Copy link

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

@alirezanet
Copy link

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
Copy link

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

@jzabroski
Copy link

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
Copy link

alirezanet commented Dec 8, 2021

@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
Copy link

alirezanet commented Dec 8, 2021

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
Copy link

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

@waldosax
Copy link
Author

waldosax commented Dec 8, 2021 via email

@alirezanet
Copy link

alirezanet commented Dec 8, 2021

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

No, just this one.

@waldosax
Copy link
Author

waldosax commented Dec 8, 2021 via email

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

Successfully merging this pull request may close these issues.

[BUG] error: Value cannot be null. (Parameter 'path2')
8 participants