gdesmott / system-deps

Run pkg-config from declarative dependencies in Cargo.toml
Apache License 2.0
89 stars 21 forks source link

allow to define target specific dependencies #34

Closed gdesmott closed 3 years ago

gdesmott commented 3 years ago

Fix #33

codecov[bot] commented 3 years ago

Codecov Report

Merging #34 (5f9670d) into master (9ac6f2f) will decrease coverage by 13.05%. The diff coverage is 98.31%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #34       +/-   ##
===========================================
- Coverage   91.23%   78.17%   -13.06%     
===========================================
  Files           3        3               
  Lines        1699     2250      +551     
===========================================
+ Hits         1550     1759      +209     
- Misses        149      491      +342     
Impacted Files Coverage Δ
src/lib.rs 63.12% <97.61%> (-20.81%) :arrow_down:
src/metadata.rs 97.86% <97.87%> (-0.05%) :arrow_down:
src/test.rs 97.57% <100.00%> (+0.10%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9ac6f2f...5f9670d. Read the comment docs.

svenfoo commented 3 years ago

This doesn't work so well for the use case of librsvg. We would like to be able to express that pangoft2 and fontconfig are required dependencies for all platforms but win32.

I think what would work better is to stick more closely to the syntax that Cargo uses for platform specific configurations. Something like the following would certainly be somewhat more complex, but afaics it should allow all sorts of platform-specific dependencies to be expressed:

[package.metadata.system-deps]
libxml2 = { name = "libxml-2.0", version = "2.9" }
pangocairo = "1.38"

[package.metadata.system-deps.'cfg(windows)']
fontconfig = { version = "1.7", optional = true }
pangoft2 = { version = "1.38", optional = true }

[package.metadata.system-deps.'cfg(not(windows))']
fontconfig = { version = "1.7" }
pangoft2 = { version = "1.38" }
jnqnfe commented 3 years ago

I'd agree that supporting such cfg() based conditions would be the ideal means of capturing platform conditional data.

gdesmott commented 3 years ago

Looks like there is a cfg!, I suppose we could use that to implement such feature.

gdesmott commented 3 years ago

Actually cfg! does not allow me to pass a string, so I think I'd have to use something like parse_cfg, cfg-expr or cargo_platform.

gdesmott commented 3 years ago

@svenfoo @jnqnfe : I implemented just that thanks to the awesome cfg-expr and their great maintainers which have been very helpful improving the crate to fit my needs.

Can you please give it a shot?

svenfoo commented 3 years ago

Wow, that's exactly the syntax that I proposed, and it seems to work nicely. That's awesome! 😍

I have updated the feature branch in librsvg with this solution.

gdesmott commented 3 years ago

Great! It's been quite easy to implement actually, thanks to the cfg-expr crate.

I just released 3.1.0 with this new feature so you should be good for librsvg.

svenfoo commented 3 years ago

Thanks a lot! We are now using this in librsvg master branch and this brings us one step closer to the goal of dropping the autoconf/automake based build system in favor of using cargo only.