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

gate the build with target_os = "macos" #506

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alexanderkjall
Copy link

@jdm
Copy link
Member

jdm commented Apr 19, 2022

Why is core-graphics-types being compiled on Linux?

@alexanderkjall
Copy link
Author

This is due to the peculiarities of how the Debian resolution of source packaged work, I'll try to explain why I packaged it.

Problem statement

I would like to package the font-kit package, and that depends on core-graphics that depend on core-graphics-types.

A deeper look into why

Each rust crates becomes one source package in Debian, so that the copyright + license information can be reviewed, and in according to the Debian policy of not vendoring dependencies.

Each dependency from Cargo.toml becomes one item in the Debian depends specification, you can see an (unrelated) example here: https://linux-packages.com/debian/package/librust-addr2line-dev

Since the depends resolution happens before cargo starts building the source code, the #[cnf] gating in Cargo.toml in font-kit isn't availible.

Then the option becomes to either patch the font-kit package to remove all unneeded dependencies, or to package those into their own debian packages.

I prefer to reduce the amount of debian specific patches if that is possible, so I packaged core-graphics-types, that turned out to still require a patch, since the code didn't even build on linux, but I thought I could at least provide that patch upstream.

Sorry for the wall of text, but I hope that this explains the background of the patch.

@jdm
Copy link
Member

jdm commented Apr 20, 2022

Does every foundational crate used by a cross-platform rust library require patches like this?

@alexanderkjall
Copy link
Author

I think many build cross platform without trouble as it is today, we also package windows api crates for example: https://packages.debian.org/sid/librust-winapi-dev

I don't know if all of them have gotten patches from some debian package maintainer at some point in time or not :)

Copy link
Contributor

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know anything about the Debian depends translation, but the fact that it wants to use non-unix dependencies sounds to me more like a bug in that? I mean, font-kit's Cargo.toml clearly describes that these dependencies are macOS/iOS only (same goes for winapi, it shouldn't have to download that extra 1MB of useless data).

But, even barring that, what if we at some point to support a Linux version of Core Graphics like GNUStep's Opal (not entirely theoretical, I might want to do that at some point)? Then your Debian build would start failing again, since we try to link to something that doesn't exist. And then you would need to package Opal, and all of it's sub-dependencies, even though none of it is ever used in font-kit.

Comment on lines 13 to 16
#[cfg(target_os = "macos")]
pub mod base;
#[cfg(target_os = "macos")]
pub mod geometry;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would remove support for iOS and tvOS (and watchOS), those should allowed as well.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably b009c87) made this pull request unmergeable. Please resolve the merge conflicts.

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

4 participants