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 gifsave bindings. #31

Merged
merged 1 commit into from
Jan 23, 2023
Merged

Add gifsave bindings. #31

merged 1 commit into from
Jan 23, 2023

Conversation

bigfarts
Copy link
Contributor

@bigfarts bigfarts commented Jan 6, 2023

No description provided.

@bassco
Copy link
Collaborator

bassco commented Jan 9, 2023

Thanks for the contribution. Can you let us know how you created these bindings. i.e. was it an additional dev dependency added to the cli, or did you manually make the changes?

In this way, we can add the additional context for future upgrades.

@bassco
Copy link
Collaborator

bassco commented Jan 23, 2023

@bigfarts - can you help us with the config you used to create these binds - they will be lost in future upgrades if we cannot add the generation to the current workflow

@bigfarts
Copy link
Contributor Author

Oops, I didn't realize there was a generator. It looks like I just had to install cgif-dev to make this work.

@bigfarts
Copy link
Contributor Author

Hmm, it looks like I ended up generating aarch64 bindings. I don't have an x86_64 machine to generate x86_64 bindings unfortunately, you might need to generate the bindings on your side for the right architecture. Sorry about that!

@bassco
Copy link
Collaborator

bassco commented Jan 23, 2023

@bigfarts

Hmm, it looks like I ended up generating aarch64 bindings. I don't have an x86_64 machine to generate x86_64 bindings unfortunately, you might need to generate the bindings on your side for the right architecture. Sorry about that!
How did you determine this? Excuse my ignorance of the rust toolchain.

Do you think the removal of the --toolchain 1.66.0-x86_64-unknown-linux-musl parameter might have anything to do with it?

The attached files in the PR LGTM. i.e. I don't see any architecture specifics within them.

@bigfarts
Copy link
Contributor Author

My computer is aarch64 and it looks like it installed the aarch64 Docker image. In the bindings.rs diff (hidden by default) there's a bunch of aarch64-specific symbols it decided to put in there: https://github.com/olxgroup-oss/libvips-rust-bindings/pull/31/files#diff-c030113407a164cbd1ad3500c613f59abd4346ba13ee5b653c56f393c937b0a8R4499-R4504

@bassco
Copy link
Collaborator

bassco commented Jan 23, 2023

good catch!

ok, can I propose that I take your changes and generate the code into a separate PR? I'll be able to get @BogdanVidrean to review and merge tomorrow and then we can publish a patch release to crates?

@bassco
Copy link
Collaborator

bassco commented Jan 23, 2023

hold on, let me see if I can push some changes to this branch - it wasn't obvious initially, but I thinkI have found a way to do this.

The bindings unfortunately aren't generated because my computer has a funny architecture.
@bigfarts
Copy link
Contributor Author

I've removed the bindings generation from this PR and just edited the Dockerfile. Thanks!

@bassco bassco merged commit 26a173f into olxgroup-oss:master Jan 23, 2023
@bassco
Copy link
Collaborator

bassco commented Jan 23, 2023

I appreciate your help @bigfarts and thanks for the contribution.

I'll setup the PR for the new version

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.

None yet

2 participants