Skip to content

Commit

Permalink
chore: remove comments on BUILD licenses() (#4112)
Browse files Browse the repository at this point in the history
Google internally now recommends avoiding comments on the
`licenses()` rule, arguing that they cause confusion and easily
become out of sync with the source-of-truth LICENSE file.

This adds a CI check in the GitHub Actions workflow to check
for license comments.

Tested that the CI action works in the test PR:
#4113
https://github.com/tensorflow/tensorboard/pull/4113/checks?check_run_id=1053651164

See also
https://opensource.google/docs/thirdparty/linter/#build-licenses-comment%20.no-toc
Googlers: cl/326102766
  • Loading branch information
psybuzz authored Sep 1, 2020
1 parent e5c1df3 commit ea2da3b
Show file tree
Hide file tree
Showing 9 changed files with 60 additions and 8 deletions.
14 changes: 12 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ on:
pull_request: {}

env:
BUILDIFIER: '3.0.0'
BUILDTOOLS_VERSION: '3.0.0'
BUILDIFIER_SHA256SUM: 'e92a6793c7134c5431c58fbc34700664f101e5c9b1c1fcd93b97978e8b7f88db'
BUILDOZER_SHA256SUM: '3d58a0b6972e4535718cdd6c12778170ea7382de7c75bc3728f5719437ffb84d'

jobs:
lint-python-flake8:
Expand Down Expand Up @@ -113,8 +114,12 @@ jobs:
- uses: actions/checkout@v1
- name: 'Set up Buildifier'
run: |
ci/download_buildifier.sh "${BUILDIFIER}" "${BUILDIFIER_SHA256SUM}" ~/buildifier
ci/download_buildifier.sh "${BUILDTOOLS_VERSION}" "${BUILDIFIER_SHA256SUM}" ~/buildifier
sudo mv ~/buildifier /usr/local/bin/buildifier
- name: 'Set up Buildozer'
run: |
ci/download_buildozer.sh "${BUILDTOOLS_VERSION}" "${BUILDOZER_SHA256SUM}" ~/buildozer
sudo mv ~/buildozer /usr/local/bin/buildozer
- name: 'Lint BUILD files'
# TODO(tensorboard-team): address all lint warnings and remove the exemption.
run:
Expand All @@ -125,6 +130,11 @@ jobs:
# Use | to start a literal so YAML doesn't complain about the '!' character.
run: |
! git grep 'python_version = "PY2"' '*BUILD'
- name: 'No comments on licenses rule'
# Assert buildozer error code for 'success, when no changes were made'.
# https://github.com/bazelbuild/buildtools/blob/master/buildozer/README.md#error-code
run: |
buildozer '//tensorboard/...:%licenses' remove_comment && false || test $? = 3
lint-proto:
runs-on: ubuntu-16.04
Expand Down
2 changes: 1 addition & 1 deletion BUILD
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
load("@npm_bazel_typescript//:index.bzl", "ts_config")

licenses(["notice"]) # Apache 2.0
licenses(["notice"])

exports_files(["tsconfig.json"])

Expand Down
2 changes: 1 addition & 1 deletion ci/download_buildifier.sh
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ die() {
}

if [ "$#" -ne 3 ]; then
die "Usage: $0 <version> <sha256sum> <destination-file>"
die "Usage: $0 <buildtools-version> <sha256sum> <destination-file>"
fi

version="$1"
Expand Down
38 changes: 38 additions & 0 deletions ci/download_buildozer.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#!/bin/sh
# Copyright 2020 The TensorFlow Authors. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
# ==============================================================================

# Script to download Buildozer binary directly onto a build machine.

set -e

die() {
printf >&2 "%s\n" "$1"
exit 1
}

if [ "$#" -ne 3 ]; then
die "Usage: $0 <buildtools-version> <sha256sum> <destination-file>"
fi

version="$1"
checksum="$2"
dest="$3"

mirror_url="http://mirror.tensorflow.org/github.com/bazelbuild/buildtools/releases/download/${version}/buildozer"
github_url="https://github.com/bazelbuild/buildtools/releases/download/${version}/buildozer"

exec "$(dirname "$0")/download_executable.sh" "${checksum}" "${dest}" \
"${mirror_url}" "${github_url}"
2 changes: 2 additions & 0 deletions tensorboard/tools/mirror_urls_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,13 @@ check_urls_resolve() {
url_pcre='(?<=")https?://mirror\.tensorflow\.org/[^"]*'
exclude_bazel=':!ci/download_bazel.sh' # uses a '${version}' format string
exclude_buildifier=':!ci/download_buildifier.sh' # likewise
exclude_buildozer=':!ci/download_buildozer.sh' # likewise
# We use `git-grep` to efficiently get an initial result set, then
# filter it down with GNU `grep` separately, because `git-grep` only
# learned `-o` in Git v2.19; Travis uses v2.15.1.
unresolved_urls_file="${tmpdir}/unresolved_urls"
git grep -Ph "${url_pcre}" "${exclude_bazel}" "${exclude_buildifier}" \
"${exclude_buildozer}" \
| grep -o 'https\?://mirror\.tensorflow\.org/[^"]*' \
| sort -u \
>"${unresolved_urls_file}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ load("//tensorboard/defs:defs.bzl", "tf_sass_binary", "tf_ts_library")

package(default_visibility = ["//tensorboard:internal"])

licenses(["notice"]) # Apache 2.0
licenses(["notice"])

tf_sass_binary(
name = "annotations_list_component_styles",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ load("//tensorboard/defs:defs.bzl", "tf_sass_binary", "tf_ts_library")

package(default_visibility = ["//tensorboard:internal"])

licenses(["notice"]) # Apache 2.0
licenses(["notice"])

tf_sass_binary(
name = "annotations_list_toolbar_component_styles",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ load("//tensorboard/defs:defs.bzl", "tf_sass_binary", "tf_ts_library")

package(default_visibility = ["//tensorboard:internal"])

licenses(["notice"]) # Apache 2.0
licenses(["notice"])

tf_sass_binary(
name = "annotations_search_component_styles",
Expand Down
4 changes: 3 additions & 1 deletion third_party/bleach.BUILD
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
# Description:
# Build file for Bleach.
# License:
# Apache 2.0
package(default_visibility = ["//visibility:public"])

licenses(["notice"]) # Apache 2.0
licenses(["notice"])

exports_files(["LICENSE"])

Expand Down

0 comments on commit ea2da3b

Please sign in to comment.