linebender / piet

An abstraction for 2D graphics.
Apache License 2.0
1.26k stars 94 forks source link

Unable to `cargo test` on windows when no cairo installed #102

Open hwchen opened 5 years ago

hwchen commented 5 years ago

With #97 adding default members to piet's workspace Cargo.toml, I was unable to run cargo test on windows because cargo attempted to build piet-cairo and I did not have the required cairo libraries installed.

Removing default-members in the Cargo.toml fixed the issue for me for now.

Not sure what the permanent fix is yet, though.

Related to #33

raphlinus commented 5 years ago

I think --all --exclude piet-cairo might be a temporary workaround, but we probably want a more systematic solution.

cmyr commented 5 years ago

@dianarg no pressure, but if you wanted to look into this you'd be very welcome! I see you'd already thought of this issue (https://github.com/linebender/piet/pull/97/files#r341583903).

I do think that by default, on windows, piet should just be building direct-write. I dunno. Maybe piet should always require an explicit backend? I'm not sure what the perfect structure for the piet project is, but I am confident that we haven't found it yet. 😆

dianarg commented 5 years ago

Sure, I'm happy to take a look at it this weekend. At the very least there should be a way to print a nicer error message, along the lines of "cairo library not installed, try --all --exclude cairo " when cargo build or cargo test fails. The current behavior I believe is just a normal ugly looking link error.

dianarg commented 5 years ago

I started looking into this today. I am struggling to figure out if it's even possible to make Cargo do what I want (a nicer error message at the time it checks the cairo dependency, instead of the long error from link.exe). The error when you build on Windows without cairo installed looks like this:

error: linking with `link.exe` failed: exit code: 1181
[huge link.exe command with LIBPATH]
  = note: LINK : fatal error LNK1181: cannot open input file 'cairo.lib'

The output on Linux is similar but I didn't get around testing it yet.

I would have expected that adding links = "cairo" to the package settings would do something but it seems the cairo-rs crate has already specified the dependency:

> cargo build
error: failed to select a version for `cairo-sys-rs`.
    ... required by package `cairo-rs v0.7.1`
    ... which is depended on by `cairo_test v0.1.0 (C:\Users\Diana\cairo_test)`
versions that meet the requirements `= 0.9.0` are: 0.9.0

the package `cairo-sys-rs` links to the native library `cairo`, but it conflicts with a previous package which links to `cairo` as well:
package `cairo_test v0.1.0 (C:\Users\Diana\cairo_test)`

failed to select a version for `cairo-sys-rs` which could resolve this conflict

Here is a smaller reproducer for the problem:

Cargo.toml

[package]
name = "cairo_test"
version = "0.1.0"

# specify that we link to native library cairo
# this doesn't work
#links = "cairo"

[dependencies]
cairo-rs = { version = "0.7.1", default_features = false }

src/main.rs

extern crate cairo;

use cairo::{Context, Format, ImageSurface};

fn main() {
    let surface = ImageSurface::create(Format::ARgb32, 10, 10)
        .expect("Can't create surface");
    let mut cr = Context::new(&surface);
    cr.scale(3.3, 3.3);
}
cmyr commented 5 years ago

The hacky way to do this might be with a custom build.rs script, and that feels a bit bad. I'd personally perfer to not require cairo on windows, since I just don't expect many folks to have it? Not sure how fair this is, very much an assumption.

dianarg commented 5 years ago

109 should fix that. I agree cairo should not be required on Windows, since it seems direct2d just works. That patch didn't need to be bundled with the Azure testing.