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

Add an easy way to include fabric api modules #183

Merged
merged 6 commits into from
May 27, 2020

Conversation

modmuss50
Copy link
Member

@modmuss50 modmuss50 commented Mar 14, 2020

modImplementation fabricApi.module("fabric-resource-loader-v0", project.fabric_version)

include fabricApi.module("fabric-resource-loader-v0", project.fabric_version)

Not sure if I like it, it does mean adding fabric api specific code. Atleast people can stop complaining that the hashes are broken.

Code may not be great, if people actaully want this it could be improved.

@modmuss50 modmuss50 changed the base branch from dev/0.2.7 to dev/0.4 May 14, 2020 00:25
@modmuss50
Copy link
Member Author

I have updated this PR, it has had minimal feedback, I think merging this is best. Its not ideal that its fabric api specific but I think its a lot better than having a whole plugin to do it.

@Sollace
Copy link

Sollace commented May 14, 2020

No. The correct way to fix this is to remove the hashes.

@shedaniel
Copy link
Contributor

As what I said in the past,

  • Remove hashes
  • Bump version for everything
  • Publish to maven only if the version doesn't exist on maven already

@natanfudge
Copy link
Contributor

You would also need to include the Minecraft version that the module version supports.

@modmuss50
Copy link
Member Author

No one has provided a solution to the issue the hashes solve.

@shedaniel
Copy link
Contributor

What does hashes solve?

@Sollace
Copy link

Sollace commented May 15, 2020

The hashes don't solve anything. They create more problems than they resolve.

You want a solution for not being able to identify a version? Allow me to introduce you to semver.

@modmuss50
Copy link
Member Author

Ive been over why they are needed about 10 times.

  • They prevent identical builds being uploaded more than once, such as using build numbers would do

  • They prevent overwriting existing versions accidentally

  • They are only metadata so should be ingored by version parser (I know gradle isnt perfect)

  • They work great across branches as well unlike build numbers.

And a myriad of reasons ive prob forgetten, yes they are annoying but thats what this PR sloves. Everytime this issue comes up no one provides any possible replacements, and doesnt say why they are bad.

@Sollace
Copy link

Sollace commented May 15, 2020

image

According to semver, a change that would render a module incompatible with a previous mc version constitutes a major version change.

I'll cut it half way and suggest this:

  • changes and updates within the came mc version, that don't break compatibility but add new features in a compatible way or fix bugs, can be the patch version
  • minor is incremented when moving to a new mc version (modders will generally know what the next number will be since it's always last_minor + 1. Updating to the immediate next mc version is straightforward. Skipping is less so, but also simply x -> x + n for the number of version jumps, assuming you know that. Otherwise just ask someone.)
  • major is incremented for every breaking api change.

@i509VCB
Copy link
Contributor

i509VCB commented May 15, 2020

Even if that were the solution, we would have to bring submodules into the question. Appended it with the fapi version? Bump every module version no matter what. What about 1.15 and 1.16, do we append 1.16 on the end of the module version?

Also the scripts don't write themselves. And until someone writes them, this is just going to end up in the bikeshedding closet.

@Sollace
Copy link

Sollace commented May 15, 2020

@modmuss50

and doesnt say why they are bad.

Here's one reason:

  • They are only metadata so are ingored by version parser
    Because of that, every time you upload a version differing by only the hash, you are actually publishing a new build *with the same version. Existing project configs that rely on that version then break when you publish incompatible builds under the same version.

Every time fabric updates all of its dependents are forced to update as well because you're retroactively destroying their dependency chain.

@modmuss50
Copy link
Member Author

This is just the way gradle handles it, 99% of the time the versions get updated, I think there has been 1 or 2 cases where I forgot.

Each sub module follows the semver spec (the hashes are part of the semver sepc).

@Sollace
Copy link

Sollace commented May 15, 2020

Even if that were the solution, we would have to bring submodules into the question. Appended it with the fapi version?
wtf no.

Fapi depends on the modules. It specified the version of the module that's included in them. Modules should not be indicating what fabapi version they're used in.

What about 1.15 and 1.16, do we append 1.16 on the end of the module version?

No. Mc version is implied by the major version, since you would be bumping that every time the mc version changes. You don't need to append it on the end a second time.

Bump every module version no matter what.

Bump every module version if that module needs to be changed. Or yes, bump the major whenever you do a new build against a new mc version the same way you would (should) bump the fabapi version every time one of its modules is updated. That's how versions are supposed to work.

@modmuss50
Copy link
Member Author

No one has provided a valid solution that sloves the issues the hashes do, untill someone makes a PR or atleast provides a valid solution its not going to change.

This PR will also help if that magicial solution comes into play as it would still require you to open the jar file to find the right module version for the fabric api module you want to include / depend on.

This fix isnt specfic to the hashes, it helps in all cases.

@Sollace
Copy link

Sollace commented May 15, 2020

This is just the way gradle handles it, 99% of the time the versions get updated, I think there has been 1 or 2 cases where I forgot.

Each sub module follows the semver spec (the hashes are part of the semver sepc).

have you looked at your maven? Everything is one version (or two).
image

No the hashes do not count.

You need to BUMP THE F**ING VERSION NUMBERS.

@FabricMC FabricMC locked as too heated and limited conversation to collaborators May 15, 2020
@modmuss50
Copy link
Member Author

Locked this PR as its got nothing to do with the PR and is getting out of hand.

I still plan to merge this as it has nothing to do specifically with the hashes.

@FabricMC FabricMC unlocked this conversation May 15, 2020
Copy link

@Prospector Prospector left a comment

Choose a reason for hiding this comment

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

While I worry a bit about adding fabric api-specific stuff to the other fabric projects, it would greatly improve things until we have a better solution

Copy link
Member

@Juuxel Juuxel left a comment

Choose a reason for hiding this comment

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

I think something like fabricApi.module() would make a more readable API, used like this:

implementation fabricApi.module("fabric-loot-tables-v1", project.fabric_version)

@Prospector
Copy link

^ actually I agree with juuz here, that would be nicer

@modmuss50
Copy link
Member Author

Yes, I also agree with @Juuxel I will merge and make that change

@TheGlitch76
Copy link

This does not solve the issue, this is a cheap workaround.

  • A lot of users don't need fabric api
  • Those that do just install the fat jar anyways so the modders don't need this change
  • Does not solve the issue for people not using loom/using hash system in projects that are not fabric api.

On a more serious note, is it possible to have this apply to projects also using the hash system but aren't fabric api?

@modmuss50
Copy link
Member Author

@TheGlitch76 thats the major issue with this, I dont see a way to make this none fabric api specfic easily.

@liach
Copy link
Contributor

liach commented May 22, 2020

Imo this is more like eclipse integration for idea users. This part is not coerced on users; it will only be used when they call fabricApi extension

@frqnny
Copy link

frqnny commented May 22, 2020

I guess this is useful but it is just a workaround until it becomes automatic in Loom.

# Conflicts:
#	src/main/java/net/fabricmc/loom/AbstractPlugin.java
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.

None yet

10 participants