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

Make glib dependency optional #127

Merged
merged 8 commits into from May 4, 2017

Conversation

Projects
None yet
4 participants
@rtsuk

rtsuk commented May 1, 2017

The Cairo bindings used the glib::translate traits to handle internal reference counting discipline. However, these are not actually required, and should be possible to use Cairo without a dependency on glib. This patch makes the dependency optional, and changes the implementation to not use the glib traits internally.

The glib feature is a default feature, so this should not cause problems for gtk users of cairo-rs, but does allow for use of Cairo from Rust in projects that don't want to also depend on glib.

Rob Tsuk
Make glib dependency optional
The cairo bindings used the glib::translate traits to handle internal
reference counting discipline. However, these are not actually
required, and should be possible to use cairo without a dependency on
glib. This patch makes the dependency optional, and changes the
implementation to not use the glib traits internally.
@raphlinus

This comment has been minimized.

Show comment
Hide comment
@raphlinus

raphlinus Apr 28, 2017

Collaborator

Slightly inconsistent, you write self.0 here but self.to_raw_ptr() elsewhere. I'd personally go with the latter consistently.

Collaborator

raphlinus commented on src/context.rs in e5f7577 Apr 28, 2017

Slightly inconsistent, you write self.0 here but self.to_raw_ptr() elsewhere. I'd personally go with the latter consistently.

This comment has been minimized.

Show comment
Hide comment
@rtsuk

rtsuk May 1, 2017

Owner

There is no to_raw_ptr() for Context. Are you suggesting I write one?

Owner

rtsuk replied May 1, 2017

There is no to_raw_ptr() for Context. Are you suggesting I write one?

This comment has been minimized.

Show comment
Hide comment
@raphlinus

raphlinus May 1, 2017

Collaborator

Yes.

Collaborator

raphlinus replied May 1, 2017

Yes.

@raphlinus

This comment has been minimized.

Show comment
Hide comment
@raphlinus

raphlinus Apr 28, 2017

Collaborator

I'm not getting why this needs two different impls, for glib or not. Seems to me the non-glub from_raw_full fn should also work even in the glib case. What am I missing?

Edit: ok, I see now. Two totally different structs, one of which is a Stash from the glib wrapper, the other is a newtype over the pointer. I think this is fine.

Collaborator

raphlinus commented on src/font/font_face.rs in e5f7577 Apr 28, 2017

I'm not getting why this needs two different impls, for glib or not. Seems to me the non-glub from_raw_full fn should also work even in the glib case. What am I missing?

Edit: ok, I see now. Two totally different structs, one of which is a Stash from the glib wrapper, the other is a newtype over the pointer. I think this is fine.

@raphlinus

This comment has been minimized.

Show comment
Hide comment
@raphlinus

raphlinus Apr 28, 2017

Collaborator

My sense of aesthetics would call for the conversion from C-style string pointer to Option to be split out into its own helper function. But this could go either way.

Collaborator

raphlinus commented on src/font/font_face.rs in e5f7577 Apr 28, 2017

My sense of aesthetics would call for the conversion from C-style string pointer to Option to be split out into its own helper function. But this could go either way.

This comment has been minimized.

Show comment
Hide comment
@rtsuk

rtsuk May 1, 2017

Owner

Where would your send of aesthetics put the helper function?

Owner

rtsuk replied May 1, 2017

Where would your send of aesthetics put the helper function?

This comment has been minimized.

Show comment
Hide comment
@raphlinus

raphlinus May 1, 2017

Collaborator

Given that this is the only usage and there's no existing "utils" module, I'd say at the end of the file. Then if there became more usages, move it to some kind of "common" location.

Collaborator

raphlinus replied May 1, 2017

Given that this is the only usage and there's no existing "utils" module, I'd say at the end of the file. Then if there became more usages, move it to some kind of "common" location.

Show outdated Hide outdated src/font/font_face.rs
Show outdated Hide outdated src/font/font_face.rs
Show outdated Hide outdated src/font/font_face.rs
@raphlinus

This comment has been minimized.

Show comment
Hide comment
@raphlinus

raphlinus May 1, 2017

Other than these minor style issues, I think this would be a good thing to have. There are lots of use cases for cairo (like command-line or server based tools for drawing images) where a glib dependency is not relevant. I reviewed an earlier version and provided feedback on it.

raphlinus commented May 1, 2017

Other than these minor style issues, I think this would be a good thing to have. There are lots of use cases for cairo (like command-line or server based tools for drawing images) where a glib dependency is not relevant. I reviewed an earlier version and provided feedback on it.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 1, 2017

Member

Thanks @rtsuk
Only problem is support: CI need updated too
I almost sure that we can merge this after current version published, not sure that it need be merged before.

Member

EPashkin commented May 1, 2017

Thanks @rtsuk
Only problem is support: CI need updated too
I almost sure that we can merge this after current version published, not sure that it need be merged before.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 1, 2017

Member

@EPashkin: As long as it's tested (and working haha), I don't mind merging it before.
@rtsuk: Thanks a lot for your PR!
@EPashkin && @raphlinus: Thanks for your review! :)

Member

GuillaumeGomez commented May 1, 2017

@EPashkin: As long as it's tested (and working haha), I don't mind merging it before.
@rtsuk: Thanks a lot for your PR!
@EPashkin && @raphlinus: Thanks for your review! :)

Rob Tsuk added some commits May 1, 2017

Rob Tsuk
Rob Tsuk
@raphlinus

This comment has been minimized.

Show comment
Hide comment
@raphlinus

raphlinus May 1, 2017

+1 from me, assuming the CI passes.

raphlinus commented May 1, 2017

