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 Ninja 1.12.1 support, including improved platform detection #29

Merged
merged 16 commits into from
Jan 31, 2025

Conversation

Ahajha
Copy link
Contributor

@Ahajha Ahajha commented Aug 22, 2024

This adds Ninja 1.12.1 (and 1.12.0) support. Ninja ships Linux and Windows ARM binaries since 1.12.0, so I've added some logic to account for that (context: https://github.com/ninja-build/ninja/releases/tag/v1.12.0)

I unfortunately have no way to test if the ARM support works, as Github doesn't have public ARM runners available.

Feel free to roast me on my JS, I normally don't write in it :) I figured the new logic was too complex to shove into a series of ternaries.

I spent way too much time trying to minimize the whitespace changes in dist/index.js, I think there's something up with the file. Saving the file even without formatting creates thousands of whitespace changes that are really hard to get git to ignore.

const destDir = core.getInput('destination') || 'ninja-build'
const proxyServer = core.getInput('http_proxy')

const [error, platform] = selectPlatform(core.getInput('platform'), version)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aside from the above function, this is the only changed line

.github/workflows/main.yml Outdated Show resolved Hide resolved
Co-authored-by: Christian Clauss <[email protected]>
cclauss added a commit to cclauss/gha-setup-ninja that referenced this pull request Jan 21, 2025
@cclauss
Copy link
Contributor

cclauss commented Jan 21, 2025

@seanmiddleditch Please approve the 1 workflow awaiting approval below so we can see if these tests pass.

@seanmiddleditch seanmiddleditch dismissed their stale review January 31, 2025 05:12

The merge-base changed after approval.

@seanmiddleditch seanmiddleditch self-requested a review January 31, 2025 05:30
@seanmiddleditch seanmiddleditch merged commit 78f0be8 into seanmiddleditch:master Jan 31, 2025
13 checks passed
@seanmiddleditch
Copy link
Owner

Merged, thanks! Sorry for the ridiculous delay.

@cclauss
Copy link
Contributor

cclauss commented Jan 31, 2025

Should the default version be raised to the current version so things work by default on Ubuntu ARM?

@Ahajha
Copy link
Contributor Author

Ahajha commented Jan 31, 2025

@seanmiddleditch Just happy to have it merged!

@cclauss It should be, I changed it in action.yaml.

@Ahajha Ahajha deleted the ninja-1.12.1 branch January 31, 2025 05:53
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