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

fix: make JarNester deterministic #1197

Merged
merged 2 commits into from
Nov 15, 2024

Conversation

ashhhleyyy
Copy link
Contributor

Sorts the list of jars to nest before adding them to fabric.mod.json to ensure the ordering is deterministic.

Sorts the list of jars to nest before adding them to fabric.mod.json to ensure the ordering is deterministic.
@solonovamax
Copy link
Contributor

solonovamax commented Oct 20, 2024

sorting it only if the isReproducibleFileOrder may be a desired solution as it avoides sorting when determinism isn't desired
from the fabric discord:

Everything should be reproducible by default, I think this one would be a regression

also, the sort order can be different on windows vs unix, as-per the javadocs for File.compareTo:

Compares two abstract pathnames lexicographically. The ordering defined by this method depends upon the underlying system. On UNIX systems, alphabetic case is significant in comparing pathnames; on Microsoft Windows systems it is not.

it may be desireable to .toLowerCase() the pathnames first before sorting (also possibly to ensure the input files are absolute and/or only sort using the base file name?)

@ashhhleyyy
Copy link
Contributor Author

I think just using the filename would be okay as using the absolute path makes it dependent on the build environment, and only the filename makes it into the jar. I don't think it's a good idea to lower-case them first, since then two files with different cases will get sorted the same, and the ordering between the two becomes platform dependent again

@solonovamax
Copy link
Contributor

I think just using the filename would be okay as using the absolute path makes it dependent on the build environment, and only the filename makes it into the jar. I don't think it's a good idea to lower-case them first, since then two files with different cases will get sorted the same, and the ordering between the two becomes platform dependent again

if you don't want to lowercase them, then make sure to sort using the String representation of the file/filename rather than the File representation

@ashhhleyyy
Copy link
Contributor Author

yeah, i meant to push the change that went along with that but forgor, will push when i get home

Copy link
Contributor

@solonovamax solonovamax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@modmuss50 modmuss50 added this to the 1.9 milestone Nov 1, 2024
@modmuss50 modmuss50 changed the base branch from dev/1.8 to exp/1.9 November 14, 2024 22:25
@modmuss50 modmuss50 merged commit 13ed992 into FabricMC:exp/1.9 Nov 15, 2024
89 checks passed
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.

4 participants