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 Linux Mips support #16

Merged
merged 1 commit into from
Jan 26, 2023
Merged

Conversation

bitPogo
Copy link
Contributor

@bitPogo bitPogo commented Jan 25, 2023

No description provided.

ORG_GRADLE_PROJECT_mavenCentralUsername: ${{ secrets.MAVEN_CENTRAL_USERNAME }}
ORG_GRADLE_PROJECT_mavenCentralPassword: ${{ secrets.MAVEN_CENTRAL_PASSWORD }}
ORG_GRADLE_PROJECT_signingInMemoryKey: ${{ secrets.SIGNING_KEY }}
run: ./gradlew build publishMips -x check
Copy link
Contributor

@JakeWharton JakeWharton Jan 25, 2023

Choose a reason for hiding this comment

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

I'm not sure I'd bother with running build (and needing to exclude check) here.

I think instead we make this job depend on the other one so that we know build has already succeeded and we are only concerned with publishing these two targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want to split the jobs, we need to cache the build artifact from the other job (we are not doing step both run in 2 independent containers), which is more effort. (I did that here and you will need this as well to understand it).

The check we can skip since a) it did run already with MacOS and b) the container will not execute the tests since it does not run on MIPS.

Copy link
Contributor

@JakeWharton JakeWharton Jan 25, 2023

Choose a reason for hiding this comment

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

Ah, looks like you already have the needs: publish. I must have missed it first time around because I was reviewing on mobile. My bad. That's all I was saying we needed.

So yeah the only thing left is why do build publishMips -x check instead of only publishMips?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad...you are absolutely right.

build.gradle.kts Outdated
Comment on lines 35 to 36
linuxX64()
linuxMips32()
linuxMipsel32()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
linuxX64()
linuxMips32()
linuxMipsel32()
linuxMips32()
linuxMipsel32()
linuxX64()

(The current list is alphabetized)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, kinda. I see I put 32 after 64 above 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (if I got it correctly)

@bitPogo bitPogo force-pushed the feature/add-linuxmips branch 2 times, most recently from 77249a7 to 9ebdb08 Compare January 25, 2023 18:45
@JakeWharton
Copy link
Contributor

You accidentally checked in a .DS_Store file. You can put it in your global Git ignore so it never winds up in any repo. See https://pineco.de/snippets/globally-gitignore-the-ds_store-file/.

@bitPogo bitPogo force-pushed the feature/add-linuxmips branch from 9ebdb08 to 912e1c5 Compare January 26, 2023 11:56
build.gradle.kts Outdated
Comment on lines 34 to 36
linuxMips32()
linuxMipsel32()
linuxArm64()
Copy link
Owner

Choose a reason for hiding this comment

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

Now Arm64 comes after Arm32 👍 In total I'm not sure this new list is an improvement when it comes to alphabetical ordering, though 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tell me what you prefer

build.gradle.kts Outdated
Comment on lines 34 to 36
linuxMips32()
linuxMipsel32()
linuxArm64()
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
linuxMips32()
linuxMipsel32()
linuxArm64()
linuxArm64()
linuxMips32()
linuxMipsel32()

@bitPogo bitPogo force-pushed the feature/add-linuxmips branch from 912e1c5 to 91ad47a Compare January 26, 2023 12:55
@bitPogo bitPogo requested a review from cketti January 26, 2023 12:55
@bitPogo bitPogo force-pushed the feature/add-linuxmips branch from 91ad47a to 7bb4def Compare January 26, 2023 12:56
@cketti cketti merged commit 6d2c1d2 into cketti:main Jan 26, 2023
@cketti
Copy link
Owner

cketti commented Jan 26, 2023

Thanks 👍

@bitPogo
Copy link
Contributor Author

bitPogo commented Jan 26, 2023

I thank you for the effort!

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