Bazel: incompatible_package_name_is_a_function: Remove PACKAGE_NAME and REPOSITORY_NAME

[c-parsons]

This is a tracking issue for offering a migration solution for
--incompatible_package_name_is_a_function

This flag disables use of the skylark constants PACKAGE_NAME and REPOSITORY_NAME. Users should use package_name() and repository_name() instead.

Migration

1. replace PACKAGE_NAME with package_name() call
2. replace REPOSITORY_NAME with repository_name() call

The behavior is identical

[laurentlb]

Timeline:

• New functions added in May 2017 (https://github.com/bazelbuild/bazel/commit/ba51cda99733a9facb9479683755944229f63ee3)
• Old names officially marked as deprecated in July 2017 (https://github.com/bazelbuild/bazel/commit/286f1e9839f34efc910ce49f040bac23716f8520)
• Flag to disable the old names added in March 2018 (https://github.com/bazelbuild/bazel/commit/1cbce0fad88d02c76386e8525e03d0f427515ad2)
• Deprecation announced in Bazel 0.13 blog post (https://blog.bazel.build/2018/05/02/bazel-0.13.html)

[benjaminp]

I hope major Bazel-using packages will release fixed versions before this deprecation is finished. The latest release of protobuf is still using PACKAGE_NAME, though it has been fixed on master.

[meteorcloudy]

https://github.com/bazelbuild/bazel/commit/4a8bacd33d96b0499bb4d8303d285133268c01b4 is causing many projects to fail in bazel downstream job.
See https://buildkite.com/bazel/bazel-with-downstream-projects-bazel/builds/433#fc709679-63bc-4517-a4fb-02f5d8cbed2b

Most of the failing projects depends on protobuf and have the following error message:

ERROR: /var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/daf61e1fe31310589cae072c149a6fc3/external/com_google_protobuf/BUILD:573:1: Traceback (most recent call last):
--
  | File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/daf61e1fe31310589cae072c149a6fc3/external/com_google_protobuf/BUILD", line 573
  | internal_gen_well_known_protos_java(srcs = WELL_KNOWN_PROTOS)
  | File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/daf61e1fe31310589cae072c149a6fc3/external/com_google_protobuf/protobuf.bzl", line 269, in internal_gen_well_known_protos_java
  | Label(("%s//protobuf_java" % REPOSITOR...))
  | File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/daf61e1fe31310589cae072c149a6fc3/external/com_google_protobuf/protobuf.bzl", line 269, in Label
  | REPOSITORY_NAME
  | The value 'REPOSITORY_NAME' has been removed in favor of 'repository_name()', please use the latter

As @benjaminp pointed out, the release version of protobuf still uses PACKAGE_NAME, should we rollback https://github.com/bazelbuild/bazel/commit/4a8bacd33d96b0499bb4d8303d285133268c01b4 or use --incompatible_package_name_is_a_function=false for all the failing projects?

[meteorcloudy]

@davido Looks like Gerrit still use PACKAGE_NAME in its bzl file

ERROR: /Users/buildkite/builds/buildkite-imacpro-6-1/bazel-downstream-projects/gerrit/gerrit-gwtui/BUILD:23:1: Traceback (most recent call last):
--
  | File "/Users/buildkite/builds/buildkite-imacpro-6-1/bazel-downstream-projects/gerrit/gerrit-gwtui/BUILD", line 23
  | license_test(name = "ui_module_license_test", t...")
  | File "/Users/buildkite/builds/buildkite-imacpro-6-1/bazel-downstream-projects/gerrit/tools/bzl/license.bzl", line 42, in license_test
  | PACKAGE_NAME
  | The value 'PACKAGE_NAME' has been removed in favor of 'package_name()', please use the latter

Can you help fixing that?

[hlopko]

cc @aehlig this is the reason why the bazel with downstream projects is in such a bad shape for release.
@laurentlb @c-parsons what is the best course of action here? Is it an option to postpone flipping the flag until 0.19? Philosophically this is a protobuf issue. Practically half of the bazel world will be broken.

[laurentlb]

Rollback commit is being submitted.

[davido]

@meteorcloudy

@davido Looks like Gerrit still use PACKAGE_NAME in its bzl file
[...]
Can you help fixing that?

Done.

[dslomov]

What is missing for migration: migration docs, length of migration window. After these are done, please add "migration-ready" label.

[laurentlb]

To automatically fix the code and migrate, use Buildifier:

buildifier --lint=fix --warnings=package-name,repository-name

[laurentlb]

@dslomov Why did you remove the migration-ready label?

[jin]

@laurentlb Protobuf was just tagged with 3.6.1.2 , but the tag has not been promoted to a release yet.. The latest protobuf release is still 3.6.1, which still breaks on this incompatible change. Why is the incompatible flag removed already?

Also: all android_instrumentation_test targets depend on github.com/android/android-test, which is still using Protobuf 3.4.1. However, it's not trivial to just update to 3.6.1.2 (or any later Protobuf version for that matter), because each project may hit different breaking changes within Protobuf upgrades themselves. We currently use the --incompatible_ flag to have a workaround to keep the tests passing (which can reveal other breakages) until the issues are fully resolved.

cc @brettchabot

[laurentlb]

Bazel 0.21 still has the flag. Bazel 0.22 won't have the flag, but it will be released end of January.

[meteorcloudy]

@laurentlb Is it too soon to remove this flag from HEAD? Because if we do this, there's no way we can test Android in downstream. The cannot use this flag as a workaround, nor can they upgrade protobuf (until 3.7 came out). But I'd like to keep Android Testing in downstream to catch other errors.
Does --incompatible_package_name_is_a_function have to be removed in 0.22.0?

[meteorcloudy]

Good news! It turned it's fine to upgrade android_test to protobuf 3.6.1.2. See my PR https://github.com/android/android-test/pull/147 and tested with android testing at https://github.com/googlesamples/android-testing/pull/227

[meteorcloudy]

@jin, can you make sure https://github.com/android/android-test/pull/147 get merged and update the ATS commit in android_testing?

[jin]

@meteorcloud wow thanks for looking into this! I’ll follow up on your work.

[Reflexe]

Right now it just fails with the the ambiguous error "builtin variable 'PACKAGE_NAME' is referenced before assignment.". shouldn't it be a more descriptive error?

[duderino]

+1 to @Reflexe's concerns. I just hit this trying to build https://github.com/istio/proxy on macos and found the error message extremely cryptic.

$ bazel version
WARNING: The following rc files are no longer being read, please transfer their contents or import their path into one of the standard rc files:
/Users/jblatt/src/istio.io/proxy/tools/bazel.rc
INFO: Invocation ID: 2895dafb-6e22-482a-b14a-46d22c124722
Build label: 0.22.0
Build target: bazel-out/darwin-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Mon Jan 28 13:00:18 2019 (1548680418)
Build timestamp: 1548680418
Build timestamp as int: 1548680418

ERROR: /Users/jblatt/src/istio.io/proxy/src/envoy/utils/BUILD:24:1: Traceback (most recent call last):
    File "/Users/jblatt/src/istio.io/proxy/src/envoy/utils/BUILD", line 24
        envoy_cc_library(name = "authn_lib", srcs = ["authn..."], <4 more arguments>)
    File "/private/var/tmp/_bazel_jblatt/559fb9825058e8a27a59dd1256731e92/external/envoy/bazel/envoy_build_system.bzl", line 148, in envoy_cc_library
        native.cc_library(name = name, srcs = srcs, hdrs = hdr..., <9 more arguments>)
    File "/private/var/tmp/_bazel_jblatt/559fb9825058e8a27a59dd1256731e92/external/envoy/bazel/envoy_build_system.bzl", line 162, in native.cc_library
        envoy_include_prefix(PACKAGE_NAME)
    File "/private/var/tmp/_bazel_jblatt/559fb9825058e8a27a59dd1256731e92/external/envoy/bazel/envoy_build_system.bzl", line 162, in envoy_include_prefix
        PACKAGE_NAME
builtin variable 'PACKAGE_NAME' is referenced before assignment.