-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Don't require dev-dependencies when not needed in certain cases #5012
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
Changes from 3 commits
7de30dd
ee78456
3fc0715
df5f7d6
da41b4e
9c5eecd
31f6f84
70170d1
d46db71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,7 @@ use std::sync::Arc; | |
|
|
||
| use core::{Source, Package, Target}; | ||
| use core::{Profile, TargetKind, Profiles, Workspace, PackageId, PackageIdSpec}; | ||
| use core::resolver::Resolve; | ||
| use core::resolver::{Resolve, Method}; | ||
| use ops::{self, BuildOutput, Executor, DefaultExecutor}; | ||
| use util::config::Config; | ||
| use util::{CargoResult, profile}; | ||
|
|
@@ -226,12 +226,18 @@ pub fn compile_ws<'a>(ws: &Workspace<'a>, | |
| let profiles = ws.profiles(); | ||
|
|
||
| let specs = spec.into_package_id_specs(ws)?; | ||
| let resolve = ops::resolve_ws_precisely(ws, | ||
| source, | ||
| features, | ||
| all_features, | ||
| no_default_features, | ||
| &specs)?; | ||
| let features = Method::split_features(features); | ||
| let method = Method::Required { | ||
| dev_deps: ws.require_optional_deps() || filter.need_dev_deps(), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps the name Additionally, I think that we'd want this flag to affect target-specific dependencies, right? If this is a sort of "one shot" compilation there should also be no need to resolve and require windows-specific dependencies when on Linux, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you mind if we leave that as a TODO for the future? What you say makes sense but it is not actually needed in Debian right now, because we union over all targets when translating dependencies - so e.g. winapi etc are all pulled in regardless of what target you're building for. This means we have to have less special-cases in the packaging, and also supports cross-compilation later. If I understood correctly, on Fedora they are patching out windows dependencies so they don't appear in Cargo.toml, so this functionality wouldn't be needed either. (Perhaps @ignatenkobrain can confirm).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok sure yeah, I think a TODO should suffice!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm unsure if it's appropriate to mention "targets" in the sense of "platform" here on this line. The term "target" in the context of CompileFilter and OTOH the functionality you mention does seem to be missing - if I add
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm ok, perhaps an issue can be filed? I think the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed as #5133 |
||
| features: &features, | ||
| all_features, | ||
| uses_default_features: !no_default_features, | ||
| }; | ||
| let resolve = ops::resolve_ws_with_method(ws, | ||
| source, | ||
| method, | ||
| &specs, | ||
| )?; | ||
| let (packages, resolve_with_overrides) = resolve; | ||
|
|
||
| if specs.is_empty() { | ||
|
|
@@ -413,9 +419,21 @@ impl<'a> CompileFilter<'a> { | |
| } | ||
| } | ||
|
|
||
| pub fn need_dev_deps(&self) -> bool { | ||
| match *self { | ||
| CompileFilter::Default { .. } => false, | ||
| CompileFilter::Only { examples, tests, benches, .. } => | ||
| examples.is_specific() || tests.is_specific() || benches.is_specific() | ||
| } | ||
| } | ||
|
|
||
| pub fn matches(&self, target: &Target) -> bool { | ||
| match *self { | ||
| CompileFilter::Default { .. } => true, | ||
| CompileFilter::Default { .. } => match *target.kind() { | ||
| TargetKind::Bin => true, | ||
| TargetKind::Lib(..) => true, | ||
| _ => false, | ||
| }, | ||
|
||
| CompileFilter::Only { lib, bins, examples, tests, benches, .. } => { | ||
| let rule = match *target.kind() { | ||
| TargetKind::Bin => bins, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -175,7 +175,11 @@ fn install_one(root: &Filesystem, | |
|
|
||
| let ws = match overidden_target_dir { | ||
| Some(dir) => Workspace::ephemeral(pkg, config, Some(dir), false)?, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the optional deps flag be set for this as well?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ephemeral() already sets that internally |
||
| None => Workspace::new(pkg.manifest_path(), config)?, | ||
| None => { | ||
| let mut ws = Workspace::new(pkg.manifest_path(), config)?; | ||
| ws.set_require_optional_deps(false); | ||
| ws | ||
| } | ||
| }; | ||
| let pkg = ws.current()?; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this start out as a
-Zflag to be unstable by default?