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

allow to define target specific dependencies #34

Merged
merged 1 commit into from
Mar 17, 2021
Merged

Conversation

gdesmott
Copy link
Owner

@gdesmott gdesmott commented Mar 9, 2021

Fix #33

@gdesmott gdesmott added enhancement New feature or request feature New feature labels Mar 9, 2021
@codecov
Copy link

codecov bot commented Mar 9, 2021

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%) ⬇️
src/metadata.rs 97.86% <97.87%> (-0.05%) ⬇️
src/test.rs 97.57% <100.00%> (+0.10%) ⬆️

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
Copy link

svenfoo commented Mar 12, 2021

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
Copy link

jnqnfe commented Mar 13, 2021

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

@gdesmott
Copy link
Owner Author

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

@gdesmott
Copy link
Owner Author

gdesmott commented Mar 15, 2021

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
Copy link
Owner Author

@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?

@gdesmott gdesmott changed the title allow to define OS specific dependencies allow to define target specific dependencies Mar 16, 2021
@svenfoo
Copy link

svenfoo commented Mar 16, 2021

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 gdesmott merged commit e68b15d into master Mar 17, 2021
@gdesmott gdesmott deleted the os-specific-33 branch March 17, 2021 08:28
@gdesmott
Copy link
Owner Author

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
Copy link

svenfoo commented Mar 23, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support target specific dependencies
3 participants