Skip to content

Conversation

@jbogard
Copy link

@jbogard jbogard commented Aug 13, 2019

This also removes the System.Reflection.Emit.Lightweight dependency in favor of expression tree compilation.

I left the test projects alone since they target netcoreapp2.1. In order to test against netstandard1.5 you'd have to add netcoreapp1.x target....which is deprecated.

@jbogard jbogard changed the title Adding netstandard2.0 support to libraries CSHARP-2592: Adding netstandard2.0 support to libraries Aug 13, 2019
@Digi59404
Copy link

This looks good! Thanks James!

@jbogard
Copy link
Author

jbogard commented Aug 20, 2019

Thoughts? Comments? Concerns?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ship it! :)

@vincentkam
Copy link
Contributor

Thanks for your PR! The .NET Driver team is considering adding support for .NET Standard 2.0 soon

@horeaper
Copy link

horeaper commented Nov 7, 2019

Thanks for your PR! The .NET Driver team is considering adding support for .NET Standard 2.0 soon

Is there an time line for that? Currently when publishing a .net core 3.0 app with MongoDB driver, error NU1605 will block the process. It can be ignored but that requires modifing every project file that uses the driver, which is quite tedious for large solutions.

@adamhathcock
Copy link

Any chance to move on to the better netstandard?

@jbogard
Copy link
Author

jbogard commented Nov 25, 2019

@adamhathcock I'd fix the merge conflicts but I don't know if this PR is still under review for merge 🤷‍♂

@AlseinX
Copy link

AlseinX commented Jan 13, 2020

Looking forward to it. But as it shows, a conflict in src/MongoDB.Driver.Core/Core/Compression/SnappyCompressor.cs still exists.
When will it be solved and merged?

@jbogard
Copy link
Author

jbogard commented Jan 13, 2020

@AlseinX same comment, don't know if this PR is under review. I didn't get any feedback so I don't want to go through the work of fixing merge conflicts if there's no point

@AlseinX
Copy link

AlseinX commented Jan 14, 2020

This PR is really helpful and the removal of Emitting brings much more compatibility to some runtime implementations.
It sounds disappointing that the maintainers of an open source project ignore a helpful PR for months.
Commonly a PR is reviewed in several days when it is ready for review, either merged, rejected, requested changes, or given a reason why it is being on hold, as long as the project is still active, otherwise the readme file would contain "This repo is developed by the official team only and contribution is not welcomed".

@rstam
Copy link
Contributor

rstam commented Jan 14, 2020

I'm sorry we haven't gotten to this yet. We are definitely planning to target .NET Standard 2.0 as soon as possible. The main issues that have stopped us from doing it sooner are:

  1. Uncertainty regarding whether it's OK to drop support for .NET Standard 1.5 and just replace it with .NET Standard 2.0
  2. If we keep support for .NET Standard 1.5 how do we test both the netstandard15 and netstandard20 artifacts

We are researching whether any of our users would be impacted by removing support for .NET Standard 1.5.

Note that once we target .NET Standard 2.0 we also need to review the #if's in the entire code base to see what features can now be re-enabled once we target .NET Standard 2.0 (and are no longer constrained by the more limited APIs available in .NET Standard 1.5). We plan to do that as a separate effort.

Can someone elaborate on why removing use of Emit is helpful? Where is Emit not supported?

Thanks!

@Digi59404
Copy link

Hey @rstam I can confirm that Emit is not supported on Xamarin iOS Devices or Devices that do not allow dynamic generation of code. Microsoft has released a hotfix for this by enabling an interpreter to run on iOS. But this affects performance and causes other issues.

#377
https://devblogs.microsoft.com/xamarin/introducing-xamarin-ios-interpreter/

@rstam
Copy link
Contributor

rstam commented Jan 14, 2020

@Digi59404 thanks for that information. That's helpful.

@AlseinX
Copy link

AlseinX commented Jan 15, 2020

As for the issues:

  1. Actually the .NET Implementations that do not support netstandard2.0 but support netstandard1.5 are out of support and deprecated, and the projects that are built against netstandard1.5 only (ASP.NET Core 1.x/2.x targets netstandard rather than netcoreapp) are also compatible to netstandard2.0 by design. The ancient projects that never intend to upgrade the target platform are basically inactive, no longer maintained and more unlikely to update the mongodb driver.

  2. Even if it is really necessary to keep support for netstandard1.5, and test for both targets separately are needed. Assuming that there is an exception to make the test pass on netstandard1.5 and fail on netstandard2.0 (by design, this is impossible unless there is conditional compilation to behave specifically, or mongodb driver is designed to work with a bug that only occurs on netstandard1.5 implementations), the projects that run on a netstandard2.0 implementation would not work as well even if the mongodb packages pass the test on a .netstandard1.5 implementation. Currently the mongodb csharp driver is tested on netstandard1.5 implementations but actually used on netstandard2.0+ implementations. If the assumption is a truth, it is broken already. So the assumption is not true, therefore the current version that passes the test on .netstandard1.5 implementation does not need a test on a netstandard2.0 implementation.

@jbogard
Copy link
Author

jbogard commented Jan 15, 2020

I've fixed merge conflicts here.

@jbogard
Copy link
Author

jbogard commented Jan 16, 2020

It looks like just adding netstandard2.0 support breaks some tests around ISupportInitialize:

image

Even if I go back to using the Reflection.Emit approach, these tests break. This will need some investigation into why netstandard2.0 breaks but netstandard1.5 works

@jbogard
Copy link
Author

jbogard commented Jan 24, 2020

@rstam @vincentkam I've fixed the test failures by adding the NETSTANDARD2_0 in the places where it conditionally compiles NET452. Build is green now.

@jbogard
Copy link
Author

jbogard commented Jan 24, 2020

I rebased and squashed this PR because it had a bunch of useless old commits

@adamhathcock
Copy link

Somehow, there’s already conflicts. Sorry @jbogard

I still think out of support versions of net standard should be dropped.

…nditional compilations for netstandard2.0

CSHARP-2592: Adds support for netstandard2.0
@jbogard
Copy link
Author

jbogard commented Jan 24, 2020

Merged from upstream, which incorporated CSHARP-2394

@jbogard
Copy link
Author

jbogard commented Jan 24, 2020

@adamhathcock not anymore ;)

@DmitryLukyanov
Copy link
Contributor

Thanks @jbogard for your participation, the changes are already in master. They will be available to consume in the next release

@jbogard jbogard closed this Jan 29, 2020
@jbogard jbogard deleted the netstandard20 branch January 29, 2020 15:54
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

Successfully merging this pull request may close these issues.

8 participants