+1 from me, assuming the CI passes.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 1, 2017

Member

Please add test in appveyor.yml too, just before - mkdir .cargo

Member

EPashkin commented May 1, 2017

Please add test in appveyor.yml too, just before - mkdir .cargo

Rob Tsuk
@rtsuk

This comment has been minimized.

Show comment
Hide comment
@rtsuk

rtsuk May 1, 2017

I'm afraid I have no Windows box to use to try to fix Win32Surface.

rtsuk commented May 1, 2017

I'm afraid I have no Windows box to use to try to fix Win32Surface.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 1, 2017

Member

Can you just do same as image_surface.rs ?

Member

EPashkin commented May 1, 2017

Can you just do same as image_surface.rs ?

@rtsuk

This comment has been minimized.

Show comment
Hide comment
@rtsuk

rtsuk May 1, 2017

I assume so, but I'll have no way to compile my changes except for appveyor. Is using appveyor that way ok?

rtsuk commented May 1, 2017

I assume so, but I'll have no way to compile my changes except for appveyor. Is using appveyor that way ok?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 1, 2017

Member

IMHO it normal if just don't push too often.

Member

EPashkin commented May 1, 2017

IMHO it normal if just don't push too often.

@rtsuk

This comment has been minimized.

Show comment
Hide comment
@rtsuk

rtsuk May 1, 2017

OK, not comfortable working that way. I'll try to track down a Windows machine I can use.

rtsuk commented May 1, 2017

OK, not comfortable working that way. I'll try to track down a Windows machine I can use.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 1, 2017

Member

Theoretically you also can use rustc cross-compilation.

Member

EPashkin commented May 1, 2017

Theoretically you also can use rustc cross-compilation.

@rtsuk

This comment has been minimized.

Show comment
Hide comment
@rtsuk

rtsuk May 1, 2017

Yah, I'm doing that for Fuchsia, but I think I'd have to have the cross compiled version of Cairo present as well, along with its dependencies. I'm not sure that's easier than finding a Windows box.

rtsuk commented May 1, 2017

Yah, I'm doing that for Fuchsia, but I think I'd have to have the cross compiled version of Cairo present as well, along with its dependencies. I'm not sure that's easier than finding a Windows box.

Rob Tsuk
@rtsuk

This comment has been minimized.

Show comment
Hide comment
@rtsuk

rtsuk May 1, 2017

It turns out the Acer Switch 12 I have on my desk for other purposes has a working windows install on it.

rtsuk commented May 1, 2017

It turns out the Acer Switch 12 I have on my desk for other purposes has a working windows install on it.

Rob Tsuk
@rtsuk

This comment has been minimized.

Show comment
Hide comment
@rtsuk

rtsuk May 3, 2017

The AppVeyor failure above is not in cairo-rs, but in a dependency and only in 64 bit.

Compiling libc v0.2.22
Compiling glib v0.1.1 (https://github.com/gtk-rs/glib#d40b8344)
Compiling pango v0.1.1 (https://github.com/gtk-rs/pango#23484dab)
error[E0106]: missing lifetime specifier
--> C:\Users\appveyor.cargo\git\checkouts\gdk-0e2106e2e12a0cc1\afa1247\src\rectangle.rs:131:6
|
131 | impl glib::value::FromValueOptional for Rectangle {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected lifetime parameter
error[E0106]: missing lifetime specifier
--> C:\Users\appveyor.cargo\git\checkouts\gdk-0e2106e2e12a0cc1\afa1247\src\rgba.rs:182:6
|
182 | impl glib::value::FromValueOptional for RGBA {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected lifetime parameter
error: aborting due to 2 previous errors

rtsuk commented May 3, 2017

The AppVeyor failure above is not in cairo-rs, but in a dependency and only in 64 bit.

Compiling libc v0.2.22
Compiling glib v0.1.1 (https://github.com/gtk-rs/glib#d40b8344)
Compiling pango v0.1.1 (https://github.com/gtk-rs/pango#23484dab)
error[E0106]: missing lifetime specifier
--> C:\Users\appveyor.cargo\git\checkouts\gdk-0e2106e2e12a0cc1\afa1247\src\rectangle.rs:131:6
|
131 | impl glib::value::FromValueOptional for Rectangle {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected lifetime parameter
error[E0106]: missing lifetime specifier
--> C:\Users\appveyor.cargo\git\checkouts\gdk-0e2106e2e12a0cc1\afa1247\src\rgba.rs:182:6
|
182 | impl glib::value::FromValueOptional for RGBA {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected lifetime parameter
error: aborting due to 2 previous errors

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 3, 2017

Member

Don't worry, it because gtk-rs/glib#162 merged in between.
I will restart appveyour after second part merged

Member

EPashkin commented May 3, 2017

Don't worry, it because gtk-rs/glib#162 merged in between.
I will restart appveyour after second part merged

@rtsuk

This comment has been minimized.

Show comment
Hide comment
@rtsuk

rtsuk May 4, 2017

Is there anything else you'd like me to do on this PR?

rtsuk commented May 4, 2017

Is there anything else you'd like me to do on this PR?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 4, 2017

Member

I think it's good. Confirmation @EPashkin? :)

Member

GuillaumeGomez commented May 4, 2017

I think it's good. Confirmation @EPashkin? :)

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin
Member

EPashkin commented May 4, 2017

@GuillaumeGomez its good.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 4, 2017

Member

Great! Thanks!

Member

GuillaumeGomez commented May 4, 2017

Great! Thanks!

@GuillaumeGomez GuillaumeGomez merged commit 058ac45 into gtk-rs:master May 4, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment