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

Remove Lumina Pinning #1598

Closed
KazWolfe opened this issue Jan 3, 2024 · 3 comments
Closed

Remove Lumina Pinning #1598

KazWolfe opened this issue Jan 3, 2024 · 3 comments
Labels
core Dalamud core dependencies Pull requests that update a dependency file needs api bump Held for the next API bump

Comments

@KazWolfe
Copy link
Member

KazWolfe commented Jan 3, 2024

Currently, Dalamud blocks plugins from loading in their own version of Lumina.

This behavior has caused problems for a couple plugins that needed to fork or modify things against test or tweaked versions of Lumina, and has requested to be removed. At present this feature is slated for removal in API 10, with this Issue serving as a reminder that we need to give it consideration and actually do it.

@KazWolfe KazWolfe added core Dalamud core needs api bump Held for the next API bump dependencies Pull requests that update a dependency file labels Jan 3, 2024
@KazWolfe
Copy link
Member Author

KazWolfe commented Jan 3, 2024

As a personal note, I'm still not particularly in support of doing this as this feels like a massive footgun just waiting to happen.

While it is strange for us to pin Lumina (and only Lumina) like this, raw Lumina types are also part of our exposed API surface in many places. Any plugin bringing their own version of Lumina will have headaches with IDataManager and various properties scattered all over our public API surface. Plugins shipping their own Lumina will not be able to use any of these API surfaces, and it's not particularly clear at a glance what would or wouldn't be broken especially to a novice developer.

Given that we have had plugins accidentally forget <Private>false</Private> in their csproj files before or were otherwise shipping Lumina (sometimes intentionally?), it does seem like problems related to this are actually possible.

If we decide that we do want to go forward with this, I'd like to request that this be heavily documented somewhere easily accessible to developers so that everyone is well aware of this choice and how to resolve this problem if one inadvertently ships their own Lumina. I'm not sure I'd go as far as adding a manifest flag for AllowCustomLumina, but I'm not strictly opposed to that either...

@KazWolfe
Copy link
Member Author

KazWolfe commented Apr 21, 2024

Alright, upon further consideration here. Seeing that API 10 is just around the corner, I think this would be a good thing to bring in.

Let's define this such that we will allow loading a custom Lumina if a manifest field is set to a truthy value. We can then create documentation around this to indicate that multiple/many APIs provided by Dalamud will no longer function properly. We can also likely add a warning to our local plugin system indicating that this is enabled.

I think this satisfies any concerns about API safety while still allowing developers control over their dependencies. So long as the behavior is known and explicitly opt-in, I can't see many problems arising from this.


For the manifest field, I think it'd make sense to define a new K/V field:

...
FeatureGates:
  AllowCustomCoreDependencies: true
  SomeOtherFeatureGate: true
  AnotherThing: true
...

We can expand on this to add extra features later on (e.g. the ability to opt in to [Experimental] attributes on Plogon builds).

If anyone wants to PR this to the API 10 branch, let me know and I'll assign you to this Issue.

@KazWolfe
Copy link
Member Author

Closing until such time as this becomes necessary again and someone wants to work on it.

@KazWolfe KazWolfe closed this as not planned Won't fix, can't repro, duplicate, stale Jun 29, 2024
KazWolfe added a commit that referenced this issue Jun 29, 2024
See #1598 for requirements to do this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Dalamud core dependencies Pull requests that update a dependency file needs api bump Held for the next API bump
Projects
None yet
Development

No branches or pull requests

1 participant