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

Make UnionPath behave as paths from ZipFileSystem do. #26

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

noeppi-noeppi
Copy link
Contributor

This changes the behaviour of union paths to match the behaviour from zip paths. To ensure this, all path tests are first run against a zip file system and it is asserted that they don't fail.

This will most likely break modlauncher and forge

Also the tests for the relativize method were moved from TestUnionFS to TestUnionPath.

@noeppi-noeppi
Copy link
Contributor Author

I did some fixes to the Jar class so it works with the changes from this pull request now. There may be more things in securejarhandler that break with these changes though. Trying to run forge (in a forgedev environment) with this now results in the following exception:

Exception in thread "main" java.lang.NullPointerException: Cannot invoke "java.nio.file.Path.toString()" because the return value of "java.nio.file.Path.getFileName()" is null
	at MC-BOOTSTRAP/[email protected]/cpw.mods.modlauncher.util.ServiceLoaderUtils.lambda$fileNameFor$2(ServiceLoaderUtils.java:50)
	at java.base/java.util.Optional.map(Optional.java:260)
	at MC-BOOTSTRAP/[email protected]/cpw.mods.modlauncher.util.ServiceLoaderUtils.fileNameFor(ServiceLoaderUtils.java:50)
	at MC-BOOTSTRAP/[email protected]/cpw.mods.modlauncher.LaunchPluginHandler.lambda$new$2(LaunchPluginHandler.java:50)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.HashMap$EntrySpliterator.forEachRemaining(HashMap.java:1850)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:575)
	at java.base/java.util.stream.AbstractPipeline.evaluateToArrayNode(AbstractPipeline.java:260)
	at java.base/java.util.stream.ReferencePipeline.toArray(ReferencePipeline.java:616)
	at java.base/java.util.stream.ReferencePipeline.toArray(ReferencePipeline.java:622)
	at java.base/java.util.stream.ReferencePipeline.toList(ReferencePipeline.java:627)
	at MC-BOOTSTRAP/[email protected]/cpw.mods.modlauncher.LaunchPluginHandler.<init>(LaunchPluginHandler.java:51)
	at MC-BOOTSTRAP/[email protected]/cpw.mods.modlauncher.Launcher.<init>(Launcher.java:64)
	at MC-BOOTSTRAP/[email protected]/cpw.mods.modlauncher.Launcher.main(Launcher.java:77)
	at MC-BOOTSTRAP/[email protected]/cpw.mods.modlauncher.BootstrapLaunchConsumer.accept(BootstrapLaunchConsumer.java:26)
	at MC-BOOTSTRAP/[email protected]/cpw.mods.modlauncher.BootstrapLaunchConsumer.accept(BootstrapLaunchConsumer.java:23)
	at [email protected]/cpw.mods.bootstraplauncher.BootstrapLauncher.main(BootstrapLauncher.java:149)

@SizableShrimp
Copy link
Contributor

The specific crash provided uses an optional chain that defaults to the string "MISSING FILE". It would be good to investigate what the change from "" empty string to "MISSING FILE" would look like for consumers of ServiceLoadersUtils#fileNameFor. And if a null path should count as empty string still or "MISSING FILE".

@marchermans marchermans self-assigned this Mar 21, 2022
@marchermans marchermans added bug Something isn't working for next mc release Indicates issues or pull requests which can only be merged or closed in the next MC release window. labels Mar 21, 2022
@marchermans marchermans added this to the MC 1.19 milestone Mar 21, 2022
@marchermans marchermans marked this pull request as draft March 22, 2022 07:09
@marchermans marchermans marked this pull request as ready for review April 11, 2022 14:25
@marchermans
Copy link
Member

Testing procedure is setup and completed.
Last call for this PR before I merge it in a couple of days.

The additional endpoint in #20 mentioned to get the proper paths of a mod needs to be added later.

@cpw
Copy link
Member

cpw commented Jul 6, 2022

I suspect that the performance tweaks have completely invalidated this PR. Can you re-evaluate if this is still appropriate?

 into pathfix

� Conflicts:
�	src/main/java/cpw/mods/niofs/union/UnionFileSystem.java
�	src/main/java/cpw/mods/niofs/union/UnionPath.java
�	src/test/java/cpw/mods/niofs/union/TestUnionFS.java
@noeppi-noeppi
Copy link
Contributor Author

I just merged the tests from this PR (which assert that UnionPath behaves the same as JarPath) with the performance tweaks and some of them still fail. So yes, it is still appropriate, ill update it to resolve the conflicts.

@cpw
Copy link
Member

cpw commented Jun 14, 2023

@marchermans is this still relevant??

@marchermans
Copy link
Member

Not that I know of
@noeppi-noeppi ? You ran those tests before.

@noeppi-noeppi
Copy link
Contributor Author

If it is still relevant to mimic the behaviour of ZipFileSystem, then yes. The UnionPath still behaves different than ZipPath. So if Lex statement here is still relevant, so is this PR.

# Conflicts:
#	src/main/java/cpw/mods/jarhandling/impl/Jar.java
#	src/main/java/cpw/mods/niofs/union/UnionFileSystem.java
@MelanX
Copy link

MelanX commented May 31, 2024

This PR is included in the 1.19 milestone. I guess that means it’ll be merged right before 1.19, right?
In just two weeks, 1.21 releases. I think, it’s a bit late.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working for next mc release Indicates issues or pull requests which can only be merged or closed in the next MC release window.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants