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

ScintillaNET dependency breaks bootstrapper update #1932

Closed
glopesdev opened this issue Jul 22, 2024 · 3 comments · Fixed by #1907
Closed

ScintillaNET dependency breaks bootstrapper update #1932

glopesdev opened this issue Jul 22, 2024 · 3 comments · Fixed by #1907
Labels
bug Something isn't working critical This is a blocking issue affecting all users
Milestone

Comments

@glopesdev
Copy link
Member

glopesdev commented Jul 22, 2024

The move to .NET core ScintillaNET in #1924 breaks bootstrapper update on environments which already contain the legacy jacobslusser.ScintillaNET package. The reason is that the new bootstrapper requires fernandreu.ScintillaNET but this new package is not a direct update of jacobslusser.ScintillaNET which means there is no reason to uninstall the previous version, and this ends up causing a duplicate registration of the ScintillaNET assembly for resolution.

The only way to implement something resembling this kind of redirection seems to be something along the lines of what is described in dotnet/aspnetcore#1222 (comment). Basically we would have to generate a "dummy" release (or pre-release) of jacobslusser.ScintillaNET which is simply an empty package pointing to fernandreu.ScintillaNET as a dependency, but which itself registers no assemblies. This would cause uninstallation of the old package and remove the duplicate assembly registration.

We would have to publish this package as a dependency on the MyGet feed since we do not have keys to publish it on NuGet.org. This is likely not a problem since the jacobslusser.ScintillaNET project has been stale for 6 years and the GitHub public repo has now been archived.

The only other options would be to either somehow make this replacement explicit at the level of the bootstrapper which I am not favoring currently since it would be really hard to maintain and definitely not scalable, or make fernandreu.ScintillaNET a conditional dependency for .NET core (and keep jacobslusser.ScintillaNET for .NET Framework).

The latter option might not be free of issues itself, especially if dependent projects do not get updated to support .NET core. Fortunately, this specific dependency is not so widespread as to cause too many issues.

@glopesdev glopesdev added bug Something isn't working critical This is a blocking issue affecting all users labels Jul 22, 2024
@glopesdev glopesdev added this to the 2.9 milestone Jul 22, 2024
@PathogenDavid
Copy link
Member

The only other options would be to either somehow make this replacement explicit at the level of the bootstrapper which I am not favoring currently since it would be really hard to maintain and definitely not scalable

Would it be? It seems way simpler to me to hide a little edge case fixup somewhere that ignores jacobslusser.ScintillaNET when fernandreu.ScintillaNET is present.

I don't think scalability matters here. This doesn't have to be reusable and generic, it just has to handle this one weird edge case.

The dummy MyGet package seems way more gross and hacky to me.


Thinking about using the new package only for modern .NET (which seems like an OK but not ideal solution) made me realize we have a bit of a ticking time bomb here though. We're going to encounter similar issues with Rx-Core/etc once we migrate to modern .NET too. (People will presumably expect their Bonsai.config to either continue working or be upgradable without too much drama.)

It's unfortunate that we don't really have a way to disambiguate between manually installed packages and transitive dependencies in Bonsai.config because that'd make this situation a lot simpler to handle. (Unused transitive dependencies would just get dropped.)

I wonder how much drama it'd cause to migrate towards something closer to PackageReference (maybe with an optional lock file) like modern NuGet? (I'd assume for Bonsai 3.0, that's a pretty fundamental change for 2.9.)

Alternatively maybe this is all the more reason to just stomach the hack since Rx-Core and ScintillaNET are hopefully the only bad citizens we'll have to worry about 😅 (I like to think most packages follow the common practice of package name == assembly name and we just got unlucky that Bonsai depended on two who bucked the trend.)

@glopesdev
Copy link
Member Author

Thinking about using the new package only for modern .NET (which seems like an OK but not ideal solution) made me realize we have a bit of a ticking time bomb here though. We're going to encounter similar issues with Rx-Core/etc once we migrate to modern .NET too.

Indeed, Rx-Core is even worse, they actually have bridge packages for this purpose, but because they broke strong-name references following v3.0 and the move to .NET foundation, this doesn't even work for us, we just have to assume everything is fully broken (this is also why I made package references for System.Reactive on Bonsai.Core conditional on .NET Framework vs .NET Core, as it provides for a natural bisection in the dependency graph).

I wonder how much drama it'd cause to migrate towards something closer to PackageReference (maybe with an optional lock file) like modern NuGet?

This would make a lot of sense, and yes we would need to save this for 3.0. We could also then properly address #895 and #1572.

Alternatively maybe this is all the more reason to just stomach the hack since Rx-Core and ScintillaNET are hopefully the only bad citizens we'll have to worry about

I've been burned in the past by being similarly hopeful, and I feel in fact there will be more of these dependencies, for example with OpenTK, etc. This was part of the reason why I was aiming to fully target .NET core for the editor only in 3.0.

I think for now the solution of making package reference conditional on .NET Framework vs .NET Core is likely the way to go, since nothing really significant changed on ScintillaNET that is worth making us worry about this.

I'll make another PR to fix this before release, depending on whether or not we can make advances towards a .NET Core compatible editor in this release. My feeling is that we probably don't want to make any installer or portable zip for this release that targets .NET Core, but if the source was made compatible it could be a good testing ground to start upgrading and testing other packages, so we can start migrating dependencies to .NET Core before transitioning the entire editor.

@PathogenDavid
Copy link
Member

PathogenDavid commented Jul 23, 2024

It's definitely a mess :)

Conditional reference etc sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working critical This is a blocking issue affecting all users
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants