Skip to content

Commit

Permalink
Fix all lint warnings
Browse files Browse the repository at this point in the history
As per `trunk check --all`.
  • Loading branch information
Siddhartha Bagaria committed Sep 13, 2023
1 parent c196bc1 commit 1694f2e
Show file tree
Hide file tree
Showing 16 changed files with 69 additions and 57 deletions.
30 changes: 15 additions & 15 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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.
Expand All @@ -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 = {
Expand All @@ -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
Expand All @@ -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'
```

Expand All @@ -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"

Expand All @@ -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
Expand All @@ -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:

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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:

Expand All @@ -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
Expand All @@ -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
Expand Down
9 changes: 7 additions & 2 deletions tests/scripts/bazel.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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)"
Expand Down Expand Up @@ -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

Expand Down
3 changes: 3 additions & 0 deletions tests/scripts/centos_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
7 changes: 4 additions & 3 deletions tests/scripts/run_external_tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -50,12 +49,14 @@ 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 \
@rules_rust//test/unit/interleaved_cc_info:all \
@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
8 changes: 4 additions & 4 deletions tests/scripts/run_tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/scripts/suse_leap_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand Down
2 changes: 1 addition & 1 deletion tests/scripts/suse_tumbleweed_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand Down
8 changes: 5 additions & 3 deletions toolchain/cc_wrapper.sh.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -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.
#
Expand All @@ -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
Expand All @@ -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
14 changes: 7 additions & 7 deletions toolchain/internal/common.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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('\"')
Expand Down Expand Up @@ -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):
Expand All @@ -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
Expand Down
6 changes: 0 additions & 6 deletions toolchain/internal/configure.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 5 additions & 2 deletions toolchain/internal/llvm_distributions.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 = "")
Expand All @@ -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
Expand Down
6 changes: 4 additions & 2 deletions toolchain/internal/release_name.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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":
Expand Down Expand Up @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions toolchain/internal/repo.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion toolchain/internal/sysroot.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit 1694f2e

Please sign in to comment.