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

suggest unlocking locked pkgs that cause dep resolution failures #3970

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 55 additions & 4 deletions compiler-cli/src/dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,23 @@ impl PartialEq for ProvidedPackageSource {
}
}

// Estimates whether the CLI is ran in a CI environment for use in silencing
// certain CLI dialogues.
fn is_ci_env() -> bool {
let ci_vars = [
"CI",
"TRAVIS",
"CIRCLECI",
"GITHUB_ACTIONS",
"GITLAB_CI",
"JENKINS_URL",
"TF_BUILD",
"BITBUCKET_COMMIT",
];

ci_vars.iter().any(|var| std::env::var_os(*var).is_some())
}

fn resolve_versions<Telem: Telemetry>(
runtime: tokio::runtime::Handle,
mode: Mode,
Expand Down Expand Up @@ -691,18 +708,52 @@ fn resolve_versions<Telem: Telemetry>(
}

// Convert provided packages into hex packages for pub-grub resolve
let provided_hex_packages = provided_packages
let provided_hex_packages: HashMap<EcoString, hexpm::Package> = provided_packages
.iter()
.map(|(name, package)| (name.clone(), package.to_hex_package(name)))
.collect();

let resolved = dependency::resolve_versions(
let root_requirements_clone = root_requirements.clone();
let resolved: HashMap<String, Version> = match dependency::resolve_versions(
PackageFetcher::boxed(runtime.clone()),
provided_hex_packages,
provided_hex_packages.clone(),
config.name.clone(),
root_requirements.into_iter(),
&locked,
)?;
) {
Ok(it) => it,
Err(
ref err @ Error::DependencyResolutionFailed {
error: _,
ref locked_conflicts,
},
) => {
// Do not ask the user to unlock conflicts in CI or if they don't exist
if is_ci_env() || locked_conflicts.is_empty() {
return Err(err.clone());
}

if cli::confirm(
"\nSome of these dependencies are locked to specific versions. It may
be possible to find a solution if they are unlocked, would you like
to unlock and try again?",
)? {
unlock_packages(&mut locked, locked_conflicts, manifest)?;

dependency::resolve_versions(
PackageFetcher::boxed(runtime.clone()),
provided_hex_packages,
config.name.clone(),
root_requirements_clone.into_iter(),
&locked,
)?
} else {
return Err(err.clone());
}
}

Err(err) => return Err(err),
};

// Convert the hex packages and local packages into manifest packages
let manifest_packages = runtime.block_on(future::try_join_all(
Expand Down
69 changes: 64 additions & 5 deletions compiler-core/src/dependency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@ where
{
tracing::info!("resolving_versions");
let root_version = Version::new(0, 0, 0);
let requirements =
root_dependencies(dependencies, locked).map_err(Error::dependency_resolution_failed)?;

let requirements = root_dependencies(dependencies, locked)
.map_err(|err| Error::dependency_resolution_failed(err, locked))?;

// Creating a map of all the required packages that have exact versions specified
let exact_deps = &requirements
Expand All @@ -55,7 +56,7 @@ where
root_name.as_str().into(),
root_version,
)
.map_err(Error::dependency_resolution_failed)?
.map_err(|err| Error::dependency_resolution_failed(err, locked))?
.into_iter()
.filter(|(name, _)| name.as_str() != root_name.as_str())
.collect();
Expand Down Expand Up @@ -789,14 +790,72 @@ mod tests {
.unwrap_err();

match err {
Error::DependencyResolutionFailed(msg) => assert_eq!(
msg,
Error::DependencyResolutionFailed{error, locked_conflicts: _} => assert_eq!(
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you need to run the formatter here!

error,
"An unrecoverable error happened while solving dependencies: gleam_stdlib is specified with the requirement `~> 0.1.0`, but it is locked to 0.2.0, which is incompatible."
),
_ => panic!("wrong error: {err}"),
}
}

#[test]
fn resolution_locked_version_doesnt_satisfy_requirements_locked() {
// we're creating a dependency logging v1.4.0 that requires gleam_stdlib v0.40.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// we're creating a dependency logging v1.4.0 that requires gleam_stdlib v0.40.0
// We're creating a dependency logging v1.4.0 that requires gleam_stdlib v0.40.0

let mut requirements: HashMap<String, Dependency> = HashMap::new();
let _ = requirements.insert(
"gleam_stdlib".to_string(),
Dependency {
requirement: Range::new("~> 0.40.0".to_string()),
optional: false,
app: None,
repository: None,
},
);
let mut provided_packages: HashMap<EcoString, hexpm::Package> = HashMap::new();
let _ = provided_packages.insert(
"logging".into(),
hexpm::Package {
name: "logging".to_string(),
repository: "repository".to_string(),
releases: vec![Release {
version: Version::new(1, 4, 0),
requirements: requirements,
retirement_status: None,
outer_checksum: vec![0],
meta: (),
}],
},
);

// now try and resolve versions with gleam_stdlib v0.20.0 in lock.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// now try and resolve versions with gleam_stdlib v0.20.0 in lock.
// Now try and resolve versions with gleam_stdlib v0.20.0 in lock.

let err = resolve_versions(
make_remote(),
provided_packages,
"root_name".into(),
vec![("logging".into(), Range::new(">= 1.3.0 and < 2.0.0".into()))].into_iter(),
&vec![("gleam_stdlib".into(), Version::new(0, 20, 0))]
.into_iter()
.collect(),
)
.unwrap_err();

// expect failure
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// expect failure
// Expect failure

match err {
Error::DependencyResolutionFailed {
error,
locked_conflicts,
} => {
assert_eq!(
error,
format!("Unable to find compatible versions for the version constraints in your gleam.toml.\n\
The conflicting packages are:\n- gleam_stdlib"),
);
assert_eq!(locked_conflicts, vec!["gleam_stdlib"])
}
_ => panic!("wrong error: {err}"),
}
}

#[test]
fn resolution_with_exact_dep() {
let result = resolve_versions(
Expand Down
103 changes: 77 additions & 26 deletions compiler-core/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use pubgrub::package::Package;
use pubgrub::report::DerivationTree;
use pubgrub::version::Version;
use std::borrow::Cow;
use std::collections::HashSet;
use std::collections::{HashMap, HashSet};
use std::env;
use std::fmt::{Debug, Display};
use std::io::Write;
Expand Down Expand Up @@ -235,8 +235,11 @@ file_names.iter().map(|x| x.as_str()).join(", "))]
#[error("Failed to create canonical path for package {0}")]
DependencyCanonicalizationFailed(String),

#[error("Dependency tree resolution failed: {0}")]
DependencyResolutionFailed(String),
#[error("Dependency tree resolution failed: {error}")]
DependencyResolutionFailed {
error: String,
locked_conflicts: Vec<EcoString>,
},

#[error("The package {0} is listed in dependencies and dev-dependencies")]
DuplicateDependency(EcoString),
Expand Down Expand Up @@ -378,7 +381,10 @@ impl Error {
Self::TarFinish(error.to_string())
}

pub fn dependency_resolution_failed(error: ResolutionError) -> Error {
pub fn dependency_resolution_failed(
error: ResolutionError,
locked: &HashMap<EcoString, hexpm::version::Version>,
) -> Error {
fn collect_conflicting_packages<'dt, P: Package, V: Version>(
derivation_tree: &'dt DerivationTree<P, V>,
conflicting_packages: &mut HashSet<&'dt P>,
Expand Down Expand Up @@ -406,54 +412,98 @@ impl Error {
}
}

Self::DependencyResolutionFailed(match error {
match error {
ResolutionError::NoSolution(mut derivation_tree) => {
derivation_tree.collapse_no_versions();

let mut conflicting_packages = HashSet::new();
collect_conflicting_packages(&derivation_tree, &mut conflicting_packages);

wrap_format!("Unable to find compatible versions for \
the version constraints in your gleam.toml. \
The conflicting packages are:
let conflict_names: Vec<EcoString> = conflicting_packages
.iter()
.map(|pkg| (*pkg).to_string().into())
Copy link
Member

Choose a reason for hiding this comment

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

str -> ecostring rather than str -> string -> ecostring please

.collect();

let locked_conflicts: Vec<EcoString> = conflict_names
.iter()
.filter(|name| locked.contains_key(*name))
.cloned()
.collect();

if locked_conflicts.is_empty() {
Error::DependencyResolutionFailed {
error: format!(
"Unable to find compatible versions for the version constraints in your gleam.toml.\n\
The conflicting packages are:\n{}",
conflicting_packages.into_iter().map(|s| format!("- {s}")).join("\n")
),
locked_conflicts,
}
} else {
Error::DependencyResolutionFailed {
error: format!(
"Unable to find compatible versions for the version constraints in your gleam.toml.\n\
The conflicting packages are:\n{}",
locked_conflicts.iter().map(|s| format!("- {s}")).join("\n")
Copy link
Member

Choose a reason for hiding this comment

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

Why are only some of the conflicting packages are listed in this case? Wouldn't we want to show them all? If not we need to update the message to say this is only a subset of them.

),
locked_conflicts,
}

}

{}
",
conflicting_packages.into_iter().map(|s| format!("- {s}")).join("\n"))
}

ResolutionError::ErrorRetrievingDependencies {
package,
version,
source,
} => format!(
"An error occurred while trying to retrieve dependencies of {package}@{version}: {source}",
),
} => {
Error::DependencyResolutionFailed{
error: format!(
"An error occurred while trying to retrieve dependencies of {package}@{version}: {source}"),
locked_conflicts: vec![],
}
}

ResolutionError::DependencyOnTheEmptySet {
package,
version,
dependent,
} => format!(
"{package}@{version} has an impossible dependency on {dependent}",
),
} => {
Error::DependencyResolutionFailed{
error: format!("{package}@{version} has an impossible dependency on {dependent}"),
locked_conflicts: vec![],
}
}

ResolutionError::SelfDependency { package, version } => {
format!("{package}@{version} somehow depends on itself.")
Error::DependencyResolutionFailed{
error: format!("{package}@{version} somehow depends on itself."),
locked_conflicts: vec![],
}
}

ResolutionError::ErrorChoosingPackageVersion(err) => {
format!("Unable to determine package versions: {err}")
Error::DependencyResolutionFailed{
error: format!("Unable to determine package versions: {err}"),
locked_conflicts: vec![],
}
}

ResolutionError::ErrorInShouldCancel(err) => {
format!("Dependency resolution was cancelled. {err}")
Error::DependencyResolutionFailed{
error: format!("Dependency resolution was cancelled. {err}"),
locked_conflicts: vec![],
}
}

ResolutionError::Failure(err) => format!(
"An unrecoverable error happened while solving dependencies: {err}"
),
})
ResolutionError::Failure(err) => {
Error::DependencyResolutionFailed{
error: format!("An unrecoverable error happened while solving dependencies: {err}"),
locked_conflicts: vec![],
}
}
}
}

pub fn expand_tar<E>(error: E) -> Error
Expand Down Expand Up @@ -3552,7 +3602,7 @@ The error from the parser was:
}]
}

Error::DependencyResolutionFailed(error) => {
Error::DependencyResolutionFailed{error, locked_conflicts: _} => {
let text = format!(
"An error occurred while determining what dependency packages and
versions should be downloaded.
Expand All @@ -3568,7 +3618,8 @@ The error from the version resolver library was:
location: None,
level: Level::Error,
}]
}
},


Error::GitDependencyUnsupported => vec![Diagnostic {
title: "Git dependencies are not currently supported".into(),
Expand Down