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.

Sign up for GitHub

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(packages-driver): allow defining additional aspects instead #3330

Merged
merged 2 commits into from
Oct 24, 2022
Merged

fix(packages-driver): allow defining additional aspects instead #3330

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a1d5e4d
fix(packages-driver): allow defining additional aspects instead of ov…
JamyDev Oct 21, 2022
98d593e
Update go/tools/gopackagesdriver/main.go
JamyDev Oct 21, 2022
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
4 changes: 3 additions & 1 deletion go/tools/gopackagesdriver/bazel_json_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,13 @@ func (b *BazelJSONBuilder) Build(ctx context.Context, mode LoadMode) ([]string,
return nil, fmt.Errorf("found no labels matching the requests")
}

aspects := append(additionalAspects, rulesGoAspect)
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK the order may be quite significant here.

@linzhp Is this the correct one (and if so, why?)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a syntax simplification assuming the order didn't matter, if it does I'd be happy to change it so rules_go is first.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not saying this order is wrong, I simply don't know the aspect well enough to decide this. If I understood the interaction correctly, the latter aspect would see providers added by the former, but maybe that's just not how it works. :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

If the additional aspects need some information from each other, or from the default aspect, they can specify required_providers in their aspect definition, then Bazel would run those aspect in the order to satisfy the requirement.

@JamyDev did you test the these aspects don't override each other when that have the same output group and provider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is possibility for overlap if you output to the same .pkg.json file as the default aspect. The packagesdriver only considers the .pkg.json files, so long as there's no overlap in these files it's fine. In our (Uber's) case that's not a problem, we output to ruleName.pkg.json (just like the default aspect does) and only output anything when we encounter our custom rule.

Now if the order in the argument here is actually followed this means that if I write a bad custom aspect, it'll get overridden by the rulesGo aspect, unless I directly declare it as a required_provider.


buildArgs := concatStringsArrays([]string{
"--experimental_convenience_symlinks=ignore",
"--ui_event_filters=-info,-stderr",
"--noshow_progress",
"--aspects=" + customAspect,
"--aspects=" + strings.Join(aspects, ","),
"--output_groups=" + b.outputGroupsForMode(mode),
"--keep_going", // Build all possible packages
}, bazelFlags, bazelBuildFlags, labels)
Expand Down
3 changes: 2 additions & 1 deletion go/tools/gopackagesdriver/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,14 @@ var (
// It seems https://github.com/bazelbuild/bazel/issues/3115 isn't fixed when specifying
// the aspect from the command line. Use this trick in the mean time.
rulesGoRepositoryName = getenvDefault("GOPACKAGESDRIVER_RULES_GO_REPOSITORY_NAME", "@io_bazel_rules_go")
rulesGoAspect = rulesGoRepositoryName + "//go/tools/gopackagesdriver:aspect.bzl%go_pkg_info_aspect"
JamyDev marked this conversation as resolved.
Show resolved Hide resolved
bazelBin = getenvDefault("GOPACKAGESDRIVER_BAZEL", "bazel")
bazelFlags = strings.Fields(os.Getenv("GOPACKAGESDRIVER_BAZEL_FLAGS"))
bazelQueryFlags = strings.Fields(os.Getenv("GOPACKAGESDRIVER_BAZEL_QUERY_FLAGS"))
bazelQueryScope = getenvDefault("GOPACKAGESDRIVER_BAZEL_QUERY_SCOPE", "")
bazelBuildFlags = strings.Fields(os.Getenv("GOPACKAGESDRIVER_BAZEL_BUILD_FLAGS"))
workspaceRoot = os.Getenv("BUILD_WORKSPACE_DIRECTORY")
customAspect = getenvDefault("GOPACKAGESDRIVER_BAZEL_ASPECT", rulesGoRepositoryName+"//go/tools/gopackagesdriver:aspect.bzl%go_pkg_info_aspect")
additionalAspects = strings.Fields(os.Getenv("GOPACKAGESDRIVER_BAZEL_ADDTL_ASPECTS"))
additionalKinds = strings.Fields(os.Getenv("GOPACKAGESDRIVER_BAZEL_KINDS"))
emptyResponse = &driverResponse{
NotHandled: false,
Expand Down