Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds toolchain for freebsd. #794

Merged
merged 13 commits into from
Nov 29, 2021

Conversation

yesudeep
Copy link
Contributor

@yesudeep yesudeep commented Sep 29, 2021

I've got this passing all tests (with caveats) and building libraries (e.g. libdill). However,
I'm running into a rather tricky issue. The shebang line for a couple tests starts as follows:

#!/bin/bash 

while it should ideally be #!/usr/bin/env bash (preferable)
or #!/usr/local/bin/bash (where the sudo pkg install bash
installs it on FreeBSD).

OS and compiler:

❯ uname -a
FreeBSD hostname13.0-STABLE FreeBSD 13.0-STABLE GENERIC  amd64

❯ clang --version
clang version 14.0.0
Target: x86_64-portbld-freebsd13.0
Thread model: posix
InstalledDir: /usr/local/llvm-devel/bin

Tests

The tests pass if I create a symbolic link for bash here /bin/bash.
How do I get the test shell scripts to use the correct shebang line
without having to resort to creating a symbolic link as a workaround?

With workaround symbolic link:

❯ bazel test ...
DEBUG: /usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/b062c755353147a0b3f2b067655758e1/external/bazel_toolchains/rules/rbe_repo/version_check.bzl:68:14:
Current running Bazel is ahead of bazel-toolchains repo. Please update your pin to bazel-toolchains repo in your WORKSPACE file.
DEBUG: /usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/b062c755353147a0b3f2b067655758e1/external/bazel_toolchains/rules/rbe_repo/checked_in.bzl:125:14: buildkite_config not using checked in configs; Bazel version 4.2.1 was picked/selected but no checked in config was found in map {"0.20.0": ["8.0.0"], "0.21.0": ["8.0.0"], "0.22.0": ["8.0.0", "9.0.0"], "0.23.0": ["8.0.0", "9.0.0"], "0.23.1": ["8.0.0", "9.0.0"], "0.23.2": ["9.0.0"], "0.24.0": ["9.0.0"], "0.24.1": ["9.0.0"], "0.25.0": ["9.0.0"], "0.25.1": ["9.0.0"], "0.25.2": ["9.0.0"], "0.26.0": ["9.0.0"], "0.26.1": ["9.0.0"], "0.27.0": ["9.0.0"], "0.27.1": ["9.0.0"], "0.28.0": ["9.0.0"], "0.28.1": ["9.0.0"], "0.29.0": ["9.0.0"], "0.29.1": ["9.0.0", "10.0.0"], "1.0.0": ["9.0.0", "10.0.0"], "1.0.1": ["10.0.0"], "1.1.0": ["10.0.0"], "1.2.0": ["10.0.0"], "1.2.1": ["10.0.0"], "2.0.0": ["10.0.0"], "2.1.0": ["10.0.0"], "2.1.1": ["10.0.0", "11.0.0"], "2.2.0": ["11.0.0"], "3.0.0": ["11.0.0"], "3.1.0": ["11.0.0"], "3.2.0": ["11.0.0"], "3.3.0": ["11.0.0"], "3.3.1": ["11.0.0"], "3.4.1": ["11.0.0"], "3.5.0": ["11.0.0"], "3.5.1": ["11.0.0"], "3.6.0": ["11.0.0"], "3.7.0": ["11.0.0"], "3.7.1": ["11.0.0"], "3.7.2": ["11.0.0"], "4.0.0": ["11.0.0"]}
INFO: Analyzed 48 targets (58 packages loaded, 21141 targets configured).
INFO: Found 26 targets and 22 test targets...
INFO: Elapsed time: 0.943s, Critical Path: 0.08s
INFO: 91 processes: 46 internal, 45 processwrapper-sandbox.
INFO: Build completed successfully, 91 total actions
//test:cmake_script_test_suite_test_0                                    PASSED in 0.0s
//test:cmake_script_test_suite_test_1                                    PASSED in 0.0s
//test:cmake_script_test_suite_test_10                                   PASSED in 0.0s
//test:cmake_script_test_suite_test_11                                   PASSED in 0.0s
//test:cmake_script_test_suite_test_12                                   PASSED in 0.0s
//test:cmake_script_test_suite_test_2                                    PASSED in 0.0s
//test:cmake_script_test_suite_test_3                                    PASSED in 0.0s
//test:cmake_script_test_suite_test_4                                    PASSED in 0.0s
//test:cmake_script_test_suite_test_5                                    PASSED in 0.0s
//test:cmake_script_test_suite_test_6                                    PASSED in 0.0s
//test:cmake_script_test_suite_test_7                                    PASSED in 0.0s
//test:cmake_script_test_suite_test_8                                    PASSED in 0.0s
//test:cmake_script_test_suite_test_9                                    PASSED in 0.0s
//test:shell_method_symlink_contents_to_dir_test                         PASSED in 0.0s
//test:shell_script_conversion_suite_test_0                              PASSED in 0.0s
//test:shell_script_conversion_suite_test_1                              PASSED in 0.0s
//test:shell_script_conversion_suite_test_2                              PASSED in 0.0s
//test:shell_script_conversion_suite_test_3                              PASSED in 0.0s
//test:shell_script_conversion_suite_test_4                              PASSED in 0.0s
//test:shell_script_conversion_suite_test_5                              PASSED in 0.0s
//test:shell_script_inner_fun_test                                       PASSED in 0.0s
//test:utils_test_suite_test_0                                           PASSED in 0.0s

Executed 22 out of 22 tests: 22 tests pass.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these INFO: Build completed successfully, 91 total actions

The tests that fail without the workaround are:

...
//test:shell_method_symlink_contents_to_dir_test                         FAILED in 0.0s
  /usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/b062c755353147a0b3f2b067655758e1/execroot/rules_foreign_cc/bazel-out/freebsd-fastbuild/testlogs/test/shell_method_symlink_contents_to_dir_test/test.log
//test:shell_script_inner_fun_test                                       FAILED in 0.0s
  /usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/b062c755353147a0b3f2b067655758e1/execroot/rules_foreign_cc/bazel-out/freebsd-fastbuild/testlogs/test/shell_script_inner_fun_test/test.log

Executed 22 out of 22 tests: 20 tests pass and 2 fail locally.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these INFO: Build completed, 2 tests FAILED, 91 total actions

The Error:

❯ cat /usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/b062c755353147a0b3f2b067655758e1/execroot/rules_foreign_cc/bazel-out/freebsd-fas
tbuild/testlogs/test/shell_method_symlink_contents_to_dir_test/test.log

exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //test:shell_method_symlink_contents_to_dir_test
-----------------------------------------------------------------------------
external/bazel_tools/tools/test/test-setup.sh: /usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/b062c755353147a0b3f2b067655758e1/sandbox/processwrapper-sandbox/416/execroot/rules_foreign_cc/bazel-out/freebsd-fastbuild/bin/test/shell_method_symlink_contents_to_dir_test-test.sh.runfiles/rules_foreign_cc/test/shell_method_symlink_contents_to_dir_test-test.sh: /bin/bash: bad interpreter: No such file or directory

@UebelAndre
Copy link
Collaborator

Sorry for the massive delay here! I'd love to support Freebsd but unless we have something that runs in CI, there's a big risk of regression and I don't think we can confidently say we actually "support" it without tests. I think it'd be good to try and ping bazelbuild/continuous-integration#258 to communicate to Google that there's interest in Freebsd test runners.

@yesudeep
Copy link
Contributor Author

yesudeep commented Oct 22, 2021

@UebelAndre indeed. I've pinged on that thread, but it would still be neat to have each piece that Bazel depends on be ready regardless anyway. FreeBSD packages Bazel and currently because of all the breakages the experience is somewhat lacking. I think we can build the car and the road will come. wdyt?

I've also sent a PR for diff_test to use #!/usr/bin/env bash for the shebang line so that these rules start working on various *BSDs: bazelbuild/bazel-skylib#329

While we wait for a CI, people like me who use FreeBSD can help report any issues. Over time when you have CI support,
you can just make an announcement saying "We have FreeBSD support!" It would be easier to let people use these rules as beta in the meantime.

@jsharpe
Copy link
Member

jsharpe commented Nov 17, 2021

My opinion would be to merge this despite not having any testing capacity but with the caveat that changes will only be propagated to this platform on a best effort. We'd rely on you as freeBSD users to keep this updated? I know in the past cockroachdb have been using a fork that supports freeBSD so having support in the main repo would mean that they could move to the main releases of rules_foreign_cc?

@jsharpe
Copy link
Member

jsharpe commented Nov 17, 2021

@UebelAndre do you have any opinions on this?

@UebelAndre
Copy link
Collaborator

I'm worried about our inability to test those code paths for future changes. If someone makes a change, then unless we have failing tests for BSD then I would say there's no obligation for them to care about that environment. Which would very likely frustrate BSD users who had their builds break after that commit. If there's a good story for managing expectations or these code paths then I'm for it

@yesudeep
Copy link
Contributor Author

yesudeep commented Nov 18, 2021

I'm worried about our inability to test those code paths for future changes. If someone makes a change, then unless we have failing tests for BSD then I would say there's no obligation for them to care about that environment. Which would very likely frustrate BSD users who had their builds break after that commit. If there's a good story for managing expectations or these code paths then I'm for it

I'd say we shouldn't worry about frustrating FreeBSD users (having a build that works is better than one that doesn't, no?). I can send patches for changes in the meantime because I'm using Bazel on FreeBSD. best effort is alright with me. :)

@UebelAndre
Copy link
Collaborator

@jsharpe I wish we had some CI for this but I don't want to block efforts for FreeBSD users. @yesudeep Would you be willing to add a disclaimer somewhere and reference the BazelCI issue? I think that'd make me happy 😃

@yesudeep
Copy link
Contributor Author

@UebelAndre Sure thing!

@UebelAndre
Copy link
Collaborator

@yesudeep sorry about the merge conflict! Would you be able to resolve?

@jsharpe I'm good with this change if you are, would you be able to take a quick look?

Copy link
Member

@jsharpe jsharpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yesudeep
Copy link
Contributor Author

yesudeep commented Nov 27, 2021

Hi @UebelAndre please wait before merging this PR. Cross-checking a few things. My PR to bazel-skylib got merged but bazel-skylib in this repository is tracking release version 1.1.1. There haven't been releases since September 27 but the tests require a new release to pass for FreeBSD.

Would you be okay with including a non-release version of bazel-skylib here (not sure what the policy for Bazel projects says)?

@yesudeep
Copy link
Contributor Author

yesudeep commented Nov 27, 2021

Test results attached:

Sat Nov 27 09:53 AM yesudeep at hostname running bash5.1 on FreeBSD 13.0-RELEASE-p4 amd64
in ~/code/rules_foreign_cc  add-toolchain-for-freebsd
❯ bazel test ...
DEBUG: /usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/d7205c2104ca083cc429f28f6bdb95ca/external/bazel_toolchains/rules/rbe_repo/version_check.bzl:68:14:
Current running Bazel is ahead of bazel-toolchains repo. Please update your pin to bazel-toolchains repo in your WORKSPACE file.
DEBUG: /usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/d7205c2104ca083cc429f28f6bdb95ca/external/bazel_toolchains/rules/rbe_repo/checked_in.bzl:125:14: buildkite_config not using checked in configs; Bazel version 4.2.1 was picked/selected but no checked in config was found in map {"0.20.0": ["8.0.0"], "0.21.0": ["8.0.0"], "0.22.0": ["8.0.0", "9.0.0"], "0.23.0": ["8.0.0", "9.0.0"], "0.23.1": ["8.0.0", "9.0.0"], "0.23.2": ["9.0.0"], "0.24.0": ["9.0.0"], "0.24.1": ["9.0.0"], "0.25.0": ["9.0.0"], "0.25.1": ["9.0.0"], "0.25.2": ["9.0.0"], "0.26.0": ["9.0.0"], "0.26.1": ["9.0.0"], "0.27.0": ["9.0.0"], "0.27.1": ["9.0.0"], "0.28.0": ["9.0.0"], "0.28.1": ["9.0.0"], "0.29.0": ["9.0.0"], "0.29.1": ["9.0.0", "10.0.0"], "1.0.0": ["9.0.0", "10.0.0"], "1.0.1": ["10.0.0"], "1.1.0": ["10.0.0"], "1.2.0": ["10.0.0"], "1.2.1": ["10.0.0"], "2.0.0": ["10.0.0"], "2.1.0": ["10.0.0"], "2.1.1": ["10.0.0", "11.0.0"], "2.2.0": ["11.0.0"], "3.0.0": ["11.0.0"], "3.1.0": ["11.0.0"], "3.2.0": ["11.0.0"], "3.3.0": ["11.0.0"], "3.3.1": ["11.0.0"], "3.4.1": ["11.0.0"], "3.5.0": ["11.0.0"], "3.5.1": ["11.0.0"], "3.6.0": ["11.0.0"], "3.7.0": ["11.0.0"], "3.7.1": ["11.0.0"], "3.7.2": ["11.0.0"], "4.0.0": ["11.0.0"]}
INFO: Analyzed 48 targets (0 packages loaded, 0 targets configured).
INFO: Found 26 targets and 22 test targets...
INFO: Elapsed time: 0.141s, Critical Path: 0.04s
INFO: 3 processes: 4 processwrapper-sandbox.
INFO: Build completed successfully, 3 total actions
//test:cmake_script_test_suite_test_0                           (cached) PASSED in 0.1s
//test:cmake_script_test_suite_test_1                           (cached) PASSED in 0.0s
//test:cmake_script_test_suite_test_10                          (cached) PASSED in 0.0s
//test:cmake_script_test_suite_test_11                          (cached) PASSED in 0.0s
//test:cmake_script_test_suite_test_12                          (cached) PASSED in 0.0s
//test:cmake_script_test_suite_test_2                           (cached) PASSED in 0.0s
//test:cmake_script_test_suite_test_3                           (cached) PASSED in 0.0s
//test:cmake_script_test_suite_test_4                           (cached) PASSED in 0.0s
//test:cmake_script_test_suite_test_5                           (cached) PASSED in 0.0s
//test:cmake_script_test_suite_test_6                           (cached) PASSED in 0.0s
//test:cmake_script_test_suite_test_7                           (cached) PASSED in 0.0s
//test:cmake_script_test_suite_test_8                           (cached) PASSED in 0.0s
//test:cmake_script_test_suite_test_9                           (cached) PASSED in 0.0s
//test:shell_script_conversion_suite_test_0                     (cached) PASSED in 0.0s
//test:shell_script_conversion_suite_test_1                     (cached) PASSED in 0.0s
//test:shell_script_conversion_suite_test_2                     (cached) PASSED in 0.0s
//test:shell_script_conversion_suite_test_3                     (cached) PASSED in 0.0s
//test:shell_script_conversion_suite_test_4                     (cached) PASSED in 0.0s
//test:shell_script_conversion_suite_test_5                     (cached) PASSED in 0.0s
//test:utils_test_suite_test_0                                  (cached) PASSED in 0.0s
//test:shell_method_symlink_contents_to_dir_test                         PASSED in 0.0s
//test:shell_script_inner_fun_test                                       PASSED in 0.0s

Executed 2 out of 22 tests: 22 tests pass.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line op
INFO: Build completed successfully, 3 total actions

Additional patch required (I'll change 'main' to the commit ref if this is okay):

Sat Nov 27 09:58 AM yesudeep at hostname running bash5.1 on FreeBSD 13.0-RELEASE-p4 amd64
in ~/code/rules_foreign_cc  add-toolchain-for-freebsd
❯ git diff
diff --git a/foreign_cc/repositories.bzl b/foreign_cc/repositories.bzl
index 931d5ea..c3f3e2b 100644
--- a/foreign_cc/repositories.bzl
+++ b/foreign_cc/repositories.bzl
@@ -64,8 +64,8 @@ def rules_foreign_cc_dependencies(
         http_archive,
         name = "bazel_skylib",
         urls = [
-            "https://github.com/bazelbuild/bazel-skylib/releases/download/1.1.1/bazel-skylib-1.1.1.tar.gz",
-            "https://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/releases/download/1.1.1/bazel-skylib-1.1.1.tar.gz",
+            "https://github.com/bazelbuild/bazel-skylib/archive/refs/heads/main.zip",
         ],
-        sha256 = "c6966ec828da198c5d9adbaa94c05e3a1c7f21bd012a0b29ba8ddbccb2c93b0d",
+        strip_prefix = "bazel-skylib-main",
+        sha256 = "40352048428e30ab4707955994d42c71dab9e6fdd92ec3f94670784565a1350b",
     )

@UebelAndre
Copy link
Collaborator

I think that's fine to use a commit vs a release for Bazel-skylib. Could you open an issue there requesting a release and link that with the updated repository rule? 🙏

@yesudeep
Copy link
Contributor Author

Hey @UebelAndre done.

bazelbuild/bazel-skylib#336

Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me if it looks good to @jsharpe!

foreign_cc/repositories.bzl Show resolved Hide resolved
@jsharpe jsharpe merged commit a2f1e5d into bazel-contrib:main Nov 29, 2021
@UebelAndre UebelAndre mentioned this pull request Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants