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

Add freebsd support #2991

Closed
wants to merge 23 commits into from
Closed

Conversation

yesudeep
Copy link

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

FreeBSD is currently an unsupported platform.

Issue Number: #2990

What is the new behavior?

Adds support to use rules_nodejs on FreeBSD.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@yesudeep
Copy link
Author

bazel test ... in the repository currently fails with this error and I'm not sure how to proceed. Any help is appreciated.

❯ bazel test ...
INFO: Invocation ID: 29836b98-8058-44a5-a737-b68876748ba7
DEBUG: /usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/09e42144392858a69d1f6561dd6bc63d/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/09e42144392858a69d1f6561dd6bc63d/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"]}
DEBUG: /usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/09e42144392858a69d1f6561dd6bc63d/external/bazel_toolchains/rules/rbe_repo/checked_in.bzl:125:14: rbe_default not using checked in configs; Bazel version 4.1.0 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: Repository npm_node_patches instantiated at:
  /usr/home/yesudeep/code/yesudeep/rules_nodejs/WORKSPACE:63:9: in <toplevel>
  /usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/09e42144392858a69d1f6561dd6bc63d/external/build_bazel_rules_nodejs/npm_deps.bzl:527:16: in npm_deps
  /usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/09e42144392858a69d1f6561dd6bc63d/external/build_bazel_rules_nodejs/index.bzl:78:17: in npm_install
Repository rule npm_install defined at:
  /usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/09e42144392858a69d1f6561dd6bc63d/external/build_bazel_rules_nodejs/internal/npm_install/npm_install.bzl:775:30: in <toplevel>
ERROR: An error occurred during the fetch of repository 'npm_node_patches':
   Traceback (most recent call last):
        File "/usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/09e42144392858a69d1f6561dd6bc63d/external/build_bazel_rules_nodejs/internal/npm_install/npm_install.bzl", line 674, column 31, in _npm_install_impl
                node = repository_ctx.path(get_node_label(repository_ctx))
Error in path: Unable to load package for @nodejs_freebsd_amd64//:bin/node: The repository '@nodejs_freebsd_amd64' could not be resolved
ERROR: Error fetching repository: Traceback (most recent call last):
        File "/usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/09e42144392858a69d1f6561dd6bc63d/external/build_bazel_rules_nodejs/internal/npm_install/npm_install.bzl", line 674, column 31, in _npm_install_impl
                node = repository_ctx.path(get_node_label(repository_ctx))
Error in path: Unable to load package for @nodejs_freebsd_amd64//:bin/node: The repository '@nodejs_freebsd_amd64' could not be resolved
INFO: Repository rules_codeowners instantiated at:
  /usr/home/yesudeep/code/yesudeep/rules_nodejs/WORKSPACE:41:30: in <toplevel>
  /usr/home/yesudeep/code/yesudeep/rules_nodejs/package.bzl:98:11: in rules_nodejs_dev_dependencies
  /usr/home/yesudeep/code/yesudeep/rules_nodejs/package.bzl:127:18: in _maybe
Repository rule http_archive defined at:
  /usr/home/yesudeep/.cache/bazel/_bazel_yesudeep/09e42144392858a69d1f6561dd6bc63d/external/bazel_tools/tools/build_defs/repo/http.bzl:336:31: in <toplevel>
ERROR: no such package '@npm_node_patches//typescript': Unable to load package for @nodejs_freebsd_amd64//:bin/node: The repository '@nodejs_freebsd_amd64' could not be resolved
INFO: Elapsed time: 0.598s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (1 packages loaded)

@alexeagle
Copy link
Collaborator

CI failure suggests we just missed a spot
ERROR: /home/circleci/rules_nodejs/internal/node/test/BUILD.bazel:344:23: //internal/node/test:nodejs_toolchain_freebsd_amd64_test: invalid value in 'platform' attribute: has to be one of 'linux_amd64', 'linux_arm64', 'linux_s390x', 'linux_ppc64le', 'darwin_amd64', 'darwin_arm64' or 'windows_amd64' instead of 'freebsd_amd64'

