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

CI: deploy library artifacts, add i686 linux and aarch64 mac targets #3185

Merged
merged 2 commits into from
Apr 19, 2023

Conversation

fdintino
Copy link
Contributor

@fdintino fdintino commented Apr 17, 2023

This adds artifacts to the deploy workflow for the cargo-c library and headers. I have created a release on my fork that includes the new release artifacts.

I have held off on opening this pull request in the hope that cross would add support for custom subcommands (cross-rs/cross#716) but that doesn't seem to be happening anytime soon. I'm using a pretty ugly hack to get around this limitation: I run cross with a custom dockerfile whose entrypoint rewrites cargo build to cargo cbuild in the CMD arguments.

I have also added targets for i686 linux and aarch64 mac.

I was motivated to do this work so that I would not need to maintain cross-platform builds of rav1e when packing pillow-avif-plugin. The librav1e artifacts on my fork are now being used here.


- name: Upload pre-release binaries
mkdir -p dist/{include/rav1e,lib/pkgconfig}
cp target/${{ matrix.target }}/release-strip/rav1e.h dist/include/rav1e/
Copy link
Collaborator

Choose a reason for hiding this comment

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

cargo cinstall wouldn't work better?

Copy link
Collaborator

@lu-zero lu-zero left a comment

Choose a reason for hiding this comment

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

It looks very good, if you could try to use directly cargo cinstall would be great.


- name: Upload pre-release binaries
mkdir -p dist/{include/rav1e,lib/pkgconfig}
cp target/${{ matrix.target }}/release-strip/rav1e.h dist/include/rav1e/
Copy link
Collaborator

Choose a reason for hiding this comment

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

use cargo cinstall instead as well


for arg in "$@"; do
arg=$(echo $arg | perl -pne \
's/(?:(?<=\s)|^)build(?=\s)/cbuild --library-type staticlib --library-type cdylib/')
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be cargo cinstall I think.

@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.04 🎉

Comparison is base (80d0ff2) 86.34% compared to head (68bad0f) 86.38%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3185      +/-   ##
==========================================
+ Coverage   86.34%   86.38%   +0.04%     
==========================================
  Files          85       85              
  Lines       33260    33260              
==========================================
+ Hits        28718    28732      +14     
+ Misses       4542     4528      -14     

see 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@lu-zero lu-zero left a comment

Choose a reason for hiding this comment

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

If it works for you, it looks fine to me.

@fdintino
Copy link
Contributor Author

Thanks. I've just reconfirmed that these artifacts still work with the cinstall change.

@lu-zero
Copy link
Collaborator

lu-zero commented Apr 19, 2023

Please rebase and I'll land it immediately :)

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.

2 participants