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

RollForward strategy for ikvmc #71

Closed
wants to merge 1 commit into from

Conversation

sergey-tihon
Copy link

@NightOwl888 I believe that with this line we will be able to run ikvmc.exe compiled for netcoreapp3.1 target on machines with net6.0 without installing .NET 3.1 runtime. (as we do here sergey-tihon/OpenNLP.NET#14)

Here you can find official docs
https://docs.microsoft.com/en-us/dotnet/core/versions/selection#framework-dependent-apps-roll-forward

We already use this feature in other tools for the same scenario. Example https://github.com/fsprojects/FsLexYacc/blob/master/src/FsLex/fslex.fsproj#L5-L6

@wasabii
Copy link
Contributor

wasabii commented Jun 6, 2022

I don't think the tools drops are framework dependent, though.

@wasabii
Copy link
Contributor

wasabii commented Jun 6, 2022

I was wrong. I had intended to make them self-contained, but they were not.

So anything wrong with them being self contained?

@NightOwl888
Copy link
Contributor

What about the ikvmc build makes you think that it is framework-dependent?

According to these docs when excluding the RID or passing --self-contained false, the executable will be framework-dependent. However, our publish process is explicitly including an RID, which I think makes it a self-contained executable. According to this, <SelfContained> is a (apparently undocumented) property, but we are not setting it. On the other hand, the dotnet publish command docs say you must explicitly include --self-contained or --no-self-contained when using RID.

But the ikvmc.runtimeconfig.json file is telling - it lists .NET Core 3.1 under includedFrameworks:

{
  "runtimeOptions": {
    "tfm": "netcoreapp3.1",
    "includedFrameworks": [
      {
        "name": "Microsoft.NETCore.App",
        "version": "3.1.0"
      }
    ],
    "configProperties": {
      "System.Reflection.Metadata.MetadataUpdater.IsSupported": false
    }
  }
}

I don't think that the tradeoff of having a smaller deployment size is worth the hassle of having to require a .NET SDK as a prerequisite to run ikvmc, so self-contained seems like the right choice (at least until #64 is a thing).

And since the target framework for the output of ikvmc is based off of the reference assembly location, it doesn't really matter if it is running on .NET Core 3.1 since we should be able to produce net6.0 targeted assemblies with it, right? Even if that isn't the case, having an output assembly that targets netcoreapp3.1 should be able to run on the .NET 6.0 runtime.

BTW - it looks like there is a way to trim the deployment size, but we probably need to manually account for any dependencies are dynamically added through IL writing or Reflection. Not to mention, we would have to ensure all of the dependencies for all of the tools are included, since there is only 1 deployment folder for all tools.

@wasabii
Copy link
Contributor

wasabii commented Jun 6, 2022

@NightOwl888 I was wrong again. You are correct. Merely by setting a RuntimeIdentifier, they are independent.

So the question remains then.... what's wrong with them now? They're independent currently, so rolling forward doesn't accomplish much.

@AaronRobinsonMSFT
Copy link

So anything wrong with them being self contained?

Not technically but I'd argue that self-contained is something that should be considered carefully. Personally having a roll-forward policy will handle the vast majority of scenarios. Using self-contained to avoid having .NET installed is advantageous but if .NET is installed, why is ikvmc being used? A counter to all of this is breaking changes across major version are permissible but rare. Being self-contained does handle this issue.

we probably need to manually account for any dependencies are dynamically added through IL writing or Reflection. Not to mention, we would have to ensure all of the dependencies for all of the tools are included, since there is only 1 deployment folder for all tools.

Trimming is very hard to get right and has taken months of work to make the runtime Trimmer friendly. I've not audited all of ikvm, but I wouldn't be surprised it would be a lot of work to make it compatible.

@wasabii
Copy link
Contributor

wasabii commented Jun 6, 2022

Yeah. Well, the goal of the tools drops is simply to get something runnable in somebody's hands as soon as possible. So they can do it from the command line, with minimal build system. Basically I have no idea what they're doing. I'd even make them SingleFile if the native stuff wasn't in the way.

Ideally, down the road, use of these would be rare. We'd have MSBuild tasks for automating the builds. And the <JavaReference> idea. And a NuGet global tools package.

For now though, these tools drops are just the lowest common denominator to get something in people's hands so they can try it.

Also, I don't think I want to be debugging issues with running them on later frameworks, even though they were built for older ones. I don't think that even handles up-tfm targets of dependent packages. For instance, say we depend on a NuGet package that has both netcoreapp3.1, as well as net5.0 targets. We distribute a framework dependent build for netcoreapp3.1. So the DLL from that package for netcoreapp3.1 is sitting there. If they run it on net5.0, it won't be using the net5.0 version of that library. Will that cause issues? I have no idea.

@wasabii
Copy link
Contributor

wasabii commented Jun 6, 2022

And yeah, Trimming is going to be insane. We'd have to account for all of the reflection going on inside OpenJDK. Maybe someday.

@AaronRobinsonMSFT
Copy link

If they run it on net5.0, it won't be using the net5.0 version of that library. Will that cause issues? I have no idea.

Nope. Libraries target a TFM not a runtime. The TFM is just the API surface area so if the APIs exist it will just work. This does relate to the breaking change issues mentioned above but those are very rare and unlikely to be an issue to optimize for.

Well, the goal of the tools drops is simply to get something runnable in somebody's hands as soon as possible. So they can do it from the command line, with minimal build system.

Doesn't minimal build system in this case mean the .NET SDK?

@wasabii
Copy link
Contributor

wasabii commented Jun 6, 2022

Well, I think something was lost in translation there. Imagine we depend on SharpZipLib. Imagine they distribute two versions of their assembly in their NuGet package, one in lib/netcoreapp3.1 and another in lib/net5.0.

We build a netcoreapp3.1 version of our project. That build ends up including netcoreapp3.1 version of SharpZipLib. User runs our code on NET5. NET5 is now executing the netcoreapp3.1 version of SharpZipLib. Will this be a problem? Probably not: but ultimately it depends on what the SharpZipLib author did.

@NightOwl888
Copy link
Contributor

Gotcha. Trimming definitely sounds like more trouble than it is worth, especially if these tools will later be the rare usage path.

ikvmc would be used both on developer machines and in CI pipelines, similar to how dotnet build is used (at least until we have an ikvm build dotnet tool). So, I guess it boils down to whether we try to minimize deployment size or number of prerequisites.

The .NET Core 3.1 End of Support is coming in December, 2022, so it seems like for the tools we should move the target to .NET 6.0 to keep up with security patches, etc. But for the class libraries, it sounds like a lot more work to deal with to upgrade and IMO, as long as netcoreapp3.1 targeted assemblies can still be consumed, we should probably not spend the time on such an upgrade.

However, we could still support the latest security patches if we continued .NET Core 3.1 on the tools, made the deployment framework dependent, and included roll forward. With the right roll forward strategy, this would also keep them up to date when .NET 6.0 reaches end of support.

We still need to specify RID for the native bits, but as I recall there is a way that can be done with a framework-dependent deployment just by putting them in the right folders.

Since it is highly likely every dev machine and most CI environments will have either a .NET Runtime or SDK installed (or can easily happen in CI with a task), and roll forward sounds like something we need to keep the runtime up to date without changing the target from netstandard3.1, it seems like framework-dependent is probably the best way to go for the long-term.

@wasabii
Copy link
Contributor

wasabii commented Jun 6, 2022

Doesn't minimal build system in this case mean the .NET SDK?

I mean, PROBABLY. I'm not going to fight this one.

@wasabii
Copy link
Contributor

wasabii commented Jun 6, 2022

Gotcha. Trimming definitely sounds like more trouble than it is worth, especially if these tools will later be the rare usage path.

For now. One of the downsides of me merging IKVM.Java was it's size. I mean it's not terrible by today's standards, but it's not great for constrained environments.

That said, JDK9+, when we get to it, comes with a reevaluation of the breakdown of the Java libraries, into Modules, giving us an initial easy win on that front. It's not trimming, but it will help.

I do think we should replace the netcore3.1 packages with net6.0 packages. I started with netcoreapp3.1 because it was what the project was built with in mind when I came to it, and without tests, or any other sort of validation, it wasn't a problem I wanted to tackle out of the gate.

That said, everything appears to work perfectly well on .NET 6 now. Or at least, just as well as on 3.1. So maybe that's worth reassessing for the tools drops.

@NightOwl888
Copy link
Contributor

Well, I think something was lost in translation there. Imagine we depend on SharpZipLib. Imagine they distribute two versions of their assembly in their NuGet package, one in lib/netcoreapp3.1 and another in lib/net5.0.

We build a netcoreapp3.1 version of our project. That build ends up including netcoreapp3.1 version of SharpZipLib. User runs our code on NET5. NET5 is now executing the netcoreapp3.1 version of SharpZipLib. Will this be a problem? Probably not: but ultimately it depends on what the SharpZipLib author did.

This will work a little differently depending on whether the actual netcoreapp3.1 targeted assembly is shipped or a <PackageReference> is used, although both of them should work.

In the former case, netcoreapp3.1 targeted assembly should work since netcoreapp3.1 only defines the API surface area, but if the consuming executable targets net6.0 it will load on (at least) the .NET 6.0 runtime. But if there is a .net6.0 targeted version of SharpZipLib, it is probably not taking advantage of all of the performance optimizations.

If <PackageReference> is used, then it will go through the restore dependency logic to do a "smart" selection of the TFM based on the current TFM. Only this case really "depends on what the author did" and will ship any additional API surface and conditional compilation sections that are optimized for net6.0. This would be the ideal way to ship the right assembly for the selected TFM (or at least re-use the logic of how it resolves package dependencies).

I do think we should replace the netcore3.1 packages with net6.0 packages. I started with netcoreapp3.1 because it was what the project was built with in mind when I came to it, and without tests, or any other sort of validation, it wasn't a problem I wanted to tackle out of the gate.

That said, everything appears to work perfectly well on .NET 6 now. Or at least, just as well as on 3.1. So maybe that's worth reassessing for the tools drops.

Alright, I was under the impression that it would involve more work. It makes more sense if this is somewhat easy to change. Of course, we need to be careful with the above dependency resolution logic so we can still target lower TFMs than net6.0 (since it is common for class libraries to target the lowest common API).

@wasabii
Copy link
Contributor

wasabii commented Jun 6, 2022

This will work a little differently depending on whether the actual netcoreapp3.1 targeted assembly is shipped or a is used, although both of them should work.

Since we're talking about the tools drop, which is a Publish, they will be shipped.

Of course, we need to be careful with the above dependency resolution logic so we can still target lower TFMs than net6.0

Still only talking about the tools drop.

@NightOwl888
Copy link
Contributor

This will work a little differently depending on whether the actual netcoreapp3.1 targeted assembly is shipped or a is used, although both of them should work.

Since we're talking about the tools drop, which is a Publish, they will be shipped.

Of course, we need to be careful with the above dependency resolution logic so we can still target lower TFMs than net6.0

Still only talking about the tools drop.

Gotcha. The dependency resolution from NuGet is documented here. The <PackageReference> dependency resolution logic is codified in NuGet.DependencyResolver.Core. This seems like the best long-term option, but probably doesn't belong in ikvmc if the plan is to keep it dumb and not know anything about TFMs.

Failing that option, it sounds like the best case scenario is to ship the lowest common TFM in the drop netcoreapp3.1 or higher. I don't think we need to go lower than that.

@wasabii
Copy link
Contributor

wasabii commented Jun 6, 2022

Sorry, I am confused. Are you talking about the tools drop or something else? I really don't know what ikvmc the tool has to do with this. Or what PackageReference has to do with it.

@NightOwl888
Copy link
Contributor

NightOwl888 commented Jun 6, 2022

Sorry, I am confused. Are you talking about the tools drop or something else?

Both.

You were asking about how the drop was supposed to figure out which TFM to use for platform libraries and 3rd party packages (such as SharpZipLib). Ideally for the long term we would use NuGet.DependencyResolver.Core to determine which assembly to include, then run ikvmc and pass the resolved assembly as dependency (specified with -r).

But until we have a higher-level tool that calls ikvmc, it seems that it would be best to simply deploy the netcoreapp3.1 assemblies (even if the tools actually target net6.0) and include them by default when running ikvmc.

Since you were talking about removing the -nostdlib option, perhaps for the time being we could use it as a signal. If it is specified (or defaulted) use the dependencies from the drop. If not, they would need to be specified with -r or -lib. Then when the -nostdlib option is removed, specifying these dependent assemblies on the command line will be a requirement and we would no longer include a specific TFM of them in the drop. But at that point we should have a higher-level tool to handle those details about TFM. Does that sound reasonable?

@wasabii
Copy link
Contributor

wasabii commented Jun 6, 2022

You were asking about how the drop was supposed to figure out which TFM to use for platform libraries and 3rd party packages (such as SharpZipLib).

It doesn't figure it out. The TFM you publish it as is what it uses. This isn't our choice. This is how it works. The SharpZipLib.dll file sits in a folder next to ikvmc.exe, and that's the end of it.

Higher level tools can't fix this. That's how .NET works.

Nothing about -stdlib effects how SharpZipLib is located in my example. You are confusing the published executable vs what the executable does in converting DLL files. These are not related.

Does that sound reasonable?

It sounds confused. Like, I really think you are not correctly understanding what the tools drop is, and what the TFM for that drop is about.

@NightOwl888
Copy link
Contributor

You were asking about how the drop was supposed to figure out which TFM to use for platform libraries and 3rd party packages (such as SharpZipLib).

It doesn't figure it out. The TFM you publish it as is what it uses. This isn't our choice. This is how it works. The SharpZipLib.dll file sits in a folder next to ikvmc.exe, and that's the end of it.

Higher level tools can't fix this. That's how .NET works.

Nothing about -stdlib effects how SharpZipLib is located in my example. You are confusing the published executable vs what the executable does in converting DLL files. These are not related.

Does that sound reasonable?

It sounds confused. Like, I really think you are not correctly understanding what the tools drop is, and what the TFM for that drop is about.

Ahh, okay. I was confused. I thought that the ikvmc dependencies were somehow inherited by the output assemblies. I guess they are when you consider that the IKVM NuGet package depends on them, but in that case the TFM is already resolving correctly.

@wasabii
Copy link
Contributor

wasabii commented Jun 6, 2022

Yeah, like you're talking about stuff inheriting TFMs.... but assemblies don't bind by TFMs. They bind by AssemblyName, and on Framework including Version and PublicKey with the option for runtime redirects.

@wasabii
Copy link
Contributor

wasabii commented Jul 6, 2022

So I'm going to close this one for now. We don't distribute non-NetCore3.1 framework-independent versions of the tools. And, in my current experience, running the tools on a later environment doesn't quite work right yet. They're going to need some work before I feel comfortable saying they're supported on later runtimes.

@wasabii wasabii closed this Jul 6, 2022
@sergey-tihon
Copy link
Author

And, in my current experience, running the tools on a later environment doesn't quite work right yet. They're going to need some work before I feel comfortable saying they're supported on later runtimes.

It is sad. @wasabii can you please leave a little bit more details about issues you faced running the tools on a later environment?

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.

4 participants