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

[Discuss] Fix packaging netstandard20 #3454

Merged
merged 2 commits into from
Aug 25, 2017

Conversation

matthid
Copy link
Contributor

@matthid matthid commented Aug 17, 2017

Includes the patch of how the nuspec should look with netstandard20. See #3451

@msftclas
Copy link

@matthid,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@matthid matthid changed the title Fix packaging netstandard20 [Discuss] Fix packaging netstandard20 Aug 17, 2017
@matthid
Copy link
Contributor Author

matthid commented Aug 17, 2017

/cc @enricosada

@KevinRansom
Copy link
Member

@dotnet-bot test Windows_NT Release_ci_part3 Build

@matthid
Copy link
Contributor Author

matthid commented Aug 17, 2017

@KevinRansom note that this cannot work as the netstandard20 binaries are not build jet. But there is no reason to invest time into it when I'm pretty sure that nobody will accept it afterwards ;)

@matthid
Copy link
Contributor Author

matthid commented Aug 17, 2017

@enricosada do you know what would happen if we add the empty group, but no actual 20 binaries.
We probably shouldn't do such experiments with FSharp.Core, but as of right now I think that could work as well.

@matthid matthid force-pushed the fix_packaging_netstandard20 branch from a6d6d50 to f497a91 Compare August 17, 2017 21:03
@KevinRansom
Copy link
Member

@matthid @forki.

I have chatted with a few more people internally. We will ship a NetStandard2.0 implementation and a NetStandard1.6 implementation and we will continue to keep them up to date, for now, I don't have a it scheduled but we will ensure it happens.

@forki
Copy link
Contributor

forki commented Aug 18, 2017 via email

@dsyme
Copy link
Contributor

dsyme commented Aug 21, 2017

Thanks to @matthid , @forki, @KevinRansom for working together to come to a common understanding and path forward - it's a great example of community-vendor cooperation

@matthid
Copy link
Contributor Author

matthid commented Aug 25, 2017

Ok I did some testing with this and it seems to work just fine (and as expected).
Is there a reason we don't want to take this as is?

Technically, we should ask someone from the runtime team just to make sure those packages/dlls are really not required when running on netcore2 (but I'd assume they are not).

Obviously we want to do some further tests than what I did:

  • dotnet new console/library check that pack/publish works as it should and run works
  • Check that it works with paket and gets rid of all the netstandard16 packages -> it did

I don't expect any old use-case to break (because NuGet would still prefer the old groups) but probably we want to test some of those.

Wouldn't this solve all the issues we have without providing another binary? Size would still be the same, just metadata is different. I think this might be worth investigating further.

If you want to test this yourself you can use the following NuGet.config

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <solution>
    <add key="disableSourceControlIntegration" value="true" />
  </solution>
  <packageSources>  
    <!--To inherit the global NuGet package sources remove the <clear/> line below -->
    <clear />
    <add key="appveyor" value="https://ci.appveyor.com/nuget/customnuspecs" />
  </packageSources>
</configuration>

and FSharp.Core version 4.1.19-alpha001 (lock it).

I understand that we might want to do further testing before merging this. Any specific scenario worth testing?

@0x53A
Copy link
Contributor

0x53A commented Aug 25, 2017

I would have expected this to blow up at runtime because of the missing shims, but couldn't get it to...

The shims are probably bundled with dotnet sdk?

It is still not a clean solution, because it does depend on the shims without declaring it.

So my vote is for shipping an explicit netstandard2.0 dll additionally to netstandard1.6

@matthid
Copy link
Contributor Author

matthid commented Aug 25, 2017

@0x53A Yes I think the standard usually is implemented in the runtime and therefore the runtime has all the required shims.

The only thing I can think of are some publish self contained app scenarios, but only if they do some optimizations there..

@matthid
Copy link
Contributor Author

matthid commented Aug 25, 2017

For example if they detect that the netstandard16 shims are not required they leave them out. But I don't think they do such optimizations at all.

@0x53A
Copy link
Contributor

0x53A commented Aug 25, 2017

But I don't think they do such optimizations at all.

yet

@matthid
Copy link
Contributor Author

matthid commented Aug 25, 2017

Just one thing: I definitely think that there might be dragons hiding somewhere (as I don't know all the details). But even if there are: Identifying them might help in understanding the ecosystem better. So we should definitely look into this.

But I don't think they do such optimizations at all.

yet

Thinking about it again: They shouldn't use NuGet metadata for such optimizations, instead they should analyze the actual binaries.

@0x53A
Copy link
Contributor

0x53A commented Aug 25, 2017

They shouldn't use NuGet metadata for such optimizations

I'm not so sure. The nuget metadata is basically a contract - if you "lie" you are on your own.

@matthid
Copy link
Contributor Author

matthid commented Aug 25, 2017

I'm not so sure. The nuget metadata is basically a contract - if you "lie" you are on your own.

@0x53A As much as I'd like that to be true I have seen that the reality is different. Problem is that the information flow stops at compiler level. We just had some discussion about assembly versions in NuGet packages. As long as there is no actual standard of what is allowed in a package and what not this is merely your own wishlist ;). I still think if I release a new Package-Version with a lower Assembly-Version it should work, but it doesn't ;)

Nobody says or disallows that your binaries should be different from your dependency groups (https://docs.microsoft.com/en-us/nuget/schema/nuspec). You can write pretty much write everything you want.

So if they do this optimization based on NuGet metadata and break "this" scenario I consider that a bug in the optimization.

@KevinRansom
Copy link
Member

According to Immo, this is fine and should be done. Apparently it means that when installed in a dotnet standard 2.0 or up app, no additional dependencies are necessary

So we can take this change and see how things go.

Thanks for pushing on this.

Kevin

@KevinRansom KevinRansom merged commit d30be47 into dotnet:master Aug 25, 2017
@matthid
Copy link
Contributor Author

matthid commented Aug 25, 2017

Tbh. I honestly don't know. But yeah if we are able to release a quick hotfix if things go sideways that should work as well.

@matthid
Copy link
Contributor Author

matthid commented Aug 25, 2017

@0x53A

So my vote is for shipping an explicit netstandard2.0 dll additionally to netstandard1.6

They made clear that this is not an option because of technical restrictions. That's why I came up with this workaround in the first place...

@KevinRansom
Copy link
Member

About a one day turn around for fixes.

@0x53A If this eliminates us having to create a specific netstandard 2.0 dll then I am very happy. In fact I had just yielded to the idea I was going to have to do that. As you can see about 10 messages up, but if this satisfies your scenarios, then having a single fsharp.core dll is much .... much preferable.

@0x53A
Copy link
Contributor

0x53A commented Aug 25, 2017

I this works, then I am happy too ;) I was maybe just a bit paranoid about this stuff

@KevinRansom
Copy link
Member

Not as paranoid as me not wanting to ship another version of FSharp.Core ... I really didn't want to do that. So I am hoping this satisfies everyones needs.

@matthid
Copy link
Contributor Author

matthid commented Aug 25, 2017

Oh I really hope there are no dragons hidden in there. It would really make me sleep better if one of the runtime/nuget/sdk team could look at this. Because FSharp.Core being an exception to the rule might hurt us sooner or later. So let us see this as what it is: A workaround for the time being. Once people have moved to netstandard20 the world will be a better place (hopefully)

@KevinRansom
Copy link
Member

They realized that they need to add this ability to dotnet cli publish without nuspec. So it should be fine.

@0x53A
Copy link
Contributor

0x53A commented Aug 25, 2017

But that means that even though I don't need to reference the supporting nuget packages explicitly, the shims are still copied to the /bin/ directory, which could be avoided if we published a real netstandard2.0 version, right?

@KevinRansom
Copy link
Member

NETstandard 2.0 is fully upwards compatible with NETstandard 1.6. And so ... anything that can run NETstandard 2.0 must be able to "by definition" run NETstandard 1.6 libraries. So the necessary type forwarding assemblies must already be present on a NETstandard 2.0 shared framework.

Anything copied to the bin directory is a build artifact ... to publish an app use dotnet publish

@dsyme
Copy link
Contributor

dsyme commented Aug 26, 2017

I said this above already - but thanks to @matthid , @forki, @KevinRansom, @0x53A for working together on this. Also thanks to @terrajobst for reviewing. Again a great example of positive community+vendor collaboration on tricky issues

p.s. If there is a trail of technical issues with this let's all be patient and work together to resolve them :)

@matthid
Copy link
Contributor Author

matthid commented Aug 26, 2017

Ah somehow I didn't realized this was reviewed. Thanks @dsyme for clarifying - somehow I missed that. I'm really looking forward to this. It should help making the next FAKE release a lot more useful.
Thanks @KevinRansom for the quick merge after clarifying the impact (which I apparently didn't realized, sorry for that).
Thanks @0x53A for trying to break it, you seem to have way more knowledge of this new stuff so I'm really glad you look over the stuff we do :)

And yes I agree let's tackle the issues as they flow in.

@praeclarum
Copy link

Would you please remove the dependencies from Xamarin also in the nuspec ? If you don’t, then xamarin will download the entire BCL with project.json projects.

@forki
Copy link
Contributor

forki commented Aug 26, 2017 via email

@0x53A
Copy link
Contributor

0x53A commented Aug 26, 2017

One case where this might cause issues is if the dll is loaded by a host other than dotnet.

For example fsi or msbuild.

Are you sure that all netcoreapp2.x hosts will also bundle all netstandard1.x shims?

By that I mean:

I compile a netcoreapp2.x standalone executable.
My app loads FSharp.Core.

If FSharp.Core were compiled against netstandard2.0, then it will work out of the box.
If FSharp.Core is compiled against netstandard1.6, then I need to redistribute all ~100 facades with my app.


To go back to a more concrete example: MSBuild seems to currently ship the netstandard1.3 facades. It will surely ship the netstandard 2.0 facades, if not already, then in the future. Does it also ship the netstandard1.6 facades? And will it for future versions, instead of dropping them in support of 2.0?

I couldn't find anything recent, but there are a ton of old threads talking about this, e.g. dotnet/msbuild#1542 dotnet/sdk#629

Again, if you have checked this internally with the respective teams and you are sure this is safe, then please ignore my paranoia and go ahead, but I am still a little bit skeptical.


@matthid
Copy link
Contributor Author

matthid commented Aug 26, 2017

If FSharp.Core is compiled against netstandard1.6, then I need to redistribute all ~100 facades with my app.

Now that you mention that
We probably should do some testing with the net461 tooling supported special case ...

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.

7 participants