From 45807800f505f0373eced8a35f177892d8e4d188 Mon Sep 17 00:00:00 2001 From: Dylan Greene Date: Thu, 19 Oct 2023 22:22:27 -0400 Subject: [PATCH 1/4] fix findBestMatch so it finds the best match and not the first match --- src/install/npm.zig | 15 ++++++++++++--- src/install/semver.zig | 7 +++++++ .../install/migration/complex-workspace.test.ts | 3 +++ 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/install/npm.zig b/src/install/npm.zig index 4cf1c2b71af06c..2d4f0d544f92d0 100644 --- a/src/install/npm.zig +++ b/src/install/npm.zig @@ -824,15 +824,24 @@ pub const PackageManifest = struct { { const releases = this.pkg.releases.keys.get(this.versions); var i = releases.len; + + var bestMatchVersion: ?Semver.Version = null; + var bestMatchPackage: ?PackageVersion = null; // For now, this is the dumb way while (i > 0) : (i -= 1) { const version = releases[i - 1]; - const packages = this.pkg.releases.values.get(this.package_versions); - if (group.satisfies(version)) { - return .{ .version = version, .package = &packages[i - 1] }; + // If we find one that matches, save it, but keep looping because we might find a newer match. + // The versions from the registry are not in any particular order. + if (group.satisfies(version) and (bestMatchVersion == null or Semver.Version.gt(version, bestMatchVersion.?))) { + bestMatchVersion = version; + bestMatchPackage = this.pkg.releases.values.get(this.package_versions)[i - 1]; } } + + if (bestMatchVersion != null and bestMatchPackage != null) { + return .{ .version = bestMatchVersion.?, .package = &bestMatchPackage.? }; + } } if (group.flags.isSet(Semver.Query.Group.Flags.pre)) { diff --git a/src/install/semver.zig b/src/install/semver.zig index 9572b85e238e88..e0c840bf781db0 100644 --- a/src/install/semver.zig +++ b/src/install/semver.zig @@ -721,6 +721,13 @@ pub const Version = extern struct { return lhs.major == rhs.major and lhs.minor == rhs.minor and lhs.patch == rhs.patch and rhs.tag.eql(lhs.tag); } + pub fn gt(lhs: Version, rhs: Version) bool { + if (lhs.major < rhs.major) return false; + if (lhs.minor < rhs.minor) return false; + if (lhs.patch < rhs.patch) return false; + return true; + } + pub const HashContext = struct { pub fn hash(_: @This(), lhs: Version) u32 { return @as(u32, @truncate(lhs.hash())); diff --git a/test/cli/install/migration/complex-workspace.test.ts b/test/cli/install/migration/complex-workspace.test.ts index ec4669a3b66c8f..fa9cfe80c2ebb5 100644 --- a/test/cli/install/migration/complex-workspace.test.ts +++ b/test/cli/install/migration/complex-workspace.test.ts @@ -119,3 +119,6 @@ mustNotExist("packages/second/node_modules/body-parser/node_modules/body-parser/ mustNotExist("packages/second/node_modules/body-parser/node_modules/iconv-lite"); mustNotExist("packages/second/node_modules/iconv-lite"); mustNotExist("node_modules/iconv-lite"); + +// Should pick 1.2.4, not 1.2.0. +validate("node_modules/lines-and-columns", "1.2.4"); From b8d96906fab9d42637811fb447d619ecee5a849e Mon Sep 17 00:00:00 2001 From: Dylan Greene Date: Thu, 19 Oct 2023 22:50:01 -0400 Subject: [PATCH 2/4] update complex-workspaces to include lines-and-columns ^1.1.6 --- test/cli/install/migration/complex-workspace/package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/test/cli/install/migration/complex-workspace/package.json b/test/cli/install/migration/complex-workspace/package.json index 55ed302cbeaa03..68a17e643e3ebd 100644 --- a/test/cli/install/migration/complex-workspace/package.json +++ b/test/cli/install/migration/complex-workspace/package.json @@ -7,6 +7,7 @@ "hello": "file:hello-0.3.2.tgz", "install-test": "bitbucket:dylan-conway/public-install-test", "install-test1": "git+ssh://git@github.com/dylan-conway/install-test.git#596234dab30564f37adae1e5c4d7123bcffce537", + "lines-and-columns": "^1.1.6", "public-install-test": "gitlab:dylan-conway/public-install-test", "svelte": "4.1.2" }, From 33a9b0cf2d31c32417ca556cf5a31ad5a18abeab Mon Sep 17 00:00:00 2001 From: Dylan Greene Date: Thu, 19 Oct 2023 23:31:04 -0400 Subject: [PATCH 3/4] PR feedback --- src/install/npm.zig | 16 +++++++++------- src/install/semver.zig | 7 ------- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/src/install/npm.zig b/src/install/npm.zig index 2d4f0d544f92d0..3e107ab3a7b688 100644 --- a/src/install/npm.zig +++ b/src/install/npm.zig @@ -825,22 +825,24 @@ pub const PackageManifest = struct { const releases = this.pkg.releases.keys.get(this.versions); var i = releases.len; - var bestMatchVersion: ?Semver.Version = null; - var bestMatchPackage: ?PackageVersion = null; + var best_match_version: ?Semver.Version = null; + var best_match_package: ?PackageVersion = null; // For now, this is the dumb way while (i > 0) : (i -= 1) { const version = releases[i - 1]; // If we find one that matches, save it, but keep looping because we might find a newer match. // The versions from the registry are not in any particular order. - if (group.satisfies(version) and (bestMatchVersion == null or Semver.Version.gt(version, bestMatchVersion.?))) { - bestMatchVersion = version; - bestMatchPackage = this.pkg.releases.values.get(this.package_versions)[i - 1]; + if (group.satisfies(version) and (best_match_version == null or + (Semver.Version.order(version, best_match_version.?, "", "") == .gt))) + { + best_match_version = version; + best_match_package = this.pkg.releases.values.get(this.package_versions)[i - 1]; } } - if (bestMatchVersion != null and bestMatchPackage != null) { - return .{ .version = bestMatchVersion.?, .package = &bestMatchPackage.? }; + if (best_match_version != null and best_match_package != null) { + return .{ .version = best_match_version.?, .package = &best_match_package.? }; } } diff --git a/src/install/semver.zig b/src/install/semver.zig index e0c840bf781db0..9572b85e238e88 100644 --- a/src/install/semver.zig +++ b/src/install/semver.zig @@ -721,13 +721,6 @@ pub const Version = extern struct { return lhs.major == rhs.major and lhs.minor == rhs.minor and lhs.patch == rhs.patch and rhs.tag.eql(lhs.tag); } - pub fn gt(lhs: Version, rhs: Version) bool { - if (lhs.major < rhs.major) return false; - if (lhs.minor < rhs.minor) return false; - if (lhs.patch < rhs.patch) return false; - return true; - } - pub const HashContext = struct { pub fn hash(_: @This(), lhs: Version) u32 { return @as(u32, @truncate(lhs.hash())); From 528a695aa15b39f41d666ace11714aeaf92c50b9 Mon Sep 17 00:00:00 2001 From: Dylan Greene Date: Fri, 20 Oct 2023 00:01:36 -0400 Subject: [PATCH 4/4] PR feedback --- src/install/npm.zig | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/install/npm.zig b/src/install/npm.zig index 3e107ab3a7b688..c9964e3ba793b8 100644 --- a/src/install/npm.zig +++ b/src/install/npm.zig @@ -823,26 +823,25 @@ pub const PackageManifest = struct { { const releases = this.pkg.releases.keys.get(this.versions); - var i = releases.len; var best_match_version: ?Semver.Version = null; - var best_match_package: ?PackageVersion = null; + var best_match_package_index: usize = 0; + var i = releases.len; // For now, this is the dumb way while (i > 0) : (i -= 1) { const version = releases[i - 1]; // If we find one that matches, save it, but keep looping because we might find a newer match. // The versions from the registry are not in any particular order. - if (group.satisfies(version) and (best_match_version == null or - (Semver.Version.order(version, best_match_version.?, "", "") == .gt))) - { + if (group.satisfies(version) and (best_match_version == null or Semver.Version.order(version, best_match_version.?, "", "") == .gt)) { best_match_version = version; - best_match_package = this.pkg.releases.values.get(this.package_versions)[i - 1]; + best_match_package_index = i - 1; } } - if (best_match_version != null and best_match_package != null) { - return .{ .version = best_match_version.?, .package = &best_match_package.? }; + if (best_match_version != null) { + const packages = this.pkg.releases.values.get(this.package_versions); + return .{ .version = best_match_version.?, .package = &packages[best_match_package_index] }; } }