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

Fix sorting of relative and @foo.bzl loads #1272

Merged
merged 3 commits into from
Jul 30, 2024
Merged
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
1 change: 1 addition & 0 deletions build/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ go_library(
importpath = "github.com/bazelbuild/buildtools/build",
visibility = ["//visibility:public"],
deps = [
"//labels",
"//tables",
],
)
Expand Down
34 changes: 16 additions & 18 deletions build/rewrite.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"sort"
"strings"

"github.com/bazelbuild/buildtools/labels"
"github.com/bazelbuild/buildtools/tables"
)

Expand Down Expand Up @@ -1081,6 +1082,7 @@ func comparePaths(path1, path2 string) bool {
// If one label has explicit repository path (starts with @), it goes first
// If the packages are different, labels are sorted by package name (empty package goes first)
// If the packages are the same, labels are sorted by their name
// Relative labels go last
func compareLoadLabels(load1Label, load2Label string) bool {
// handle absolute labels with explicit repositories separately to
// make sure they precede absolute and relative labels without repos
Expand All @@ -1092,31 +1094,27 @@ func compareLoadLabels(load1Label, load2Label string) bool {
return isExplicitRepo1
}

// Either both labels have explicit repository names or both don't, compare their packages
// and break ties using file names if necessary
module1Parts := strings.SplitN(load1Label, ":", 2)
package1, filename1 := "", module1Parts[0]
if len(module1Parts) == 2 {
package1, filename1 = module1Parts[0], module1Parts[1]
}
module2Parts := strings.SplitN(load2Label, ":", 2)
package2, filename2 := "", module2Parts[0]
if len(module2Parts) == 2 {
package2, filename2 = module2Parts[0], module2Parts[1]
}
// ensure that relative labels go last by giving them a fake package name
// that sorts after all valid package names
label1 := labels.ParseRelative(load1Label, string(rune(0x7F)))
label2 := labels.ParseRelative(load2Label, string(rune(0x7F)))

// in case both packages are the same, use file names to break ties
if package1 == package2 {
return comparePaths(filename1, filename2)
if label1.Repository != label2.Repository {
return label1.Repository < label2.Repository
}

// in case one of the packages is empty, the empty one goes first
if len(package1) == 0 || len(package2) == 0 {
return len(package1) > 0
if (len(label1.Package) == 0) != (len(label2.Package) == 0) {
return len(label1.Package) == 0
}

// both packages are non-empty and not equal, so compare them
return comparePaths(package1, package2)
if label1.Package != label2.Package {
return comparePaths(label1.Package, label2.Package)
}

// in case both packages are the same, use file names to break ties
return comparePaths(label1.Target, label2.Target)
}

// sortLoadStatements reorders sorts loads lexicographically by the source file,
Expand Down
5 changes: 5 additions & 0 deletions build/testdata/075.golden
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
load("@bar.bzl", "x", "y")
load("@foo//:foo.bzl", "x", "y")
load("//:baa.bzl", "x", "y")
load("//:bac.bzl", "x", "y")
load(":bab.bzl", "x", "y")
5 changes: 5 additions & 0 deletions build/testdata/075.in
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
load("//:bac.bzl", "x", "y")
load(":bab.bzl", "x", "y")
load("//:baa.bzl", "x", "y")
load("@foo//:foo.bzl", "x", "y")
load("@bar.bzl", "x", "y")