Skip to content

Some helpful improvements #3

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

neoFuzz
Copy link

@neoFuzz neoFuzz commented Dec 1, 2024

Working on the idea that jitpack.io could be used to help out with working with dependencies. I created this PR with the following changes:

  1. Changed the workflow to generate the package files and upload them to the latest release. I believe this ties in with the build process downloading the package.
  2. Changed the groupId and bumped up the version. This is to avoid potential conflicts.
  3. Removed <scope>test</scope> from the pom.xml - not sure if this is helpful in anyway.
  4. Fixed some JavaDoc.

Other changes to the Forge project are required to use this new package. Things like updating actions and workflows to use a new package.

I made some changes to forge-gui-android/pom.xml which seems to work locally:

  1. Adding jitpack.io as a plugin repo.
    <pluginRepositories>
        <pluginRepository>
            <id>jitpack.io</id>
            <url>https://jitpack.io</url>
        </pluginRepository>
    </pluginRepositories>
  1. Changing the groupId fromcom.simpligility.maven.plugins to com.github.Card-Forge
  2. Bumping the version up to 4.6.3

This needs to be merged and working before jitpack.io will pick it up as a usable plugin.

… the latest release. Changed the `groupId` and bumped up the version. Removed `<scope>test</scope>` from the pom.xml. Fixed some JavaDoc.
@Hanmac
Copy link

Hanmac commented Dec 1, 2024

3. Removed <scope>test</scope> from the pom.xml - not sure if this is helpful in anyway.

The test scope is for this library

We don't need the mock dependency from it

@neoFuzz
Copy link
Author

neoFuzz commented Dec 1, 2024

My comment is broad. It should have removed the test phase bindings. If it actually did anything at all.

@Hanmac
Copy link

Hanmac commented Dec 1, 2024

My comment is broad. It should have removed the test phase bindings. If it actually did anything at all.

No I mean the test scope are needed for this dependencies

@neoFuzz
Copy link
Author

neoFuzz commented Dec 1, 2024

I think I get what you mean. Correct me if I'm wrong.

The dependencies' tests still run when compiling and packaging. It looks like this package needed to avoid tests since that blows up with all kind of errors with no real fix for those errors.

Running mvn -B clean install -DskipTests -Dmaven.test.skip=true -e should work without running into problems and generate the packages for use (and store them locally etc.)

@Hanmac
Copy link

Hanmac commented Dec 1, 2024

Yeah,using would say just keep the test scope for this MR

@neoFuzz
Copy link
Author

neoFuzz commented Dec 1, 2024

For completeness, roll back the changes removing <scope>test</scope> ?

@Hanmac
Copy link

Hanmac commented Dec 1, 2024

For completeness, roll back the changes removing <scope>test</scope> ?

Yeah I think so

@neoFuzz
Copy link
Author

neoFuzz commented Dec 2, 2024

New commit to revert the test change. Any more thoughts?

@neoFuzz
Copy link
Author

neoFuzz commented Dec 4, 2024

I should also note that the GitHub Action should build a release that can be downloaded. It triggers when a new Release is created. Just released I did write in the OP😅

@Hanmac is this good to merge?

@Hanmac Hanmac requested a review from kevlahnota December 5, 2024 08:22
@Hanmac
Copy link

Hanmac commented Dec 5, 2024

@kevlahnota i don't know much about the maven plugin, can you look at this too?

@kevlahnota
Copy link

@kevlahnota i don't know much about the maven plugin, can you look at this too?

I think this just simplifies things so it's easier to grab the modded artifact though the main project will need update to get the new artifact from jitpack

Copy link

@Hanmac Hanmac 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 that might be good enough for now?

neoFuzz and others added 5 commits December 13, 2024 16:29
…ll allow the plugin to go to Maven Central
…es PowerShell over CmdLine - this avoids cmd's 8191 character limit and takes advantage of PowerShell's 32k limit. Removed Code Style check. Updated Plexus XML dependency
@neoFuzz
Copy link
Author

neoFuzz commented Dec 18, 2024

I just tried running it on the computers I use:

  1. Home computer: Windows 10 = Builds APK. The only issue was that if the directory wasn't clean, some error occurred.
  2. Work computer: Windows 11 = Builds APK, if certain restrictions aren't in effect. The problem I had was uapksigner can't run from a my user's temp folder (work restriction).

Changes don't affect *nix as they don't have cmd.exe and will use continue to bash shell.

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.

3 participants