@alexeagle alexeagle force-pushed the add-freebsd-support branch from d1d04d1 to b99dddc Compare October 7, 2021 04:33
@google-cla
Copy link

google-cla bot commented Oct 7, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Oct 7, 2021
@alexeagle
Copy link
Collaborator

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Oct 7, 2021
Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Thanks!

@alexeagle alexeagle force-pushed the add-freebsd-support branch from e2b2d43 to 4263e02 Compare October 7, 2021 04:45
@yesudeep
Copy link
Author

yesudeep commented Oct 7, 2021

Thank you!

@yesudeep
Copy link
Author

yesudeep commented Oct 7, 2021

@googlebot I consent.

For some reason they don't include cpu constraint
@alexeagle alexeagle force-pushed the add-freebsd-support branch from 4263e02 to 9daaab9 Compare October 7, 2021 13:54
Aghassi and others added 4 commits October 12, 2021 13:50
Given the prior fix we made to load from 5.x, the range can be updated to support the latest versions
@yesudeep yesudeep requested a review from mrmeku as a code owner October 23, 2021 04:30
@yesudeep
Copy link
Author

I've sent a PRs to rules_foreign_cc and bazel-skylib to add FreeBSD support. Those should allow me to experiment with building nodejs from source as fallback on FreeBSD:

@alexeagle
Copy link
Collaborator

Wow you're doing good work in those upstreams. Being able to build node from source makes sense. That's what google does.

@alexeagle
Copy link
Collaborator

I guess one trick will be that we need node available as a repository rule so you can't use regular actions to build it. The bootstrapping might be hard.

@alexeagle
Copy link
Collaborator

@yesudeep are you still working on this?

If so, you should target the 5.x branch at this point. It has a new toolchain implementation and you won't want to have to port your fix after it lands.

@alexeagle alexeagle added the Can Close? We will close this in 30 days if there is no further activity label Jan 9, 2022
@knz
Copy link

knz commented Jan 14, 2022

I've been able to use these patches on top of 4.4.6.
(Need to create my own binary release of nodeJS for freebsd, but otherwise the patch looks good)

So this work is valuable.

@alexeagle
Copy link
Collaborator

This can be closed with the 5.0 release, since you can now register whatever nodejs toolchains you like not needing any code changes in rules_nodejs. See the /examples/vendored_node_and_yarn

@github-actions github-actions bot removed the Can Close? We will close this in 30 days if there is no further activity label Jan 15, 2022
@knz
Copy link

knz commented Jan 15, 2022

you can now register whatever nodejs toolchains you like not needing any code changes in rules_nodejs. See the /examples/vendored_node_and_yarn

Can you say more? How does this allow one to use a custom non-linux version?

@alexeagle
Copy link
Collaborator

@knz you'd fetch nodejs source code, build it with something like rules_foreign_cc, then the resulting binary can be registered in the toolchain.

@knz
Copy link

knz commented Apr 6, 2022

Thanks Alex

There are two parts here:

build it with something like rules_foreign_cc

This sounds like something regular, so I guess I could do my own research, but if you have examples that would be swell.

then the resulting binary can be registered in the toolchain.

This is the part that stumps me. What does "registered in" mean? What is the thing we need to modify to register a binary?

@mari-crl
Copy link
Contributor

mari-crl commented Apr 7, 2022

This is the part that stumps me. What does "registered in" mean? What is the thing we need to modify to register a binary?

Full docs here:
https://bazel.build/docs/toolchains#toolchain-definitions
https://github.com/bazelbuild/rules_nodejs/blob/stable/docs/Toolchains.md

But the tl;dr is that you need:

  1. A way of getting a node binary (the aforementioned rules_foreign_cc build rule will do nicely)
  2. A node_toolchain rule (which are documented here: https://github.com/bazelbuild/rules_nodejs/blob/stable/docs/Core.md#node_toolchain) which depends on your binary from step 1 as its target_tool
  3. A toolchain rule that depends on your node_toolchain rule from step 2 as its toolchain and on @rules_nodejs//nodejs:toolchain_type as its toolchain_type and has appropriate platform restrictions
  4. A register_toolchains function call in your WORKSPACE that refers to the toolchain rule from step 3

If necessary, you can substitute building the node binary as part of the build with using a locally installed version by skipping step 1 and replacing step 2 with:

  1. A node_toolchain rule which has the path of the system binary as its target_tool_path

@mari-crl
Copy link
Contributor

mari-crl commented Apr 7, 2022

if you have examples that would be swell.

There's an example in the CockroachDB repository here (actually, this whole BUILD file is deps using foreign build systems via various rules_foreign_cc):
https://github.com/cockroachdb/cockroach/blob/b038da197ead4fc73592b8033aae77d6d6eddebb/c-deps/BUILD.bazel#L5-L46

@alexeagle
Copy link
Collaborator

Awesome, thanks @mari-crl ! Do you have a few minutes to add your notes to the documentation? I guess probably here https://github.com/bazelbuild/rules_nodejs/blob/stable/docs/Core.md#node_toolchain

@mari-crl mari-crl mentioned this pull request Apr 7, 2022
12 tasks
@rickystewart
Copy link
Contributor

rickystewart commented Apr 15, 2022

Today I've been playing around with trying to get a locally-installed FreeBSD toolchain working following the steps in #3396 and the example at https://github.com/bazelbuild/rules_nodejs/tree/stable/examples/vendored_node_and_yarn. Without any changes to upstream, the build will still fail in yarn_install:

ERROR: Error fetching repository: Traceback (most recent call last):
        File "/usr/home/ricky/.cache/bazel/_bazel_ricky/5066011bb5c40bf5236d56a5083f06db/external/build_bazel_rules_nodejs/internal/npm_install/npm_install.bzl", line 825, column 36, in _yarn_install_impl
                is_windows_host = is_windows_os(repository_ctx)
        File "/usr/home/ricky/.cache/bazel/_bazel_ricky/5066011bb5c40bf5236d56a5083f06db/external/rules_nodejs/nodejs/private/os_name.bzl", line 65, column 19, in is_windows_os
                return os_name(rctx) == OS_NAMES[0]
        File "/usr/home/ricky/.cache/bazel/_bazel_ricky/5066011bb5c40bf5236d56a5083f06db/external/rules_nodejs/nodejs/private/os_name.bzl", line 62, column 9, in os_name
                fail("Unsupported operating system {} architecture {}".format(os_name, arch))
Error in fail: Unsupported operating system freebsd architecture amd64

With the following patch on top of 5.4.0 I can get the build to make some progress, but it is janky:

diff --git a/nodejs/private/os_name.bzl b/nodejs/private/os_name.bzl
index b9fb286f..0a031d9e 100644
--- a/nodejs/private/os_name.bzl
+++ b/nodejs/private/os_name.bzl
@@ -25,6 +25,7 @@ OS_ARCH_NAMES = [
     ("linux", "arm64"),
     ("linux", "s390x"),
     ("linux", "ppc64le"),
+    ("freebsd", "amd64"),
 ]
 
 OS_NAMES = ["_".join(os_arch_name) for os_arch_name in OS_ARCH_NAMES]
@@ -58,6 +59,9 @@ def os_name(rctx):
             return OS_NAMES[5]
         elif arch == "ppc64le":
             return OS_NAMES[6]
+    elif os_name.startswith("freebsd"):
+        if arch == "amd64":
+            return OS_NAMES[7]
 
     fail("Unsupported operating system {} architecture {}".format(os_name, arch))
 

A more elegant fallthrough in the case that someone is running rules_nodejs on an unsupported OS rather than just failing outright would be nice.

Then it is necessary to define a new repository named [something]_freebsd_amd64 that exposes a bin/node target and to set node_repository = "[something]" in the call to yarn_install. It's pretty odd IMO that I still have to provide a repository that contains the node binary when I've already defined and registered a toolchain capturing the local node, but if you don't do this you get the following error:

ERROR: Error fetching repository: Traceback (most recent call last):
        File "/usr/home/ricky/.cache/bazel/_bazel_ricky/5066011bb5c40bf5236d56a5083f06db/external/rules_nodejs/nodejs/yarn_repositories.bzl", line 138, column 43, in _yarn_repositories_impl
                node = repository_ctx.path(node_label),
Error in path: Unable to load package for @nodejs_freebsd_amd64//:bin/node: The repository '@nodejs_freebsd_amd64' could not be resolved

I also had trouble telling rules_nodejs where to find a globally installed yarn. Out of the box the yarn kwarg to yarn_install takes a label and not a path, but AFAICT that label cannot refer to a genrule or anything other than an actual vendored source file. I tried defining a genrule that exposes the yarn executable (by copying it from /usr/local/bin), but the library doesn't appear to be able to deal with that (in this example my genrule is at //build/toolchains:yarn):

ERROR: Error fetching repository: Traceback (most recent call last):
        File "/usr/home/ricky/.cache/bazel/_bazel_ricky/5066011bb5c40bf5236d56a5083f06db/external/build_bazel_rules_nodejs/internal/npm_install/npm_install.bzl", line 839, column 40, in _yarn_install_impl
                yarn_version = _detect_yarn_version(repository_ctx, yarn_cmd)
        File "/usr/home/ricky/.cache/bazel/_bazel_ricky/5066011bb5c40bf5236d56a5083f06db/external/build_bazel_rules_nodejs/internal/npm_install/npm_install.bzl", line 815, column 26, in _detect_yarn_version
                result = rctx.execute(yarn + ["--version"])
Error in execute: Not a regular file: /usr/home/ricky/cockroach/build/toolchains/yarn

However, in my case, I also couldn't just cp /usr/local/bin/yarn.js into the tree because then you get a bunch of import errors from yarn trying to resolve imports and not finding them in ../lib. I was able to get the build to make progress by not setting yarn at all. Presumably rules_nodejs downloads some version of yarn in this case.

Altogether I have a patch that appears to be more or less half-working which I'll post over at cockroachdb/cockroach#74208.

@rickystewart
Copy link
Contributor

Update on the above, we have a change in cockroach to allow FreeBSD support. I did have to apply that patch to rules_nodejs that I mentioned above, and exposing a nodejs_freebsd_amd64 repo is still necessary (in our case I made it a new_local_repository).

@alexeagle
Copy link
Collaborator

@rickystewart sounds like good progress, do you want to land some changes in rules_nodejs so you don't have to carry that patch? looks like this PR needs a rebase or start a new one.

@rickystewart
Copy link
Contributor

@alexeagle Sure, I opened #3490.

@alexeagle
Copy link
Collaborator

and exposing a nodejs_freebsd_amd64 repo is still necessary (in our case I made it a new_local_repository).

Is there still something to add to rules_nodejs to provide this repo? Just a matter of adding some documentation to the toolchains page saying how a user could do it? Or can we close this one now?

@rickystewart
Copy link
Contributor

rickystewart commented Jun 23, 2022

Is there still something to add to rules_nodejs to provide this repo?

I think the problem now is twofold:

  1. The problem that necessitates Teach os_name() about freebsd-amd64 #3490: most of the repository_rules call into the utility functions in os_name.bzl, and generally this causes them all to fail on any OS/arch pair that is not in OS_ARCH_NAMES. Ideally this would fall back to something else elegantly rather than just causing these functions to fail outright (maybe only failing outright if the caller failed to provide alternatives for whatever OS/arch they happen to be running, as in below).
  2. On OS/arch combinations that rules_nodejs doesn't support natively, node_repositories could synthesize a @nodejs_{OS}_{ARCH} repository. I can declare a node_toolchain with a target_tool_path of /usr/local/bin/node, or a target_tool of label; probably node_repositories should allow me to specify something similar, maybe parameterized by OS/arch?
node_repositories(
    target_tool_paths = {
        "16.13.0-freebsd_amd64": "/usr/local/bin/node",
    },
    node_version = "16.13.0",
)

In this case the idea is that on freebsd_amd64, node_repositories would automatically synthesize a @nodejs_freebsd_amd64 with bin/node copied from my local /usr/local/bin/node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants