Skip to content

Commit

Permalink
fix: ignore phantom targets and ensure unique target label names (#401)
Browse files Browse the repository at this point in the history
- The description manifests from SPM can contain undeclared targets
(i.e. phantom targets). We ignore these when constructing a Bazel build
file.
- It is critical that the Bazel label name for an SPM target not
conflict with the label name from a product. Typically, we do this by
using the path of the SPM target as the basis for the label name. This
PR fixes a bug where a non-unique label name was generated when an SPM
target's name and path were equal (i.e., no parent directory in the
path).

Related to #400.
  • Loading branch information
cgrindel authored Jun 7, 2023
1 parent 3f450f4 commit 4c39a9e
Show file tree
Hide file tree
Showing 6 changed files with 77 additions and 12 deletions.
5 changes: 4 additions & 1 deletion gazelle/internal/swift/bazel_label.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,13 @@ import (
)

// BazelLabelFromTarget creates a Bazel label from a Swift target.
// The logic in this function must stay in sync with
// pkginfo_targets.bazel_label_name_from_parts() in the Starlark code.
func BazelLabelFromTarget(repoName string, target *swiftpkg.Target) *label.Label {
var name string
basename := path.Base(target.Path)
if basename == target.Name {
dirname := path.Dir(target.Path)
if basename == target.Name && dirname != "." {
name = target.Path
} else {
name = path.Join(target.Path, target.Name)
Expand Down
38 changes: 31 additions & 7 deletions gazelle/internal/swift/bazel_label_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,36 @@ import (
)

func TestBazelLabelFromTarget(t *testing.T) {
target := &swiftpkg.Target{
Name: "Foo",
Path: "Sources/Foo",
tests := []struct {
msg string
repoName string
targetName string
targetPath string
exp string
}{
{
msg: "regular path",
repoName: "example_cool_repo",
targetName: "Foo",
targetPath: "Sources/Foo",
exp: "@example_cool_repo//:Sources_Foo",
},
{
msg: "simple path",
repoName: "example_cool_repo",
targetName: "simple_path",
targetPath: "simple_path",
exp: "@example_cool_repo//:simple_path_simple_path",
},
}
for _, tt := range tests {
target := &swiftpkg.Target{
Name: tt.targetName,
Path: tt.targetPath,
}
actual := swift.BazelLabelFromTarget(tt.repoName, target)
expected, err := label.Parse(tt.exp)
assert.NoError(t, err)
assert.Equal(t, &expected, actual, tt.msg)
}
actual := swift.BazelLabelFromTarget("example_cool_repo", target)
expected, err := label.Parse("@example_cool_repo//:Sources_Foo")
assert.NoError(t, err)
assert.Equal(t, &expected, actual)
}
3 changes: 2 additions & 1 deletion gazelle/internal/swiftpkg/package_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ func NewPackageInfo(sw swiftbin.Executor, dir string) (*PackageInfo, error) {
for _, descT := range descManifest.Targets {
dumpT := dumpManifest.Targets.FindByName(descT.Name)
if dumpT == nil {
return nil, fmt.Errorf("dump manifest info for %s target not found", descT.Name)
// Ignore phantom targets (i.e., appear in description but not in the dump)
continue
}
t, err := NewTargetFromManifestInfo(&descT, dumpT, prodNames)
if err != nil {
Expand Down
20 changes: 19 additions & 1 deletion swiftpkg/internal/pkginfo_targets.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,29 @@ def _join_path(target, path):
return _join_path_from_parts(target.path, path)

def _bazel_label_name_from_parts(target_path, target_name):
"""Create a Bazel label name from a target path and name.
The logic in this function must stay in sync with BazelLabelFromTarget() in
bazel_label.go.
Args:
target_path: The target's path as a `string`.
target_name: The target's name as a `string`.
Returns:
A Bazel label name as a `string`.
"""
basename = paths.basename(target_path)
if basename == target_name:
dirname = paths.dirname(target_path)

# We are trying to construct the shortest, unique target name. We need to
# be sure that a target label does not conflict with a product label with
# the same name.
if basename == target_name and dirname != "":
name = target_path
else:
name = _join_path_from_parts(target_path, target_name)

return name.replace("/", "_")

def _bazel_label_name(target):
Expand Down
9 changes: 8 additions & 1 deletion swiftpkg/internal/pkginfos.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,19 @@ def _get(repository_ctx, directory, deps_index, env = {}):
env = env,
working_directory = directory,
)
return _new_from_parsed_json(
pkg_info = _new_from_parsed_json(
dump_manifest = dump_manifest,
desc_manifest = desc_manifest,
repo_name = repository_utils.package_name(repository_ctx),
deps_index = deps_index,
)

# Dump the merged pkg_info for debug purposes
json_str = json.encode_indent(pkg_info, indent = " ")
repository_ctx.file("pkg_info.json", content = json_str, executable = False)

return pkg_info

def _new_dependency_requirement_from_desc_json_map(req_map):
ranges = req_map.get("range")
if ranges != None:
Expand Down Expand Up @@ -204,6 +210,7 @@ def _new_target_from_json_maps(dump_map, desc_map, repo_name, deps_index):
if dep_module == None:
return None
product_memberships = dep_module.product_memberships

if len(product_memberships) == 0:
return None
dependencies = [
Expand Down
14 changes: 13 additions & 1 deletion swiftpkg/tests/pkginfo_targets_tests.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ _chocolate_target = pkginfos.new_target(
sources = [],
dependencies = [],
)

_dot_path_target = pkginfos.new_target(
name = "DotPath",
type = target_types.library,
Expand All @@ -51,6 +50,15 @@ _dot_path_target = pkginfos.new_target(
sources = ["Chicken.swift", "Smidgen/Hello.swift"],
dependencies = [],
)
_simple_path_target = pkginfos.new_target(
name = "simple_path",
type = target_types.library,
c99name = "simple_path",
module_type = module_types.swift,
path = "simple_path",
sources = ["Simple.swift"],
dependencies = [],
)

def _get_test(ctx):
env = unittest.begin(ctx)
Expand Down Expand Up @@ -125,6 +133,10 @@ def _bazel_label_name_test(ctx):
expected = "DotPath"
asserts.equals(env, expected, actual)

actual = pkginfo_targets.bazel_label_name(_simple_path_target)
expected = "simple_path_simple_path"
asserts.equals(env, expected, actual)

return unittest.end(env)

bazel_label_name_test = unittest.make(_bazel_label_name_test)
Expand Down

0 comments on commit 4c39a9e

Please sign in to comment.