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

NuGet: Remove strict dependencies on OpenSSL and PicoJSON #288

Open
sven-molkenstruck opened this issue Apr 25, 2023 · 8 comments
Open

NuGet: Remove strict dependencies on OpenSSL and PicoJSON #288

sven-molkenstruck opened this issue Apr 25, 2023 · 8 comments

Comments

@sven-molkenstruck
Copy link

sven-molkenstruck commented Apr 25, 2023

Hi,

I am using NuGet to include jwt-cpp into my project. Unfortunately it force-installs Edit: an outdated a specific old NuGet package of OpenSSL, openssl-vc141-static-x86_64.1.1.0, which does not work under Visual Studio 2022 (vc143).
I suggest to remove those dependencies and let the user decide and care about OpenSSL. Edit: I might want to link a vc143 version from NuGet, e.g. "zeroc.openssl.v143", or some other version from some other source.

That strict dependency is a contradiction to the README statement at https://github.com/Thalhammer/jwt-cpp#readme , "In the name of flexibility and extensibility, jwt-cpp supports... [various OpenSSL versions and alternatives like LibreSSL]".

From https://github.com/Thalhammer/jwt-cpp/blob/master/nuget/jwt-cpp.nuspec lines 17-20:

    <dependencies>
      <group targetFramework="native0.0">
	    <dependency id="PicoJSON" version="1.3.0" />
	    <dependency id="openssl-vc141-static-x86_64" version="1.1.0" />
	  </group>
    </dependencies>

@diogo-strube It seems you are the expert about this :-)

Can you please simply remove those dependencies, or is there something I'm not seeing?
Thank you!

Sven

@prince-chrismc
Copy link
Collaborator

Maybe it should be a version range https://learn.microsoft.com/en-us/nuget/concepts/dependency-resolution?

@sven-molkenstruck
Copy link
Author

Thanks Prince. I think I need to clarify my post above.
The problem is not the dependency on version 1.1.0 of that openssl package. The problem is that it depends on that package at all, instead of allowing me to use ANY openssl (or LibreSSL or whatever) package. The user should be allowed to link any openssl or libressl or whatever lib to their project, even from a different source than from NuGet. It IS possible when I include jwt-cpp in any other way, but not when I use NuGet.
So I think inside the NuGet configuration there should not be any dependency listed.

@Thalhammer
Copy link
Owner

Thalhammer commented Apr 26, 2023

It's been a fairly long time since I used NuGet in particular, but if I use a dependency manager I kind of expect it to pull in all dependencies of the package I install. It feels like that's its job. The issue here is in my opinion that only one possible package specified. In other systems its possible to specify a list of compatible packages and versions, one of which has to be installed, but I am not sure if NuGet can do this and if, how. jwt-cpp definitely does support all the listed ssl libraries (and probably more as long as they are somewhat api compatible), we even run CI tests on every commit to make sure it does.
Looking at the docs it seems to be possible to specify a list of supported versions, but you can't specify a list of packages.
So basically we seem to be presented with three ways:

  • Not specify a ssl dependency at all. I'd like to avoid that, because it kinda goes againts the point of a dependency manager.
  • Specify a single library and tell users who need a different one to go install jwt-cpp manually. This sounds as bad as a solution as the above (possible worse).
  • Publish one package for every supported SSL type. Still not perfect, but better than not having a dependency at all.

Personally I'd go with the last option, possible with a fourth package that does not depend on any library. Basically

  • jwt-cpp
  • jwt-cpp-openssl
  • jwt-cpp-wolfssl
  • jwt-cpp-libressl

EDIT: Thinking some more about it we kinda have the same issue with json libraries. Currently we depend on picojson, but we support a bunch of others.

EDIT2: Seems like its not that simple because nuget doesn't seem to be able to automatically pick the correct package for the current toolchain, so in addition to having a version for every ssl lib, every json library we'd also need a version for every breaking toolchain version. According to MS You can mix binaries built by different versions of the v140, v141, v142, and v143 toolsets. However, you must link by using a toolset at least as recent as the most recent binary in your app., but thats not really an option because if we always depend on the latest toolset version it prevents users using older VS from using jwt-cpp. So unless we require that (which sounds like a bad idea) we would have to build and publish at least num_ssl(4) * num_json(4) * num_toolset(4) = 64 as of right now, with the expectation for that number to increase in the future. It wouldn't be that much of an issue, because CI can do it, but it still feels wrong. TLDR: I think we might be best off removing all dependencies and putting a big fat warning in the package description with a list of usable/required dependencies.

