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 "Java CD" workflow responsible for automated publishing #19

Merged
merged 4 commits into from
Apr 5, 2024
Merged

Fix "Java CD" workflow responsible for automated publishing #19

merged 4 commits into from
Apr 5, 2024

Conversation

Kir-Antipov
Copy link
Contributor

This PR fixes (or at least it should fix) your publishing workflow.

A dry run of mc-publish results in the following output:

CurseForge ID: 930207
Modrinth ID: KuNKN7d2
Primary file type: fabric, quilt, forge, neoforge
Publishing: build/libs/noisium-merged-2.0.0+mc1.20.x.jar
Publishing: common/build/libs/noisium-common-2.0.0+mc1.20.x-sources.jar
Publishing: fabric/build/libs/noisium-fabric-2.0.0+mc1.20.x-dev-shadow.jar
Publishing: fabric/build/libs/noisium-fabric-2.0.0+mc1.20.x-sources.jar
Publishing: forge/build/libs/noisium-forge-2.0.0+mc1.20.x-dev-shadow.jar
Publishing: forge/build/libs/noisium-forge-2.0.0+mc1.20.x-sources.jar

As you can see, your mod's metadata is read correctly, and all the necessary jars are selected for publication in the expected order (i.e., noisium-merged-2.0.0+mc1.20.x.jar is the primary file, and the others are secondary).

Once again, note that mc-publish only reads one metadata file per jar. This decision was made under the assumption, which was reasonable at the time, that nobody would merge mods for different mod loaders into a single binary. Thus, given that my beloved Fabric has a completely unbiased higher priority internally, only fabric.mod.json is read. So, feel free to clean up mods.toml from duplicate entries if you feel like it :)


See Kir-Antipov/mc-publish#114.

@Steveplays28 Steveplays28 added the bug Something isn't working label Apr 5, 2024
@Steveplays28
Copy link
Owner

Thank you so much! I'll merge it in and try it out!

@Steveplays28 Steveplays28 merged commit 6269912 into Steveplays28:1.20.x Apr 5, 2024
@Steveplays28
Copy link
Owner

Ah yeah, I keep running into an issue where the CurseForge ID isn't found, see the latest publish workflow run
Not sure what's up with that

@Kir-Antipov
Copy link
Contributor Author

First of all, I really need to add a proper debug mode to mc-publish. People who suggested this like 2 years ago are probably laughing and crying right now.

Secondly, I believe ./gradlew publish is causing issues here. I tested the workflow without it, and as you can see, everything works fine. It appears to either add unwanted files to build/libs/ or overwrite ones we actually need.

@Steveplays28
Copy link
Owner

Steveplays28 commented Apr 5, 2024

Ah ok, thanks. That sounds likely
I'll move it to the end of the workflow, so I don't have to delete all the artifacts if mc-publish returns an error. Also saves us time, we won't have to fix the Glob again xD

@Steveplays28 Steveplays28 added the ci/cd Continous integration/deployment label Apr 5, 2024
@Steveplays28
Copy link
Owner

Let's go, that worked!
Thanks a bunch.

I'm writing up my thoughts on merged JARs in the issue I opened in mc-publish's repository.

@Kir-Antipov
Copy link
Contributor Author

Yay! It somewhat worked, finally :D

By the way, that very poorly worded error you received from GitHub is there because of:

    permissions:
      contents: read

mc-publish doesn't have permissions to create or modify releases in your repository. To fix it, you need:

    permissions:
      contents: write

Sorry for overlooking this when I made the PR!

@Steveplays28
Copy link
Owner

Ahh, that's all good. I'll add that in and make a few extra workflows so I can publish to specific platforms separately from each other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ci/cd Continous integration/deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants