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 crate users to opt out of some platform support #104

Closed
wants to merge 1 commit into from

Conversation

lopopolo
Copy link
Collaborator

We have gotten several inbound tickets with users expressing concern over the inclusion of cxx to support the haiku platform. Additionally, for my personal use, I would prefer to be able to prune things like wasm-bindgen from my lockfile when not expressly enabling Wasm platform support, similar to the approach the getrandom crate takes.

This commit adds cargo features which allow disabling platform support for each platform that requires a third party dependency. Currently, that set of platforms is:

  • android
  • all apple targets
  • haiku
  • wasm
  • windows

This commit adds a new default feature, platform-all, which retains the existing behavior of all platforms being enabled by default. platform-all is a meta feature which enables all of the other platform support features.

For example, to disable haiku support, users would add the following to their Cargo.toml:

iana-time-zone = { version = "0.2", default-features = false, features = ["platform-android", "platform-apple", "platform-wasm", "platform-windows"] }

The addition of a new default feature is a breaking change because it may break the build for folks currently depending on this crate with default-features = false.

I'm not sure if this is the direction we want to go, but it would address the ask from these tickets:

I know that cargo won't build deps unless compiling for a particular platform, but practically, most users don't want or need Haiku or Wasm support. These targets have heavy dependencies which end up in the lock file. Deps in the lock file have a carrying cost in terms of supply chain risk and need for patching/updating.

We have gotten several inbound tickets with users expressing concern
over the inclusion of `cxx` to support the haiku platform. Additionally,
for my personal use, I would prefer to be able to prune things like
`wasm-bindgen` from my lockfile when not expressly enabling Wasm
platform support, similar to the approach the `getrandom` crate takes.

This commit adds cargo features which allow disabling platform support
for each platform that requires a third party dependency. Currently,
that set of platforms is:

- android
- all apple targets
- haiku
- wasm
- windows

This commit adds a new default feature, `platform-all`, which retains
the existing behavior of all platforms being enabled by default.
`platform-all` is a meta feature which enables all of the other platform
support features.

For example, to disable haiku support, users would add the following to
their `Cargo.toml`:

```toml
iana-time-zone = { version = "0.2", default-features = false, features = ["platform-android", "platform-apple", "platform-wasm", "platform-windows"] }
```

The addition of a new default feature is a breaking change because it
may break the build for folks currently depending on this crate with
`default-features = false`.

I'm not sure if this is the direction we want to go, but it would
address the ask from these tickets:

- #89
- #88

I know that cargo won't build deps unless compiling for a particular
platform, but practically, most users don't want or need Haiku or Wasm
support. These targets have heavy dependencies which end up in the lock
file. Deps in the lock file have a carrying cost in terms of supply
chain risk and need for patching/updating.
@lopopolo lopopolo added Tier-1 Rust Tier-1 platform Tier-2 Rust Tier-2 platform Tier-3 Rust Tier-3 platform labels May 10, 2023
@lopopolo lopopolo requested review from astraw and Kijewski May 10, 2023 03:20
@lopopolo
Copy link
Collaborator Author

it looks like the android build fails the same way as in #105, so I don't think I've broken anything with the new Cargo features.

@astraw
Copy link
Member

astraw commented May 11, 2023

I'm not sure this merits a semver breaking change - the API has not changed. Also, the build should not break for 99.9% of people, either. Specifically, chrono does not disable the default features in its Cargo.toml.

That said, I'm undecided about this overall and am still thinking about it. @Kijewski @djc I would be interested in your thoughts.

@djc
Copy link
Contributor

djc commented May 11, 2023

I'm not sure this merits a semver breaking change - the API has not changed.

Hmm -- the API surely has changed for people who depend on iana-time-zone with default-features = false, right?

Additionally, for my personal use, I would prefer to be able to prune things like wasm-bindgen from my lockfile when not expressly enabling Wasm platform support, similar to the approach the getrandom crate takes.

Why? What are the downsides of having more crates in your Cargo.lock file? (I'm not trying to suggest there aren't any, but I think it's valuable for this PR to enumerate what the downsides are specifically.) The reporter for #89 never answered this question. The reporter in #88 wrote:

I am just a bit paranoid with my lockfile :)

These don't seem like strong motivations to me, and a lot of the "complaints" seem to be from people who don't really understand how Cargo works in this case.

FWIW, I wrote up a more general solution in a Cargo issue, but have so far not found the time to go back and turn this into an RFC with the level of detail that it needs.

Ultimately this solution is kinda neat and doesn't impose a lot of complexity, so I'm not against it. It still adds some complexity and doing a semver-breaking release for it might cause a little pain over time due to duplicate dependencies? (Are there other API-breaking changes that you'd like to make but haven't so far?)

@Kijewski
Copy link
Collaborator

The main problem I see: How would one use the feature? E.g. how would chrono use the this library?

[dependencies]
iana-time-zone = { default-features = false }

[features]
default = ["platform-all"]
platform-all = ["iana-time-zone/platform-all"]
platform-amiga = ["iana-time-zone/platform-amiga"]
platform-irix = ["iana-time-zone/platform-irix"]
platform-pcdos = ["iana-time-zone/platform-pcdos"]

How would a user of chrono use this feature?

[dependencies]
chrono = { default-features = false }

[features]
default = ["platform-all"]
platform-all = ["chrono/platform-all"]
platform-amiga = ["chrono/platform-amiga"]
platform-irix = ["chrono/platform-irix"]
platform-pcdos = ["chrono/platform-pcdos"]

Should there be an avalanche of updates if we add a new platform?

IMO rust-lang/cargo#11313 is the only feasible solution for this problem.

The breaking change would actually not be a problem, AFAIS. Dtolnay's semver-trick would work fine. A last version in 0.1 would re-export 0.2 with its default features activated.

@lopopolo
Copy link
Collaborator Author

chrono is not the only way to use this library. It wouldn't be necessary for chrono to expose these features if there would be too much complexity.

One option though would be for chrono to only enable features for tier 1 platforms by default. If applications require support for more exotic platforms, they can depend on iana-time-zone directly and activate more of its features. Because features are additive and unified in the entire dep graph, chrono's use of iana-time-zone would inherit full platform support.

@Kijewski
Copy link
Collaborator

Chrono was only an example.

Maybe features tier-1 to tier-3 would be easier to use than a feature per platform? With default = ["tier-1", "tier-2"]? And the multitude of haiku users would have to depend on this library explicitly.

@djc
Copy link
Contributor

djc commented May 11, 2023

I don't think it makes sense for chrono to disable platform support for some platforms by default on this basis (also it would still be a regression for chrono to offer less platform support going forward than it has in the past).

@lopopolo
Copy link
Collaborator Author

Why? What are the downsides of having more crates in your Cargo.lock file?

I briefly touched on this in the PR description, but I'm happy to expand here.

Practically, the first and easiest opportunity folks have to examine their dependency tree, software supply chain, and SBOM is by inspecting the Cargo.lock file in their application. Tools like cargo-about use the cargo-metadata crate to extract information from the lock file and build an SBOM. This dep tree is also exposed to users every time they do a cargo update.

I have a bias toward more minimal dependency trees and this is the vibe that the various issues we've received have as well. I follow the guideline that every dependency in my lock files should "pull their weight". If a dependency is present in the lock file for features my code does not use, I have generally filed PRs with upstream to add conditional compilation flags or changed my applications to use alternative dependencies to achieve the desired functionality.

Dependencies in Cargo.lock also drive software update workflows and security patching, for example with @dependabot or renovate. The presence of e.g. cxx and its dep tree in my lock files, which is only there to support the haiku target platform, adds a great deal of dep weight, PRs to merge, and process overhead:

$ cargo tree -p iana-time-zone-haiku --target all
iana-time-zone-haiku v0.1.1
└── cxx v1.0.94
    ├── cxxbridge-macro v1.0.94 (proc-macro)
    │   ├── proc-macro2 v1.0.56
    │   │   └── unicode-ident v1.0.8
    │   ├── quote v1.0.27
    │   │   └── proc-macro2 v1.0.56 (*)
    │   └── syn v2.0.15
    │       ├── proc-macro2 v1.0.56 (*)
    │       ├── quote v1.0.27 (*)
    │       └── unicode-ident v1.0.8
    └── link-cplusplus v1.0.8
        [build-dependencies]
        └── cc v1.0.79
            └── jobserver v0.1.26
                └── libc v0.2.144
    [build-dependencies]
    ├── cc v1.0.79 (*)
    └── cxxbridge-flags v1.0.94
[build-dependencies]
└── cxx-build v1.0.94
    ├── cc v1.0.79 (*)
    ├── codespan-reporting v0.11.1
    │   ├── termcolor v1.2.0
    │   │   └── winapi-util v0.1.5
    │   │       └── winapi v0.3.9
    │   │           ├── winapi-i686-pc-windows-gnu v0.4.0
    │   │           └── winapi-x86_64-pc-windows-gnu v0.4.0
    │   └── unicode-width v0.1.10
    ├── once_cell v1.17.1
    ├── proc-macro2 v1.0.56 (*)
    ├── quote v1.0.27 (*)
    ├── scratch v1.0.5
    └── syn v2.0.15 (*)

In a corporate environment, using my own employer as an example, deeper dep trees and larger lock files add burden to our security team. Our security team runs their own software update SLA and security patch workflows outside of @dependabot. More deps means more tickets cut and more operational burden for the security team to push teams to not breach patch timeline SLAs.

rust-lang/cargo#11313 is a really interesting idea to solve this problem and one I would love to see implemented. It would address this issue in a neat way that I'd love to share:

artichoke/artichoke is a Ruby interpreter. Its build process only produces artifacts for Linux, macOS, and Windows. I would love to not have wasm-bindgen and friends in the lock file. artichoke/playground, however, depends on artichoke; the playground targets wasm32-unknown-emscripten. The playground wants the Wasm targets/deps and doesn't want, e.g. windows-sys.

I shy away from using tier features. Both Android and Wasm targets are tier 2, but I find it unlikely that application workspaces are targeting both platforms simultaneously.

@astraw
Copy link
Member

astraw commented May 11, 2023

A few comments:

We don't have any proposed API changes we are saving for a version bump.

The rust API is unchanged here. So the semver question is whether requiring downstream to fiddle with Cargo.toml counts as a breaking change. To me, this seems related to the question of whether MSRV bumps should be considered a breaking semver change, to which I think the consensus is no, although debatable.

The BLAS/LAPACK rust ecosystem has the final binary's Cargo.toml depend on blas-src with the desired feature enabled to pull in the correct implementation which depends on the platform. This works in practice although it seems a bit inconvenient that building the binary requires depending on a crate that might otherwise be buried deep in the dependency tree. It also breaks the concept that Cargo features are additive, because for BLAS implementations they are mutually exclusive (the feature determines which .so/.dll/.dylib gets linked to provide the BLAS implementation).

If we accept this PR, including bumping from 0.1 to 0.2 and thereby making a semver breaking change, I might suggest that the default features should actually be empty. Then, only the final binary should specify the feature(s) for the desired platforms. chrono would then simply have iana-time-zone = {version="0.2.0", optional = true} in its Cargo.toml. The final binary would have iana-time-zone = {version="0.2.0", features=[...]}. This would be inconvenient in that the final binary would have to care for platform support by iana-time-zone, even if this is otherwise buried in the dependency tree.

The options under discussion seem to pit convenience against tight control of lock files. I can understand both desires. I agree that ultimately it would be nice if this could be solved generally (e.g. rust-lang/cargo#11313 ?). Pragmatically, though, perhaps we can make up for shortcomings elsewhere by changes here. I'm still undecided what I think is the right thing to do.

@lopopolo
Copy link
Collaborator Author

As a shorter term fix while we think about the design implications here, I've put up a PR I hope we can release as iana-time-zone-haiku v0.1.2 which replaces cxx and cxx-build with a C ABI-style binding and cc:

@djc
Copy link
Contributor

djc commented May 23, 2023

The rust API is unchanged here. So the semver question is whether requiring downstream to fiddle with Cargo.toml counts as a breaking change. To me, this seems related to the question of whether MSRV bumps should be considered a breaking semver change, to which I think the consensus is no, although debatable.

If functionality in this crate regresses while the feature flags are unchanged, I would consider that a semver violation. If iana-time-zone previously provided support for all platforms with the default features, then changing the default features to only support a subset of platforms is not a semver-compatible change.

I think the energy spent on this would be better directed at a solution at the Cargo level.

@astraw
Copy link
Member

astraw commented May 23, 2023

Yes, I'm inclined to suggest that with #106 the short term pain is minimized and that we should close this issue and direct future energy at the Cargo level. I am closing this now but please re-open if you think we should indeed discuss further.

@astraw astraw closed this May 23, 2023
@lopopolo lopopolo deleted the lopopolo/heavy-platform-opt-out branch December 27, 2023 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tier-1 Rust Tier-1 platform Tier-2 Rust Tier-2 platform Tier-3 Rust Tier-3 platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants