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

Recognize get_cpu_value "darwin_arm64" #191

Merged
merged 9 commits into from
May 3, 2022
Merged

Recognize get_cpu_value "darwin_arm64" #191

merged 9 commits into from
May 3, 2022

Conversation

aherrmann
Copy link
Member

On an M1 Mac get_cpu_value will return darwin_arm64 instead of darwin.
This PR adapts our platform constraint detection to take that case into account.

Note, if bazel runs as an x86_64 binary under Rosetta, then get_cpu_value will still report darwin.
So, under Rosetta this will consistently use x86_64 constraints.

This also removes an unused instance of get_cpu_value in the Go toolchain.

Copy link
Contributor

@AleksanderGondek AleksanderGondek 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...
In regards to M11 CI issues - I do not have hardware to investigate

@YorikSar
Copy link
Member

YorikSar commented Mar 4, 2022

I've tried running tests on my machine with Rosetta, got the same errors for tests. From logs, both python2-test and python3-test failed because of wrong Python executable (or rather symlink):

 % cat /private/var/tmp/_bazel_tweag/27194dc9430cdb4660c09c5ecb36f088/execroot/io_tweag_rules_nixpkgs/bazel-out/darwin-py2-fastbuild/testlogs/external/nixpkgs_python_configure_test/python2-test/test.log
exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from @nixpkgs_python_configure_test//:python2-test
-----------------------------------------------------------------------------
Python interpreter is not provided by the toolchain.
Expected: '/nix/store/k8z8arjfcnap8l8vry9vmw9dfdi9vsbq-python-2.7.18/bin/python'
Actual:   '/nix/store/k8z8arjfcnap8l8vry9vmw9dfdi9vsbq-python-2.7.18/bin/python2'.
 % cat /private/var/tmp/_bazel_tweag/27194dc9430cdb4660c09c5ecb36f088/execroot/io_tweag_rules_nixpkgs/bazel-out/darwin-fastbuild/testlogs/external/nixpkgs_python_configure_test/python3-test/test.log 
exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from @nixpkgs_python_configure_test//:python3-test
-----------------------------------------------------------------------------
Python interpreter is not provided by the toolchain.
Expected: '/nix/store/x0fw0l4d6zwgfdwbpp23iwhm3c6a1hh3-python3-3.9.6/bin/python'
Actual:   '/nix/store/x0fw0l4d6zwgfdwbpp23iwhm3c6a1hh3-python3-3.9.6/bin/python3'.

I don't quite understand how that could happen. Both python and python2 symlinks point to python2.7 binary, for example. It means that it's not just something following the symlink or smth.

I'll check the failure in examples a bit later.


Without Rosetta, when I specified aarch64-darwin in shell.nix, I got these errors:

ERROR: While resolving toolchains for target @rules_sh//sh/posix:make_variables: no matching toolchains found for types @rules_sh//sh/posix:toolchain_type
ERROR: Analysis of target '//docs:update-readme' failed; build aborted: no matching toolchains found for types @rules_sh//sh/posix:toolchain_type

I guess, rules_sh isn't ready for ARM yet.

@YorikSar
Copy link
Member

YorikSar commented Mar 4, 2022

In examples with Rosetta and nix:

  • cc segfaults for seemingly no reason (although on CI it failed to compile somehow)
  • cc_with_deps works just fine
  • go works fine
  • python throws ModuleNotFoundError: No module named 'cv2'
  • java fails with
ERROR: /private/var/tmp/_bazel_tweag/c30554a34370f0f38e20aa05121a8b82/external/bazel_tools/tools/jdk/BUILD:29:19: While resolving toolchains for target @bazel_tools//tools/jdk:current_java_runtime: no matching toolchains found for types @bazel_tools//tools/jdk:runtime_toolchain_type

With aarch64-darwin in shell.nix files:

  • cc fails with
ERROR: /private/var/tmp/_bazel_tweag/c97a5d687d8a262c6a2a40916956f782/external/local_config_cc/BUILD:48:19: in cc_toolchain_suite rule @local_config_cc//:toolchain: cc_toolchain_suite '@local_config_cc//:toolchain' does not contain a toolchain for cpu 'darwin_arm64'
  • everything else is the same.

Overall it seems like something isn't right with more complex toolchains there.

@ryanbujnowicz
Copy link

ryanbujnowicz commented Mar 5, 2022

Thanks for looking into this @aherrmann . I recently got an M1-based Macbook and I'm interested in getting this toolchain working. I was hacking around with your arm64 branch today and noticed:

  • os:linux was still showing up in the exec_compatible_with attribute of my cc-toolchain-darwin_arm64 cc toolchain. I think this is because the value of cpu at this line gets stomped on line 18 so it's falling back to the os:linux default.
  • Does similar support for darwin_arm64 need to happen at this line?

Thanks again.

@aherrmann
Copy link
Member Author

@ryanbujnowicz Thank you for looking into this! Yes, I think you're right on all accounts. I've pushed changes to fix it.
I've also used this opportunity to align the POSIX toolchain constraints with the other toolchains. (It's a bit of a special case because it doesn't set target constraints, see note here).

@aherrmann
Copy link
Member Author

Rebased on master

@fricklerhandwerk fricklerhandwerk removed their request for review March 25, 2022 09:04
@fricklerhandwerk
Copy link
Member

I cannot meaningfully test this, don't have M1. Removed myself from reviewers.

@andyscott
Copy link

I was able to get this working, but I had to make one additional tweak/hack to work around codesign_allocate issues (NixOS/nixpkgs#148282).

My toolchain is configured as follows:

In WORKSPACE.bazel:

nixpkgs_cc_configure(
    name = "nixpkgs_config_cc",
    repository = "@nixpkgs",
    # We presently use our own nix file here so we can patch in a workaround for
    # https://github.com/NixOS/nixpkgs/pull/148282. See the header of cc.nix for
    # more details on our tweak.
    nix_file = "//tools/toolchains/cc:cc.nix",
)

In tools/toolchains/cc/cc/nix:

let

  # Notes:
  #
  # 1. This whole file is just a rip of the cc expression from rules_nixpkgs, here:
  #    https://github.com/tweag/rules_nixpkgs/blob/master/toolchains/cc/cc.nix
  #
  # 2. Modification -- We add cctools to path so that codesign_allocate can be found in
  #    the postLinkSignHook phase on OSX M1 machines. This is a hack and likely can be
  #    removed onced https://github.com/NixOS/nixpkgs/pull/148282 is resolved.
  #

  pkgs = import <nixpkgs> { config = { }; overlays = [ ]; };

  darwinCC =
    # Work around https://github.com/NixOS/nixpkgs/issues/42059.
    # See also https://github.com/NixOS/nixpkgs/pull/41589.
    pkgs.runCommand "bazel-nixpkgs-cc-wrapper"
      {
        buildInputs = [ pkgs.makeWrapper ];
      }
      (''
        mkdir -p $out/bin
        for i in ${pkgs.stdenv.cc}/bin/*; do
          ln -sf $i $out/bin
        done
        # Override cc
        rm -f $out/bin/cc $out/bin/clang $out/bin/clang++
        makeWrapper ${pkgs.stdenv.cc}/bin/cc $out/bin/cc \
      '' +
      # Pipe tweak: see Note 1 at the top of this file!
      ''  --prefix PATH : ${pkgs.lib.makeBinPath [ pkgs.darwin.cctools ]} \
      '' +
      ''  --add-flags \
            "-Wno-unused-command-line-argument \
            -isystem ${pkgs.llvmPackages.libcxx}/include/c++/v1 \
            -F${pkgs.darwin.apple_sdk.frameworks.CoreFoundation}/Library/Frameworks \
            -F${pkgs.darwin.apple_sdk.frameworks.CoreServices}/Library/Frameworks \
            -F${pkgs.darwin.apple_sdk.frameworks.Security}/Library/Frameworks \
            -F${pkgs.darwin.apple_sdk.frameworks.Foundation}/Library/Frameworks \
            -L${pkgs.libcxx}/lib \
            -L${pkgs.darwin.libobjc}/lib"
      '');
in
pkgs.buildEnv {
  name = "bazel-nixpkgs-cc";
  ignoreCollisions = true;
  # XXX: `gcov` is missing in `/bin`.
  #   It exists in `stdenv.cc.cc` but that collides with `stdenv.cc`.
  paths =
    if pkgs.stdenv.isDarwin then
      [ (pkgs.overrideCC pkgs.stdenv darwinCC).cc pkgs.darwin.binutils ]
    else
      [ pkgs.stdenv.cc pkgs.binutils ];

  pathsToLink = [ "/bin" ];
}

To produce constraints consistent with the other toolchains.
The difference is that the posix toolchain does not set the
target_constraints.
@aherrmann
Copy link
Member Author

@andyscott Thanks for testing! Yes, indeed, full M1 support requires some more changes to the cc toolchain. The focus of this PR was solely on the get_cpu_info detection aspect. You can take a look at this PR for a full M1 configuration. The goal is to migrate the remaining needed changes to rules_nixpkgs in the future.

Copy link
Member

@avdv avdv left a comment

Choose a reason for hiding this comment

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

LGTM, but due to lack of an M1 machine I could not test this myself.

avdv added 3 commits May 2, 2022 10:46
Before, the `nixUnstable` package from nixpkgs commit
15d1011615d16a8b731adf28e2cfc33481102780 was used which would resolve to nix
version 2.4pre7534_b92f58f6.

This old version of nixpkgs from May 2020 lacks support for MacOS Big Sur (11)
which is only available as of nixpkgs release 21.11 and thus running Bazel
failed:

```
$ bazel test //...
error: cannot coerce null to a string

       at /private/var/tmp/_bazel_builder/ff9a81a9a8bde7159a03f31985635bae/external/remote_nixpkgs_for_nix_unstable/pkgs/stdenv/generic/make-derivation.nix:192:34:

          191|         // (lib.optionalAttrs (!(attrs ? name) && attrs ? pname && attrs ? version)) {
          192|           name = "${attrs.pname}-${attrs.version}";
             |                                  ^
          193|         } // (lib.optionalAttrs (stdenv.hostPlatform != stdenv.buildPlatform && !dontAddHostSuffix && (attrs ? name || (attrs ? pname && attrs ? version)))) {
(use '--show-trace' to show detailed location information)
INFO: Repository nix-unstable instantiated at:
  /Users/builder/claudio-rules_nixpkgs/WORKSPACE:41:16: in <toplevel>
  /private/var/tmp/_bazel_builder/ff9a81a9a8bde7159a03f31985635bae/external/rules_nixpkgs_core/nixpkgs.bzl:382:21: in nixpkgs_package
Repository rule _nixpkgs_package defined at:
  /private/var/tmp/_bazel_builder/ff9a81a9a8bde7159a03f31985635bae/external/rules_nixpkgs_core/nixpkgs.bzl:276:35: in <toplevel>
ERROR: An error occurred during the fetch of repository 'nix-unstable':
   Traceback (most recent call last):
        File "/private/var/tmp/_bazel_builder/ff9a81a9a8bde7159a03f31985635bae/external/rules_nixpkgs_core/nixpkgs.bzl", line 242, column 38, in _nixpkgs_package_impl
                exec_result = execute_or_fail(
        File "/private/var/tmp/_bazel_builder/ff9a81a9a8bde7159a03f31985635bae/external/rules_nixpkgs_core/util.bzl", line 59, column 13, in execute_or_fail
                fail("""
Error in fail:
Cannot build Nix attribute 'nixUnstable'.
Command: [/nix/var/nix/profiles/default/bin/nix-build, "-E", "import <nixpkgs> { config = {}; overlays = []; }", "-A", "nixUnstable", "--out-link", "bazel-support/nix-out-link", "-I", "nixpkgs=/private/var/tmp/_bazel_builder/ff9a81a9a8bde7159a03f31985635bae/external/remote_nixpkgs_for_nix_unstable/remote_nixpkgs_for_nix_unstable"]
Return code: 1
```

Switch to nix_2_4 and remove the remote_nixpkgs_for_nix_unstable repository.
@avdv
Copy link
Member

avdv commented May 3, 2022

Tested on an M1 successfully:

✔️ all tests passed.
✔️ all examples build (with Nix)
✔️ all examples produced native executables (Mach-O 64-bit executable arm64) [where applicable]
✔️ all examples ran

@avdv avdv added the merge-queue merge on green CI label May 3, 2022
@mergify mergify bot merged commit 210d30a into master May 3, 2022
@mergify mergify bot deleted the arm64 branch May 3, 2022 09:34
@mergify mergify bot removed the merge-queue merge on green CI label May 3, 2022
cbley-da added a commit to digital-asset/daml that referenced this pull request May 5, 2022
Since [rules_nixpkgs#191](tweag/rules_nixpkgs#191) has been merged, we can drop the
haskell-arm-m1.patch from rules_nixpkgs.

Also, rules_nixpkgs has been split into several components which need to be added explicitly in `deps.bzl`
cbley-da added a commit to digital-asset/daml that referenced this pull request May 6, 2022
Since [rules_nixpkgs#191] has been merged, we can drop the haskell-arm-m1.patch from rules_nixpkgs.

Also, rules_nixpkgs has been split into several components which need to be
added explicitly in `deps.bzl`, see [#182].

[#191]: tweag/rules_nixpkgs#191
[#182]: tweag/rules_nixpkgs#182
cbley-da added a commit to digital-asset/daml that referenced this pull request May 6, 2022
Since [rules_nixpkgs#191] has been merged, we can drop the `rules-nixpkgs-arm.patch` from rules_nixpkgs.

Also, rules_nixpkgs has been split into several components which need to be
added explicitly in `deps.bzl`, see [#182].

[#191]: tweag/rules_nixpkgs#191
[#182]: tweag/rules_nixpkgs#182
cbley-da added a commit to digital-asset/daml that referenced this pull request May 10, 2022
…3798)

* Update `rules_nixpgks` to HEAD

Since [rules_nixpkgs#191] has been merged, we can drop the `rules-nixpkgs-arm.patch` from rules_nixpkgs.

Also, rules_nixpkgs has been split into several components which need to be
added explicitly in `deps.bzl`, see [#182].

[#191]: tweag/rules_nixpkgs#191
[#182]: tweag/rules_nixpkgs#182

* Adapt `compatibility/deps.bzl`

* Update platforms repository to version 0.0.4

It has been updated in rules_nixpkgs, so this just follows suite.

* Pass through `isClang` attribute for the cc-toolchain

In rules_nixpkgs, this attribute is now used to determine whether the compiler is clang, see [#216].

[#216]: tweag/rules_nixpkgs#216

CHANGELOG_BEGIN
CHANGELOG_END
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.

7 participants