From e9b76f455b5e3791669039d2b6416479e48c3ace Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Thu, 19 Nov 2020 17:43:24 -0500 Subject: [PATCH] link: make package conflicts into errors instead of warnings We've promised to do this for more than two years, and the time has finally come. If more than one copy of a package with the same package path is provided to the linker, the GoLink action now reports an error. Distinct packages (for example, from multiple vendor directories) may be disambiguated with the importmap attribute. Fixes #1374 --- go/config/BUILD.bazel | 7 ----- go/private/actions/link.bzl | 5 +--- go/private/context.bzl | 8 ------ go/tools/builders/importcfg.go | 6 +--- go/tools/builders/link.go | 11 +------- tests/core/go_binary/package_conflict_test.go | 28 ++----------------- 6 files changed, 6 insertions(+), 59 deletions(-) diff --git a/go/config/BUILD.bazel b/go/config/BUILD.bazel index b95ba5cc60..b832742767 100644 --- a/go/config/BUILD.bazel +++ b/go/config/BUILD.bazel @@ -10,13 +10,6 @@ load( "LINKMODE_NORMAL", ) -bool_flag( - name = "incompatible_package_conflict_is_error", - # TODO(#1374): Flip in v0.25. - build_setting_default = False, - visibility = ["//visibility:public"], -) - bool_flag( name = "static", build_setting_default = False, diff --git a/go/private/actions/link.bzl b/go/private/actions/link.bzl index de1a9ab33c..9139430203 100644 --- a/go/private/actions/link.bzl +++ b/go/private/actions/link.bzl @@ -153,11 +153,8 @@ def emit_link( tool_args.add_joined("-extldflags", extldflags, join_with = " ") conflict_err = _check_conflicts(arcs) - if go._package_conflict_is_error: - builder_args.add("-package_conflict_is_error") if conflict_err: - # Error is reported if -package_conflict_is_error was set, or if the - # linker command fails. + # Report package conflict errors in execution instead of analysis. # We could call fail() with this message, but Bazel prints a stack # that doesn't give useful information. builder_args.add("-conflict_err", conflict_err) diff --git a/go/private/context.bzl b/go/private/context.bzl index 9b182a2404..5fa52386ae 100644 --- a/go/private/context.bzl +++ b/go/private/context.bzl @@ -490,8 +490,6 @@ def go_context(ctx, attr = None): # Private # TODO: All uses of this should be removed _ctx = ctx, - # TODO(#1374): Remove in v0.25. - _package_conflict_is_error = go_config_info._package_conflict_is_error if go_config_info else True, ) def _go_context_data_impl(ctx): @@ -767,9 +765,6 @@ def _go_config_impl(ctx): linkmode = ctx.attr.linkmode[BuildSettingInfo].value, tags = ctx.attr.gotags[BuildSettingInfo].value, stamp = ctx.attr.stamp, - - # TODO(#1374): Remove in v0.25. - _package_conflict_is_error = ctx.attr._package_conflict_is_error[BuildSettingInfo].value, )] go_config = rule( @@ -808,9 +803,6 @@ go_config = rule( providers = [BuildSettingInfo], ), "stamp": attr.bool(mandatory = True), - "_package_conflict_is_error": attr.label( - default = "//go/config:incompatible_package_conflict_is_error", - ), }, provides = [GoConfigInfo], doc = """Collects information about build settings in the current diff --git a/go/tools/builders/importcfg.go b/go/tools/builders/importcfg.go index da80f53dcb..2cfaab1191 100644 --- a/go/tools/builders/importcfg.go +++ b/go/tools/builders/importcfg.go @@ -172,11 +172,7 @@ func buildImportcfgFileForLink(archives []archive, stdPackageListPath, installSu depsSeen := map[string]string{} for _, arc := range archives { if _, ok := depsSeen[arc.packagePath]; ok { - // If this is detected during analysis, the -conflict_err flag will be set. - // We'll report that error if -package_conflict_is_error is set or if - // the link command fails. - // TODO(#1374): This should always be an error. Panic. - continue + return "", fmt.Errorf("internal error: package %s provided multiple times. This should have been detected during analysis.", arc.packagePath) } depsSeen[arc.packagePath] = arc.label fmt.Fprintf(buf, "packagefile %s=%s\n", arc.packagePath, arc.file) diff --git a/go/tools/builders/link.go b/go/tools/builders/link.go index 71f9559c7a..edb8bf52f0 100644 --- a/go/tools/builders/link.go +++ b/go/tools/builders/link.go @@ -51,7 +51,6 @@ func link(args []string) error { flags.Var(&xdefs, "X", "A string variable to replace in the linked binary (repeated).") flags.Var(&xstamps, "Xstamp", "Like -X but the values are looked up in the -stamp file.") flags.Var(&stamps, "stamp", "The name of a file with stamping values.") - packageConflictIsError := flags.Bool("package_conflict_is_error", false, "Whether importpath conflicts are errors.") conflictErrMsg := flags.String("conflict_err", "", "Error message about conflicts to report if there's a link error.") if err := flags.Parse(builderArgs); err != nil { return err @@ -60,7 +59,7 @@ func link(args []string) error { return err } - if *packageConflictIsError && *conflictErrMsg != "" { + if *conflictErrMsg != "" { return errors.New(*conflictErrMsg) } @@ -149,16 +148,8 @@ func link(args []string) error { goargs = append(goargs, toolArgs...) goargs = append(goargs, *main) if err := goenv.runCommand(goargs); err != nil { - if *conflictErrMsg != "" { - // TODO(#1374): this should always be reported above. - err = fmt.Errorf("%v\n%s", err, *conflictErrMsg) - } return err } - if *conflictErrMsg != "" { - // TODO(#1374): this should always be reported above. - fmt.Fprintf(os.Stderr, "%s\nThis will be an error in the future.\n", *conflictErrMsg) - } if *buildmode == "c-archive" { if err := stripArMetadata(*outFile); err != nil { diff --git a/tests/core/go_binary/package_conflict_test.go b/tests/core/go_binary/package_conflict_test.go index b51ab16508..9ec6340a89 100644 --- a/tests/core/go_binary/package_conflict_test.go +++ b/tests/core/go_binary/package_conflict_test.go @@ -113,30 +113,8 @@ func main() { }) } -func runTest(t *testing.T, expectError bool, extraArgs ...string) { - args := append([]string{"build", "//:main"}, extraArgs...) - - err := bazel_testing.RunBazel(args...) - if expectError { - if err == nil { - t.Fatal("Expected error") - } - } else { - if err != nil { - t.Fatal(err) - } +func TestPackageConflict(t *testing.T) { + if err := bazel_testing.RunBazel("build", "//:main"); err == nil { + t.Fatal("Expected error") } } - -func TestDefaultBehaviour(t *testing.T) { - // TODO(#1374): Default to `true`. - runTest(t, false) -} - -func TestPackageConflictIsWarning(t *testing.T) { - runTest(t, false, "--@io_bazel_rules_go//go/config:incompatible_package_conflict_is_error=False") -} - -func TestPackageConflictIsError(t *testing.T) { - runTest(t, true, "--@io_bazel_rules_go//go/config:incompatible_package_conflict_is_error=True") -}