@sven-molkenstruck
Copy link
Author

sven-molkenstruck commented Apr 26, 2023

Thanks @Thalhammer Very good reply!

I appreciate the concept "Dependency Manager". You are right, when I use NuGet, I should not (and actually I do not) want to choose my favorite sub-dependencies, I just want it to work.
The challenge is that NuGet apparently has no openssl package that is as flexible (regarding platform/toolset) as jwt-cpp is.

As a workaround, I found that I can manually circumvent the dependency to openssl-vc141-static-x86_64.1.1.0 in NuGet using "force uninstall":

  1. Install jwt-cpp. (It also installs the dependent openssl and picojson. Let it.)
  2. Select openssl-vc141-static-x86_64.1.1.0, enable the checkbox "Force uninstall", then uninstall it.
  3. Install any other openssl package in NuGet, e.g. zeroc.openssl.v143, or get and link openssl or libressl or ... in any other way without NuGet.

image

So while I do agree with your conclusion,

TLDR: I think we might be best off removing all dependencies and putting a big fat warning in the package description with a list of usable/required dependencies.

alternatively we could leave everything as it is, and add documentation how to make it work on platforms other than vc141.

Sven

@prince-chrismc
Copy link
Collaborator

Hunter requires nothing
https://github.com/Thalhammer/hunter/blob/49b971ee85ee1aa481176110e1ec612748f8b71b/cmake/projects/jwt-cpp/hunter.cmake

vcpkg hardcodes picojson but no openssl...
https://github.com/microsoft/vcpkg/blob/70992f64912b9ab0e60e915ab7421faa197524b7/ports/jwt-cpp/vcpkg.json#L8 there;s no override feature for a different one that I could find searching quickly

Conan lists openssl (but any recipe that's a provider can be overridden)
https://github.com/conan-io/conan-center-index/blob/a08f48696f5e06bd6dfaa68a35458502bfd73b04/recipes/jwt-cpp/all/conanfile.py#L26 and there's no JSON library

and nuget hardcodes both...


So I guess the bigger question is what should we make the default?

  • Option 1: Require nothing
    • Pro: Users pick everything
    • Con: Its pretty useless if you do not know what you are doing
  • Option 2: OpenSSL only
    • Pro: 95% are going to use it anyway, flexible for JSON library, still useful for signing
    • Con: User needs to know to add a JSON library after the fact
  • Option 3: Force Openssl and Picojson
    • Pro: Just works with default examples
    • Con: Extra to change the the JSON library

Personally I think 2️⃣ is the best, but let me know if you think differently

@Thalhammer
Copy link
Owner

Hunter requires nothing

Hunter requires what you select. It basically sets up a folder so that find_package can find the stuff it needs, but it seems we only have hunter_add_package for OpenSSL in there. Theoretically it could support all of them though (assuming we add support to the cmakelists.txt). You could then set the required one normally using JWT_SSL_LIBRARY and it would "depend" on what you select.

@Sven-HP Even though that workaround works for you I don't think we should treat it as the default solution for this issue, because it doesn't really work for CI because the way I understand it you need to do the uninstall step manually every time you set up the project on a new machine.

@prince-chrismc
Copy link
Collaborator

but it seems we only have hunter_add_package for OpenSSL in there.

I did not see that when I look 👀 I just noticed its in out repo, so we having to add it here which is kinda gross but easier to maintain.

That's 2 for option 2️⃣ IT's just nuget and vcpkg that we need to update to match?

@sven-molkenstruck
Copy link
Author

@Sven-HP Even though that workaround works for you I don't think we should treat it as the default solution for this issue, because it doesn't really work for CI because the way I understand it you need to do the uninstall step manually every time you set up the project on a new machine.

I have tested that. It does work for CI. My workaround only has to be done once, when adding jwt-cpp to the project, and not on every machine. The result is the desired package selection (here: openssl-vc143 instead of openssl-vc141) stored correctly in my project files (*.vcxproj, packages.config), and that gets committed into my project's repo as usual, and works out of the box on other machines. :-)

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

No branches or pull requests

3 participants