-
Notifications
You must be signed in to change notification settings - Fork 8
[WIP] GitHub Actions setup #6
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
base: main
Are you sure you want to change the base?
Conversation
- os: macos-latest-large | ||
target: aarch64-apple-ios | ||
- os: macos-latest-large | ||
# cross-compile for Linux ARM using Apple Silicon until we have ARM runners |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's clever. Might have to steal that one elsewhere …
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good, though a few things I think should be tweaked before merging.
Also: I'd encourage you to add a Dependabot setup to keep the GitHub Actions steps up to date. (See https://github.com/contentauth/c2pa-rs/pull/602/files#diff-dd4fbda47e51f1e35defb9275a9cd9c212ecde0b870cba89ddaaae65c5f3cd28R27-R32 for an example.)
target: x86_64-apple-darwin | ||
- os: ubuntu-latest | ||
target: x86_64-unknown-linux-musl | ||
# - os: windows-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is Windows disabled? That seems like a blocker.
sudo apt-get install musl-tools | ||
|
||
- name: Install Rust toolchain | ||
uses: dtolnay/[email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why fix on a specific Rust version? IMHO should matrix the MSRV version and stable.
run: make TARGET=${{matrix.target}} release | ||
|
||
- name: Run C tests | ||
if: ${{matrix.os != 'windows-latest'}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again … why is Windows disabled? See also lines 70 and 74.
run: echo "::warning::C/C++ tests did NOT run in Windows" | ||
|
||
- name: Move files to main folder | ||
run: mv target/release/libc2pa_c* . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not work on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC Windows uses rename
instead of mv
for this purpose. I think the conditionals used above if: ... 'windows-latest' ...
should work to separate Windows from Unix/Mac build hosts.
@scouten-adobe is there more context on this? Seems like something got dropped and I'm not sure what this PR attempts to fix |
No description provided.