Disable verify command on Mono #4026
Conversation
| @@ -1124,4 +1124,7 @@ For more information, visit https://docs.nuget.org/docs/reference/command-line-r | |||
| <data name="Multiple_Nupkgs_Detected" xml:space="preserve"> | |||
| <value>Multiple nupkg files detected on '{0}' path to trust, only 1 is allowed.</value> | |||
| </data> | |||
| <data name="VerifyCommand_NotSupported" xml:space="preserve"> | |||
| <value>Package signature validation command is not supported on this platform.</value> | |||
There was a problem hiding this comment.
Technically, verify is supposed to be able to do more than just signature validation, so this might not be very exact.
I think we should consider changing the message accordingly.
Given that nuget.exe on mono is not an extremely common scenario and this is a command that has existed for a very long time without much feedback about this, I'd be comfortable with keeping this message as is as well.
This feedback is mostly about making sure we have all the scenarios considered.
There was a problem hiding this comment.
Forgot to mentioned that beyond this, the change looks great.
I'd be comfortable approving once we confirm that this is something we have considered.
There was a problem hiding this comment.
Thanks for reviewing!
Yes, for now, we only have signature validation for verify command at
https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Commands/VerifyCommand/VerifyArgs.cs#L17-L22. And the signature validation could not be applied on Mono right now (we implement the signature validation in net472 code path by using many P/Invoke to Crypt32.dll, which only works on Windows platform).
I'm not sure what else validation options there will be in the future, and if there is any restrictions for those validation options on Mono. Any thoughts from the team? @NuGet/nuget-client Thanks!
There was a problem hiding this comment.
I'm not sure what else validation options there will be in the future, and if there is any restrictions for those validation options on Mono
You are right, we can't really predict.
So the alternative here would be to only log the message when signing is requested.
Any future limitations can be added by a future implementer.
There was a problem hiding this comment.
Good point! Fixed accordingly.
There was a problem hiding this comment.
Didn't realize it was going to be that easy to make it more specific :D
LGTM
|
|
||
| private bool IsSignatureVerifyCommandSupported() | ||
| { | ||
| #if IS_DESKTOP |
There was a problem hiding this comment.
Why don't you use IS_SIGNING_SUPPORTED here?
There was a problem hiding this comment.
I think there is no difference for the two?
IS_SIGNING_SUPPORTED is net472 + net5.0 for now,
IS_DESKTOP is net472.
While RuntimeEnvironmentHelper.IsMono only returns true when the TFM is net472.
There was a problem hiding this comment.
Does net472 works under MAC? I never tried though.
There was a problem hiding this comment.
Yes. Only in Mono. Mono is an open source implementation of Microsoft's .NET Framework.
If you check the tests running on Mac, you will see the NuGet.CommandLine.Test is able to run on Mac (should also be able to run on Linux but we just didn't). https://github.com/NuGet/NuGet.Client/blob/dev/scripts/funcTests/runFuncTests.sh#L207-L215
|
@erdembayar no problem! Thanks for reviewing! :) |
Bug
Fixes: NuGet/Home#10585
Regression? Last working version:
Description
1.Disable verify command on Mono.
2.Fix tests (add tests for running verify command on Mono)
Since the document already put:
Verification of signed packages is not yet supported under Mono.I think the document is good.PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation