Skip to content

Conversation

@shartte
Copy link
Contributor

@shartte shartte commented May 18, 2024

The use case for this is the use of SJH inside of FML unit tests where SJH itself was not loaded as a module.

It is perfectly capable of being used, but fails due to the union protocol handler not being loaded. This change makes SJH always load its own protocol handler regardless of layer and supports the extension point in non modular environments too.

As an aside: The extension point does not seem to be used by anyone else according to a GH search and can likely be deprecated/removed at some point.

@shartte shartte requested a review from Matyrobbrt May 18, 2024 21:54
@marchermans
Copy link
Member

I am okey with this PR.

Just a theoretical question: Why do we not split the UFS implementation into a seperate jar/sourceset, and then load this on the normal CP (like with jij fs, and others), then the union protocol would be there automatically.

I can not find any other use for this, other then to create url and path conversions..... So, is there something that speaks against this, that I am missing?

@shartte
Copy link
Contributor Author

shartte commented May 19, 2024

I am okey with this PR.

Just a theoretical question: Why do we not split the UFS implementation into a seperate jar/sourceset, and then load this on the normal CP (like with jij fs, and others), then the union protocol would be there automatically.

I can not find any other use for this, other then to create url and path conversions..... So, is there something that speaks against this, that I am missing?

I am not even 100% sure that SJH needs to be on the module path either. Looking at the FML tests, I doubt it has to be. But generally I'd postpone changing the architecture further. This PR just aims to make testing FML easier by making SJH less rigid, while trying not to change how our production works for now.

@shartte shartte merged commit f970393 into McModLauncher:main May 30, 2024
@shartte shartte deleted the union-url-non-modular branch May 30, 2024 17:37
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.

3 participants