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

imageproc support: include feature/dependency for a the sic_image_crate package #395

Closed
foresterre opened this issue May 9, 2020 · 3 comments · Fixed by #410
Closed
Labels
A-crate-imageproc Area: changes related to the imageproc crate
Milestone

Comments

@foresterre
Copy link
Owner

foresterre commented May 9, 2020

✅ Cargo supports transitive conditional features just one level deep (See workspace-features-experiment, #395 (comment) and #410)


I would like to add imageproc operations to sic. For this to happen we need to let sic_image_engine be dependent on imageproc. imageproc however pulls in a lot of code, so for now I would like to be able to build with and without imageproc support. Thus, I wanted to add a feature to sic_image_engine: imageproc-ops = ["imageproc"] and under dependencies imageproc = { version = "...", optional = true }`.

Now, if I can't build sic from the root workspace;

Attempts

Branch: https://github.com/foresterre/sic/tree/experiment/gated-imageproc-feature - (perm link)

(A) Attempt: define feature from workspace root directly

  1. cargo run --features imageproc-ops -- -i .\resources\1x1_a.png -o target/o.jpg

error: Package sic v0.11.0 (\ws\sic) does not have these features: imageproc-ops

  1. if the root crate also defines the above feature in its manifest, it will only be enabled for the root crate (which sounds logical to me), not the sic_image_engine package.

  2. cargo build --workspace --features 'imageproc-ops'; without imageproc-ops feature in the root results in the same "missing feature" error (A,1); with the empty feature it results in (A,2)

  3. cargo +nightly build --features 'sic_image_ops/imageproc-ops' -Z package-features

error: none of the selected packages contains these features: sic_image_ops/imageproc-ops

(B) Attempt: build with multiple packages

  1. cargo +nightly run --bin sic -p sic -p sic_cli -p sic_cli_ops -p sic_core -p sic_io -p sic_parser -p sic_testing -p sic_image_engine --features imageproc-ops -Z package-features -- -i .\resources\1x1_a.png -o target/o.jpg

error: The argument '--package ' was provided more than once, but cannot be used multiple times
happens both with this nightly flag and without (it was a trial and error attempt)

(C) Attempt: add feature to every depending package in package manifest
i.e.

[dependencies]
sic_image_engine = { path = "../../components/sic_image_engine", version = "0.11", features = ["imageproc-ops"] }

src: https://github.com/foresterre/sic/blob/experiment/gated-imageproc-feature/components/sic_cli/Cargo.imageproc-ops.toml

This works, but is very inconvenient, as we'll need to change the manifests of several packages for every custom build where we would like to include the imageproc features ...

Workarounds

Second custom manifest

  • Create a second custom manifest in sic_cli called Cargo.imageproc-ops.toml, and replace the Cargo.toml with this version temporarily when building with the imageproc feature:

create script or cargo-ws(e) [cargo workspace-enhancer <opts...>] if we need to do this a lot

sic_cli_ops = { path = "../../components/sic_cli_ops", version = "0.11.0", features = ["imageproc-ops"] }
sic_core = { path = "../../components/sic_core", version = "0.11" }
sic_io  = { path = "../../components/sic_io", version = "0.11" }
sic_image_engine = { path = "../../components/sic_image_engine", version = "0.11", features = ["imageproc-ops"] }
sic_parser = { path = "../../components/sic_parser", version = "0.11", features = ["imageproc-ops"] }

Open Questions?

  • Say, cargo would add the possibility to enable a feature for a single crate to unstable features; can we somehow use nightly cargo with a stable toolchain (alternatively, build only the imageproc version with a nightly toolchain)

Related


Similarly, sic_parser and sic_cli_ops should add the feature like so: imageproc-ops = []

foresterre added a commit to foresterre/workspace-features-experiment that referenced this issue May 9, 2020
@foresterre
Copy link
Owner Author

status: works for https://github.com/foresterre/workspace-features-experiment, but not sic ... (?)

@foresterre
Copy link
Owner Author

foresterre commented May 11, 2020

It works, but only for a crate one level deep from the crate where the action is invoked (i.e. the crate where the feature is set must directly depend on the crate where a cargo command is invoked).
In our case this means that sic_image_engine must be a direct dependency on sic (lib); the root crate which includes our main() function. For this to happen, I had to move the sic_cli crate back into sic itself.

It seems that --features <sub>/<sub-feature> works where <sub> is a package dependent on <package dir from where cargo is invoked>, instead of <sub> being a workspace member in a certain workspace.

See 2360a22
Documented here: rust-lang/cargo#2851 (comment)
Commit: https://github.com/bennofs/cargo/commit/6f28ef2dbb2c5992803a297fc0b2ec68173539d3

@foresterre
Copy link
Owner Author

foresterre commented May 15, 2020

Additional workspace support is being discussed in a new RFC (rust-lang/rfcs#2906), currently in its final comment period, which may also (partially?) solve the above.

I'm not sure whether it will actually solve the full problem as we need a dependency on imageproc only in the sic_image_engine crate, but sic_image_engine, sic_parser and sic_cli_ops need feature flags to support imageproc, yet those two out of those three crates do not need to directly depend on the imageproc crate. If this proposal gets accepted, it may be a welcome change for feature flags which do not take dependencies like our output-test-images-for-manual-inspection, however for features which do take dependencies it may pollute the dependency tree. However since all depending versions would be the same, and Rust usually links statically, this may not increase the actual final root binary size (assuming we have a single binary crate in our workspace).

See also rust-lang/rfcs#2906 (comment)

@foresterre foresterre changed the title imageproc support: setting a feature for a certain package only seems not supported (yet?) imageproc support: include feature/dependency for a the sic_image_crate package May 20, 2020
@foresterre foresterre added this to the 0.12.0 milestone May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-crate-imageproc Area: changes related to the imageproc crate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant