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

PCLCrypto required dependency version too high #4

Closed
ndonohoe-bjss opened this issue Aug 9, 2016 · 19 comments
Closed

PCLCrypto required dependency version too high #4

ndonohoe-bjss opened this issue Aug 9, 2016 · 19 comments

Comments

@ndonohoe-bjss
Copy link

The 0.0.4 nuget package requires the dependency PCLCrypto 2.0.147, this forces the inclusion of additional dependencies for BCrypt and NCrypt which are not compatible with Win 8.1 apps according to this issue AArnott/PCLCrypto#97

Is there any reason for this to be required or can the dependency be put down to 1.0.86 (or similar)? It doesn't look like there was any reason for the increase.

@dvsekhvalnov
Copy link
Owner

hi @ndonohoe-bjss ,

no special reason, i believe i just referenced latest version.

Feel free to submit patch request.

@leastprivilege
Copy link

No it is not as easy as sending a pr. There is now software out there that relies on josepcl (https://github.com/IdentityModel/IdentityModel.OidcClient).

When we started the project we had issues with the 1.x version of pclcrypto, which were all going away with 2.x. This is all documented on the issue tracker.

So reverting back would need more investigation.

Sent from my iPad

On 09 Aug 2016, at 21:37, dvsekhvalnov [email protected] wrote:

hi @ndonohoe-bjss ,

no special reason, i believe i just referenced latest version.

Feel free to submit patch request.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@dvsekhvalnov
Copy link
Owner

@leastprivilege , thanks for input.

Let's see what PCLCrypto plan to fix Win8.1 issue first.

@ndonohoe-bjss
Copy link
Author

@leastprivilege Can you elaborate on the issues with 1.x?

Changing the dependency to 1.x would not prevent you using 2.x, it would just allow the usage of 1.x as well.

It does not look like they plan to do anything about the 8.1 issue

@leastprivilege
Copy link

We had issue with v1 on Xamarin iOS/Android. Can't remember the details.

Needs more investigation as I said.

@leastprivilege
Copy link

Maybe using the core/netstandard version of jose-jwt would be the right approach - now that it is available for all platforms.

@dvsekhvalnov
Copy link
Owner

Hey guys, since i not too deep in Xamarin development, can you figure out yourself what is right PCLCrypto dependency? I don't have much preference.

@ndonohoe-bjss , may be https://github.com/dvsekhvalnov/jose-rt will do the trick for you?

@ndonohoe-bjss
Copy link
Author

The right dependency is >= 1.x.x as this means everyone can make use of the library. Putting it to 2.x.x means that no one targeting Win 8/.1 or Win Phone 8/.1 can use the library, or any other libraries that depend on it.

Right now I have an SDK using jose-pcl and sample applications for various platforms (UWP, ASP, Win8). For the SDK I have had to use jose-pcl 0.0.3 so that I am able to use pclcrypto 1.0.86, for the Win8 sample application I include the pclcrypto 1.0.86 dependency and for the others I use pclcrypto 2.0.147, all sample applications work fine with this setup and the Win8 app passes the Supported APIs test.

So the sensible course of action would be to drop down to 1.x.x and allow anyone creating a xamarin supporting library to set their dependency to 2.x.x (or instruct their users to include pclcrypto 2.x.x in their xamarin apps)

@dvsekhvalnov
Copy link
Owner

ok, @ndonohoe-bjss , so how desired nuget.spec would look like?

@ndonohoe-bjss
Copy link
Author

The nuspec file on master has the correct dependency however the 0.0.4 version seems to have been published with a different nuspec file

@leastprivilege
Copy link

Again - it is not so easy in the OSS space. Strictly speaking - making such a change is a breaking change which in turn requires you to manifest that as a breaking change too via SemVer.

Now this is all pre-release by design - and we are really talking about common sense here.

But it is fine - do whatever you want. I forked jose-pcl and will embed it if needed since I cannot rely on you playing by the OSS rules. I have a dependency on that and real users out there relying on it.

As I said before - ideally the PCL version is not needed anymore anyways because netstandard is now reality.

Don't get me wrong - you have always been very helpful (and in fact, that's what you are trying to be right now as well). But to make your libs a viable option for other OSS projects, some basic playing by the rules is needed.

Do you have a non pre-release .NET Core/netstandard version of jose-jwt now published? And will you respect semver for this project at least?

@dvsekhvalnov
Copy link
Owner

dvsekhvalnov commented Aug 15, 2016

sorry, forgot to push file :) check now.

Well, i think i've updated for no real reason, just took latest at some point.

@leastprivilege since you were the main person asking about pcl version can you make sure you really need 2.x.x dependency? Or if we set > 1.8 - you can override in your library and it still can work for you?

@ndonohoe-bjss
Copy link
Author

@leastprivilege how would you say this is a breaking change? Updating to a version that targets minimum 1.x.x will not change the dependency version currently being used by anyone, so there would be no break for anyone as far as I can see

@dvsekhvalnov
Copy link
Owner

@leastprivilege , https://www.nuget.org/packages/jose-jwt/ - is 2.0.1 for now.

Too be honest i'm trying not to make any breaking changes at all until it's really possible. That's why i'm trying to figure out what to do here.

I probably need to verify it, but if:

  1. jose-pcl requires PCLCrypto > 1.x,
  2. your library requries jose-pcl and PCLCrypto 2.x
  3. clients of your library will get PCLCrypto 2.x by dependency resolution

and if somebody requires jose-pcl on its own, it will get PCLCrypto 1.x, not?

@leastprivilege
Copy link

The real question is - if you compile Jose-Plc against 1.x - will it work at runtime against 2.x (which is a breaking change by definition). It might work, or not.

Sent from my iPad

On 15 Aug 2016, at 11:59, dvsekhvalnov [email protected] wrote:

@leastprivilege , https://www.nuget.org/packages/jose-jwt/ - is 2.0.1 for now.

Too be honest i'm trying not to make any breaking changes at all until it's really possible. That's why i'm trying to figure out what to do here.

I probably need to verify it, but if:

jose-pcl requires PCLCrypto > 1.x,
your library requries jose-pcl and PCLCrypto 2.x
clients of your library will get PCLCrypto 2.x by dependency resolution
and if somebody requires jose-pcl on its own, it will get PCLCrypto 1.x, not?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@dvsekhvalnov
Copy link
Owner

dvsekhvalnov commented Aug 15, 2016

Yep, that's right question. I really have no idea how to prove it except writing test apps for all possible platforms and then testing upgrade.. (

In reality my preferred strategy will be not to touch anything, since package is published, may have users. @leastprivilege library based on jose-pcl can have users as well. And IMO risk of breaking the chain is probably not worth it.

@ndonohoe-bjss i can fork and publish special jose-pcl version based on older PCLCrypto for instance. Or if you want - feel free to do it yourself, you'll probably have time to time merge new stuff if it arrives.

Other option can be split versions:

jose-pcl version 2 to explicitly point PCLCrypto 2.x
jose-pcl version 1 to explicitly point PCLCrypto 1.x
jose-pcl version 0 - leave as is.

Thoughts?

@ndonohoe-bjss
Copy link
Author

I will just continue to use 0.0.3 then. If any updates are required I will just split my library so that I can use jose-rt for win8 and jose-pcl for all others.

The readme should be updated to state that any Win/Phone 8 apps built with the latest version will not pass store certification tests however.

@dvsekhvalnov
Copy link
Owner

Ok, i'll put comments.

Sorry guys, i really hate such situations when you depend on 3rd party and can't do much about it. (

@dvsekhvalnov
Copy link
Owner

dvsekhvalnov commented Aug 16, 2016

@ndonohoe-bjss , i added notes: https://github.com/dvsekhvalnov/jose-pcl#which-version

closing as we can't do much right now.

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