From 87b25ed65937adc131e7d232e2226b2f822a1a85 Mon Sep 17 00:00:00 2001 From: Siddhartha Bagaria Date: Tue, 12 Sep 2023 17:48:11 -0700 Subject: [PATCH] Fix all lint warnings (#214) As per `trunk check --all`. --- README.md | 30 +++++++++++------------ tests/scripts/bazel.sh | 9 +++++-- tests/scripts/centos_test.sh | 3 +++ tests/scripts/run_external_tests.sh | 7 +++--- tests/scripts/run_tests.sh | 8 +++--- tests/scripts/suse_leap_test.sh | 2 +- tests/scripts/suse_tumbleweed_test.sh | 2 +- toolchain/cc_wrapper.sh.tpl | 8 +++--- toolchain/internal/common.bzl | 14 +++++------ toolchain/internal/configure.bzl | 6 ----- toolchain/internal/llvm_distributions.bzl | 7 ++++-- toolchain/internal/release_name.bzl | 6 +++-- toolchain/internal/repo.bzl | 4 +-- toolchain/internal/sysroot.bzl | 2 +- toolchain/osx_cc_wrapper.sh.tpl | 6 +++-- utils/llvm_checksums.sh | 12 ++++----- 16 files changed, 69 insertions(+), 57 deletions(-) diff --git a/README.md b/README.md index db3fb7d..5e57f13 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,7 @@ llvm_register_toolchains() And add the following section to your .bazelrc file: -``` +```sh # Not needed after https://github.com/bazelbuild/bazel/issues/7260 is closed build --incompatible_enable_cc_toolchain_resolution @@ -91,7 +91,7 @@ attributes to `llvm_toolchain`. ## Advanced Usage -#### Per host architecture LLVM version +### Per host architecture LLVM version LLVM does not come with distributions for all host architectures in each version. In particular patch versions often come with few prebuilt packages. @@ -104,7 +104,7 @@ specified explicitly. This is like providing `llvm_version = "15.0.6"`, just like in the example on the top. However, here we provide two more entries that map their respective target to a distinct version: -``` +```starlark llvm_toolchain( name = "llvm_toolchain", llvm_versions = { @@ -115,7 +115,7 @@ llvm_toolchain( ) ``` -#### Customizations +### Customizations We currently offer limited customizability through attributes of the [llvm_toolchain\_\* rules](toolchain/rules.bzl). You can send us a PR to add @@ -127,7 +127,7 @@ new compiler features, etc., my advice would be to look at the toolchain configurations generated by this repo, and copy-paste/edit to make your own in any package in your own workspace. -``` +```sh bazel query --output=build @llvm_toolchain//:all | grep -v -e '^#' -e '^ generator' ``` @@ -140,7 +140,7 @@ directory, and also create a wrapper script for clang such that the actual clang invocation is not through the symlinked path. See the files in the `@llvm_toolchain//:` package as a reference. -``` +```sh # See generated files for reference. ls -lR "$(bazel info output_base)/external/llvm_toolchain" @@ -158,7 +158,7 @@ See [bazel tutorial](https://docs.bazel.build/versions/main/tutorial/cc-toolchain-config.html) for how CC toolchains work in general. -#### Selecting Toolchains +### Selecting Toolchains If toolchains are registered (see Quickstart section above), you do not need to do anything special for bazel to find the toolchain. You may want to check once @@ -172,7 +172,7 @@ For specifying unregistered toolchains on the command line, please use the We no longer support the `--crosstool_top=@llvm_toolchain//:toolchain` flag, and instead rely on the `--incompatible_enable_cc_toolchain_resolution` flag. -#### Bring Your Own LLVM +### Bring Your Own LLVM The following mechanisms are available for using an LLVM toolchain: @@ -204,7 +204,7 @@ The following mechanisms are available for using an LLVM toolchain: a toolchain root specified through the `toolchain_roots` attribute with an empty key. -#### Sysroots +### Sysroots A sysroot can be specified through the `sysroot` attribute. This can be either a path on the user's system, or a bazel `filegroup` like label. One way to @@ -213,7 +213,7 @@ entire filesystem for the image you want. Another way is to use the build scripts provided by the [Chromium project](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/linux/sysroot.md). -#### Cross-compilation +### Cross-compilation The toolchain supports cross-compilation if you bring your own sysroot. When cross-compiling, we link against the libstdc++ from the sysroot @@ -231,14 +231,14 @@ Then, when cross-compiling, explicitly specify the toolchain with the sysroot and the target platform. For example, see the [WORKSPACE](tests/WORKSPACE) file and the [test script](tests/scripts/run_xcompile_tests.sh) for cross-compilation. -``` +```sh bazel build \ --platforms=@com_grail_bazel_toolchain//platforms:linux-x86_64 \ --extra_toolchains=@llvm_toolchain_with_sysroot//:cc-toolchain-x86_64-linux \ //... ``` -#### Supporting New Target Platforms +### Supporting New Target Platforms The following is a rough (untested) list of steps: @@ -256,7 +256,7 @@ The following is a rough (untested) list of steps: `toolchain_roots` or `urls` attribute. 6. Test your build. -#### Sandbox +### Sandbox Sandboxing the toolchain introduces a significant overhead (100ms per action, as of mid 2018). To overcome this, one can use @@ -267,12 +267,12 @@ substitute templated paths to the toolchain as absolute paths. When running bazel actions, these paths will be available from inside the sandbox as part of the / read-only mount. Note that this will make your builds non-hermetic. -#### Compatibility +### Compatibility The toolchain is tested to work with `rules_go`, `rules_rust`, and `rules_foreign_cc`. -#### Accessing tools +### Accessing tools The LLVM distribution also provides several tools like `clang-format`. You can depend on these tools directly in the bin directory of the distribution. When diff --git a/tests/scripts/bazel.sh b/tests/scripts/bazel.sh index 31af7fd..63740e8 100644 --- a/tests/scripts/bazel.sh +++ b/tests/scripts/bazel.sh @@ -12,7 +12,12 @@ # See the License for the specific language governing permissions and # limitations under the License. -os="$(uname -s | tr "[:upper:]" "[:lower:]")" +# shellcheck shell=bash + +short_uname="$(uname -s)" +readonly short_uname + +os="$(echo "${short_uname}" | tr "[:upper:]" "[:lower:]")" readonly os arch="$(uname -m)" @@ -47,7 +52,7 @@ common_test_args=( # TODO: Remove this once we no longer support bazel 6.x. # This feature isn't intentionally supported on macOS. -if [[ $(uname -s) == 'Darwin' ]]; then +if [[ ${short_uname} == 'Darwin' ]]; then common_test_args+=(--features=-supports_dynamic_linker) fi diff --git a/tests/scripts/centos_test.sh b/tests/scripts/centos_test.sh index 2217065..5166896 100755 --- a/tests/scripts/centos_test.sh +++ b/tests/scripts/centos_test.sh @@ -13,6 +13,9 @@ # See the License for the specific language governing permissions and # limitations under the License. +# Disable the unreachable code warning because the test is disabled. +# shellcheck disable=SC2317 + echo "This test is disabled because our supported versions of LLVM do not work with CentOS." exit 1 diff --git a/tests/scripts/run_external_tests.sh b/tests/scripts/run_external_tests.sh index 7717f11..7e19527 100755 --- a/tests/scripts/run_external_tests.sh +++ b/tests/scripts/run_external_tests.sh @@ -23,13 +23,12 @@ cd "${scripts_dir}" # Generate some files needed for the tests. "${bazel}" query "${common_args[@]}" @io_bazel_rules_go//tests/core/cgo:dylib_test >/dev/null -if [[ $USE_BZLMOD == "true" ]]; then +if [[ ${USE_BZLMOD} == "true" ]]; then "$("${bazel}" info output_base)/external/rules_go~0.41.0/tests/core/cgo/generate_imported_dylib.sh" else "$("${bazel}" info output_base)/external/io_bazel_rules_go/tests/core/cgo/generate_imported_dylib.sh" fi -set -x test_args=( "${common_test_args[@]}" # Fix LLVM version to be 14.0.0 because that's the last known version with @@ -50,6 +49,8 @@ test_args=( # @rules_rust//test/unit/{native_deps,linkstamps,interleaved_cc_info}:all # but under bzlmod the linkstamp tests fail due to the fact we are currently # overriding rules_rust locally as its not yet released in the BCR +# shellcheck disable=SC2207 +absl_targets=($("${bazel}" query "${common_args[@]}" 'attr(timeout, short, tests(@com_google_absl//absl/...))')) "${bazel}" --bazelrc=/dev/null test "${test_args[@]}" -- \ //foreign:pcre \ @openssl//:libssl \ @@ -57,5 +58,5 @@ test_args=( @io_bazel_rules_go//tests/core/cgo:all \ -@io_bazel_rules_go//tests/core/cgo:cc_libs_test \ -@io_bazel_rules_go//tests/core/cgo:external_includes_test \ - $("${bazel}" query 'attr(timeout, short, tests(@com_google_absl//absl/...))') \ + "${absl_targets[@]}" \ -@com_google_absl//absl/time/internal/cctz:time_zone_format_test diff --git a/tests/scripts/run_tests.sh b/tests/scripts/run_tests.sh index c726fcf..94782c3 100755 --- a/tests/scripts/run_tests.sh +++ b/tests/scripts/run_tests.sh @@ -18,15 +18,15 @@ set -euo pipefail toolchain_name="" while getopts "t:h" opt; do - case "$opt" in - "t") toolchain_name="$OPTARG" ;; + case "${opt}" in + "t") toolchain_name="${OPTARG}" ;; "h") echo "Usage:" echo "-t - Toolchain name to use for testing; default is llvm_toolchain" exit 2 ;; - "?") - echo "invalid option: -$OPTARG" + *) + echo "invalid option: -${OPTARG}" exit 1 ;; esac diff --git a/tests/scripts/suse_leap_test.sh b/tests/scripts/suse_leap_test.sh index a9c065d..b0faefa 100755 --- a/tests/scripts/suse_leap_test.sh +++ b/tests/scripts/suse_leap_test.sh @@ -25,7 +25,7 @@ toolchain="@llvm_toolchain_13_0_0//:cc-toolchain-x86_64-linux" git_root=$(git rev-parse --show-toplevel) readonly git_root -echo "git root: $git_root" +echo "git root: ${git_root}" for image in "${images[@]}"; do docker pull "${image}" diff --git a/tests/scripts/suse_tumbleweed_test.sh b/tests/scripts/suse_tumbleweed_test.sh index 7559350..7198eff 100755 --- a/tests/scripts/suse_tumbleweed_test.sh +++ b/tests/scripts/suse_tumbleweed_test.sh @@ -25,7 +25,7 @@ toolchain="@llvm_toolchain_13_0_0//:cc-toolchain-x86_64-linux" git_root=$(git rev-parse --show-toplevel) readonly git_root -echo "git root: $git_root" +echo "git root: ${git_root}" for image in "${images[@]}"; do docker pull "${image}" diff --git a/toolchain/cc_wrapper.sh.tpl b/toolchain/cc_wrapper.sh.tpl index 5765136..e70c880 100644 --- a/toolchain/cc_wrapper.sh.tpl +++ b/toolchain/cc_wrapper.sh.tpl @@ -13,7 +13,7 @@ # 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. -# + # OS X relpath is not really working. This is a wrapper script around gcc # to simulate relpath behavior. # @@ -24,7 +24,9 @@ # # See https://blogs.oracle.com/dipol/entry/dynamic_libraries_rpath_and_mac # on how to set those paths for Mach-O binaries. -# + +# shellcheck disable=SC1083 + set -eu # See note in toolchain/internal/configure.bzl where we define @@ -43,6 +45,6 @@ elif [[ ${BASH_SOURCE[0]} == "/"* ]]; then clang="${execroot_path}/%{toolchain_path_prefix}bin/clang" exec "${clang}" "${@}" else - echo >&2 "ERROR: could not find clang; PWD=\"$(pwd)\"; PATH=\"${PATH}\"." + echo >&2 "ERROR: could not find clang; PWD=\"${PWD}\"; PATH=\"${PATH}\"." exit 5 fi diff --git a/toolchain/internal/common.bzl b/toolchain/internal/common.bzl index 1d2f0dd..99f51c4 100644 --- a/toolchain/internal/common.bzl +++ b/toolchain/internal/common.bzl @@ -41,8 +41,8 @@ def _linux_dist(rctx): if res.return_code: fail("Failed to detect machine architecture: \n%s\n%s" % (res.stdout, res.stderr)) info = {} - for l in res.stdout.splitlines(): - parts = l.split("=", 1) + for line in res.stdout.splitlines(): + parts = line.split("=", 1) info[parts[0]] = parts[1] distname = info["ID"].strip('\"') @@ -122,12 +122,12 @@ def host_os_arch_dict_value(rctx, attr_name, debug = False): key2 = os_arch_pair(os(rctx), arch(rctx)) if debug: - print("`%s` attribute missing for key '%s' in repository '%s'; checking with key '%s'" % (attr_name, key1, rctx.name, key2)) + print("`%s` attribute missing for key '%s' in repository '%s'; checking with key '%s'" % (attr_name, key1, rctx.name, key2)) # buildifier: disable=print if key2 in d: return (key2, d.get(key2)) if debug: - print("`%s` attribute missing for key '%s' in repository '%s'; checking with key ''" % (attr_name, key2, rctx.name)) + print("`%s` attribute missing for key '%s' in repository '%s'; checking with key ''" % (attr_name, key2, rctx.name)) # buildifier: disable=print return ("", d.get("")) # Fallback to empty key. def canonical_dir_path(path): @@ -147,10 +147,10 @@ def pkg_path_from_label(label): else: return label.package -def list_to_string(l): - if l == None: +def list_to_string(ls): + if ls == None: return "None" - return "[{}]".format(", ".join(["\"{}\"".format(d) for d in l])) + return "[{}]".format(", ".join(["\"{}\"".format(d) for d in ls])) def attr_dict(attr): # Returns a mutable dict of attr values from the struct. This is useful to diff --git a/toolchain/internal/configure.bzl b/toolchain/internal/configure.bzl index 936d130..63727bc 100644 --- a/toolchain/internal/configure.bzl +++ b/toolchain/internal/configure.bzl @@ -42,12 +42,6 @@ load( # workspace builds, there is never a @@ in labels. BZLMOD_ENABLED = "@@" in str(Label("//:unused")) -def _include_dirs_str(rctx, key): - dirs = rctx.attr.cxx_builtin_include_directories.get(key) - if not dirs: - return "" - return ("\n" + 12 * " ").join(["\"%s\"," % d for d in dirs]) - def llvm_config_impl(rctx): _check_os_arch_keys(rctx.attr.sysroot) _check_os_arch_keys(rctx.attr.cxx_builtin_include_directories) diff --git a/toolchain/internal/llvm_distributions.bzl b/toolchain/internal/llvm_distributions.bzl index 3bbd2bd..f1ea542 100644 --- a/toolchain/internal/llvm_distributions.bzl +++ b/toolchain/internal/llvm_distributions.bzl @@ -336,6 +336,9 @@ def _get_auth(ctx, urls): def download_llvm(rctx): urls = [] + sha256 = None + strip_prefix = None + key = None update_sha256 = False if rctx.attr.urls: urls, sha256, strip_prefix, key = _urls(rctx) @@ -359,7 +362,7 @@ def download_llvm(rctx): def _urls(rctx): (key, urls) = _host_os_arch_dict_value(rctx, "urls", debug = False) if not urls: - print("LLVM archive URLs missing and no default fallback provided; will try 'distribution' attribute") + print("LLVM archive URLs missing and no default fallback provided; will try 'distribution' attribute") # buildifier: disable=print sha256 = rctx.attr.sha256.get(key, default = "") strip_prefix = rctx.attr.strip_prefix.get(key, default = "") @@ -371,7 +374,7 @@ def _get_llvm_version(rctx): return rctx.attr.llvm_version if not rctx.attr.llvm_versions: fail("Neither 'llvm_version' nor 'llvm_versions' given.") - (key, llvm_version) = _host_os_arch_dict_value(rctx, "llvm_versions") + (_, llvm_version) = _host_os_arch_dict_value(rctx, "llvm_versions") if not llvm_version: fail("LLVM version string missing for ({os}, {arch})", os = _os(rctx), arch = _arch(rctx)) return llvm_version diff --git a/toolchain/internal/release_name.bzl b/toolchain/internal/release_name.bzl index eae9265..5f173cf 100755 --- a/toolchain/internal/release_name.bzl +++ b/toolchain/internal/release_name.bzl @@ -16,7 +16,7 @@ def _darwin_apple_suffix(llvm_version, arch): major_llvm_version = _major_llvm_version(llvm_version) patch_llvm_version = _patch_llvm_version(llvm_version) if major_llvm_version == 9: - "darwin-apple" + return "darwin-apple" elif major_llvm_version >= 15: if arch == "arm64": if major_llvm_version == 15 and patch_llvm_version <= 6: @@ -93,6 +93,7 @@ def _linux(llvm_version, distname, version, arch): # NOTE: Many of these systems are untested because I do not have access to them. # If you find this mapping wrong, please send a Pull Request on Github. + os_name = None if arch in ["aarch64", "armv7a", "mips", "mipsel"]: os_name = "linux-gnu" elif distname == "freebsd": @@ -134,7 +135,8 @@ def _linux(llvm_version, distname, version, arch): os_name = _ubuntu_osname(arch, "18.04", major_llvm_version, llvm_version) elif float(version) >= 9: os_name = _ubuntu_osname(arch, "20.04", major_llvm_version, llvm_version) - else: + + if not os_name: fail("Unsupported linux distribution and version: %s, %s" % (distname, version)) return "clang+llvm-{llvm_version}-{arch}-{os_name}.tar.xz".format( diff --git a/toolchain/internal/repo.bzl b/toolchain/internal/repo.bzl index ffb8a9a..67b3ed8 100644 --- a/toolchain/internal/repo.bzl +++ b/toolchain/internal/repo.bzl @@ -269,8 +269,8 @@ llvm_config_attrs.update({ def llvm_repo_impl(rctx): os = _os(rctx) if os == "windows": - rctx.file("BUILD", executable = False) - return + rctx.file("BUILD.bazel", executable = False) + return None rctx.file( "BUILD.bazel", diff --git a/toolchain/internal/sysroot.bzl b/toolchain/internal/sysroot.bzl index 808146e..34e0b39 100644 --- a/toolchain/internal/sysroot.bzl +++ b/toolchain/internal/sysroot.bzl @@ -26,7 +26,7 @@ def _darwin_sdk_path(rctx): if exec_result.return_code: fail("Failed to detect OSX SDK path: \n%s\n%s" % (exec_result.stdout, exec_result.stderr)) if exec_result.stderr: - print(exec_result.stderr) + print(exec_result.stderr) # buildifier: disable=print return exec_result.stdout.strip() # Default sysroot path can be used when the user has not provided an explicit diff --git a/toolchain/osx_cc_wrapper.sh.tpl b/toolchain/osx_cc_wrapper.sh.tpl index ac744a9..4347e8b 100755 --- a/toolchain/osx_cc_wrapper.sh.tpl +++ b/toolchain/osx_cc_wrapper.sh.tpl @@ -13,7 +13,7 @@ # 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. -# + # OS X relpath is not really working. This is a wrapper script around gcc # to simulate relpath behavior. # @@ -24,7 +24,9 @@ # # See https://blogs.oracle.com/dipol/entry/dynamic_libraries_rpath_and_mac # on how to set those paths for Mach-O binaries. -# + +# shellcheck disable=all + set -eu INSTALL_NAME_TOOL="/usr/bin/install_name_tool" diff --git a/utils/llvm_checksums.sh b/utils/llvm_checksums.sh index 6e58995..ec47a23 100755 --- a/utils/llvm_checksums.sh +++ b/utils/llvm_checksums.sh @@ -18,8 +18,8 @@ set -euo pipefail use_github_host=0 while getopts "v:gh" opt; do - case "$opt" in - "v") llvm_version="$OPTARG" ;; + case "${opt}" in + "v") llvm_version="${OPTARG}" ;; "g") use_github_host=1 ;; "h") echo "Usage:" @@ -27,14 +27,14 @@ while getopts "v:gh" opt; do echo "-g - Use github to download releases" exit 2 ;; - "?") - echo "invalid option: -$OPTARG" + *) + echo "invalid option: -${OPTARG}" exit 1 ;; esac done -if ! [[ "${llvm_version-}" ]]; then +if [[ -z ${llvm_version-} ]]; then echo "Usage: ${BASH_SOURCE[0]} [-g] -v llvm_version" exit 1 fi @@ -44,7 +44,7 @@ tmp_dir="$(mktemp -d)" cleanup() { rc=$? rm -rf "${tmp_dir}" - exit $rc + exit "${rc}" } trap 'cleanup' INT HUP QUIT TERM EXIT