Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Make UnionPath behave as paths from ZipFileSystem do.#26

Open
noeppi-noeppi wants to merge 8 commits into
McModLauncher:main from
noeppi-noeppi:pathfix
Open

Make UnionPath behave as paths from ZipFileSystem do. #26
noeppi-noeppi wants to merge 8 commits into
McModLauncher:main from
noeppi-noeppi:pathfix

Conversation

@noeppi-noeppi

@noeppi-noeppi noeppi-noeppi commented Mar 15, 2022

Copy link
Copy Markdown
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.

Su5eD reacted with thumbs up emoji

Copy link
Copy Markdown
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/cpw.mods.modlauncher@9.1.3/cpw.mods.modlauncher.util.ServiceLoaderUtils.lambda$fileNameFor2ドル(ServiceLoaderUtils.java:50)
	at java.base/java.util.Optional.map(Optional.java:260)
	at MC-BOOTSTRAP/cpw.mods.modlauncher@9.1.3/cpw.mods.modlauncher.util.ServiceLoaderUtils.fileNameFor(ServiceLoaderUtils.java:50)
	at MC-BOOTSTRAP/cpw.mods.modlauncher@9.1.3/cpw.mods.modlauncher.LaunchPluginHandler.lambda$new2ドル(LaunchPluginHandler.java:50)
	at java.base/java.util.stream.ReferencePipeline3ドル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/cpw.mods.modlauncher@9.1.3/cpw.mods.modlauncher.LaunchPluginHandler.<init>(LaunchPluginHandler.java:51)
	at MC-BOOTSTRAP/cpw.mods.modlauncher@9.1.3/cpw.mods.modlauncher.Launcher.<init>(Launcher.java:64)
	at MC-BOOTSTRAP/cpw.mods.modlauncher@9.1.3/cpw.mods.modlauncher.Launcher.main(Launcher.java:77)
	at MC-BOOTSTRAP/cpw.mods.modlauncher@9.1.3/cpw.mods.modlauncher.BootstrapLaunchConsumer.accept(BootstrapLaunchConsumer.java:26)
	at MC-BOOTSTRAP/cpw.mods.modlauncher@9.1.3/cpw.mods.modlauncher.BootstrapLaunchConsumer.accept(BootstrapLaunchConsumer.java:23)
	at cpw.mods.bootstraplauncher@1.0.0/cpw.mods.bootstraplauncher.BootstrapLauncher.main(BootstrapLauncher.java:149)

Copy link
Copy Markdown
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 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

Copy link
Copy Markdown
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 commented Jul 6, 2022

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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 commented Jun 14, 2023

Copy link
Copy Markdown
Member

@marchermans is this still relevant??

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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 commented May 31, 2024

Copy link
Copy Markdown

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

Reviewers

No reviews

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

Milestone

MC 1.19

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /