Install ninja to crashpad using DEPS [crashpad/crashpad : main]

Junji Watanabe (Gerrit)

unread,

Nov 1, 2022, 9:11:10 PM11/1/22

to Mark Mentovai, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Mark Mentovai.

View Change

2 comments:

Patchset:
- Patch Set #5:
  Let's use this CL to land. I will abandon the previous one.
File DEPS:
- Patch Set #5, Line 164: 'crashpad/third_party/ninja/mac': {
  You’ll want to separate mac-arm64 and mac-amd64.
  I can imagine that Chrome Mac team wants to work on the both architectures at the same time.
  But, how should ninja wrapper detect which ninja binary to use?
  The current implementation is
  - when the host machine is arm, -> install and use mac-arm64 ninja
  - when the host machine is intel, -> install and use mac-amd64 ninja
  If you just want to install the both ninja binaries, and call the ninja without the wrapper. That should be fine.
  e.g. "./third_party/ninja/mac-amd64/ninja -C out/Default" on ARM Mac.

To view, visit change 3990128. To unsubscribe, or for help writing mail filters, visit settings.

Mark Mentovai (Gerrit)

unread,

Nov 2, 2022, 9:29:10 AM11/2/22

to Junji Watanabe, Richard Wang, Fumitoshi Ukai, Takuto Ikuta, Philipp Wollermann, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Junji Watanabe.

View Change

2 comments:

Patchset:
- Patch Set #7:
  Unless I’ve misunderstood the question?
File DEPS:

- Patch Set #5, Line 164: 'crashpad/third_party/ninja/mac': {

- I can imagine that Chrome Mac team wants to work on the both architectures at the same time.

- But, how should ninja wrapper detect which ninja binary to use?

- Just like you use `uname -s` to detect the OS, you can use `uname -m` to detect the architecture. It will be either `x86_64` or `arm64`.

To view, visit change 3990128. To unsubscribe, or for help writing mail filters, visit settings.

Junji Watanabe (Gerrit)

unread,

Nov 2, 2022, 9:53:24 AM11/2/22

to Richard Wang, Fumitoshi Ukai, Takuto Ikuta, Philipp Wollermann, Mark Mentovai, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Mark Mentovai.

View Change

1 comment:

File DEPS:

- Patch Set #5, Line 164: 'crashpad/third_party/ninja/mac': {

- > I can imagine that Chrome Mac team wants to work on the both architectures at the same time. […]
  I thought you wanted to use x86_64 ninja binary from ARM Mac via Rosetta for some reason. In that case, you will need to install 2 ninja binaries in different paths.
  But if your request is not the case, I will just update the installation paths to be aligned with CIPD's platform name. e.g. mac-arm64, mac-amd64.

To view, visit change 3990128. To unsubscribe, or for help writing mail filters, visit settings.

Junji Watanabe (Gerrit)

unread,

Nov 4, 2022, 2:14:42 AM11/4/22

to Richard Wang, Fumitoshi Ukai, Takuto Ikuta, Philipp Wollermann, Mark Mentovai, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Junji Watanabe, Mark Mentovai.

Patch set 9:Commit-Queue +1

View Change

1 comment:

File DEPS:

- Patch Set #5, Line 164: 'crashpad/third_party/ninja/mac': {

- I thought you wanted to use x86_64 ninja binary from ARM Mac via Rosetta for some reason. […]
  I updated the CL to separate the paths to mac-amd64 and mac-arm64. PTAL.

To view, visit change 3990128. To unsubscribe, or for help writing mail filters, visit settings.

Mark Mentovai (Gerrit)

unread,

Nov 4, 2022, 9:16:44 AM11/4/22

to Junji Watanabe, Richard Wang, Fumitoshi Ukai, Takuto Ikuta, Philipp Wollermann, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Junji Watanabe.

View Change

15 comments:

Patchset:
- Patch Set #10:
  Thanks, Junji!
File third_party/ninja/ninja:
- Patch Set #10, Line 1: #!/usr/bin/env bash
  Just `#!/bin/sh`. You’re not using any bash extensions.
- Patch Set #10, Line 3: # Copyright (c) 2022 Google Inc. All rights reserved.
  This is not the current boilerplate form, and you’ll trip over some presubmit at some point if you use this old form. Please borrow from a nearby file:
```
 - Remove the `(c)`.
 - Remove the period at the end of this sentence.
```
- Patch Set #10, Line 6:
  Best to run under `set -e`. Even better to run under `set -eu`.
- Patch Set #10, Line 8: ARCH="$(uname -m)"
  I wouldn’t bother grabbing this until you’re in the Darwin case below. No reason to fetch it (for now) on Linux or any other OS.
- Patch Set #10, Line 12: cat <<-EOF
  You haven’t used tab characters in the here-document, so the `<<-` form doesn’t get you anything. This can just be `<<` as in `<<EOF`.
- Patch Set #10, Line 19: EOF
  Send to stderr: `>&2`.
- Patch Set #10, Line 22: case "$OS" in
  `"${OS}"`
- Patch Set #10, Line 26: case "$ARCH" in
  `"${ARCH}"`
- Patch Set #10, Line 32: echo "Unsupported architecture ${ARCH}"
  Use stderr: `>&2`.
- Patch Set #10, Line 38: exec cmd.exe /c $(cygpath -t windows $0).exe "$@";;
  1. Use `THIS_DIR`.
  2. Quote `${THIS_DIR}/$0`.
  3. Quote the `$(…)` command substitution.
```
```
      exec cmd.exe /c "$(cygpath -t windows "${THIS_DIR}/$0").exe" "$@";;
```
```
- Patch Set #10, Line 40: cmd.exe //c $0.exe "$@";;
  1. No `exec`?
  2. Is the double `/` in `//c` intentional?
  3. Can you use `THIS_DIR`?
  4. Quote `${THIS_DIR}/$0.exe`.
- Patch Set #10, Line 37:
```
  CYGWIN*)
    exec cmd.exe /c $(cygpath -t windows $0).exe "$@";;
  MINGW*)
    cmd.exe //c $0.exe "$@";;
  MSYS_NT*)
    cmd.exe //c $0.exe "$@";;
```
```
If you’re not entirely certain about the Cygwin, MinGW, or MSYS parts, I think it’s fine to leave them out. Two reasons:
 1. I wouldn’t expect these cases to ever get hit in this script if `depot_tools` still has a `ninja` wrapper that goes straight to `ninja.exe` on Windows.
 2. I don’t think that those three environments are significant to anyone doing Crashpad work  on Windows.
```
- Patch Set #10, Line 39:
```
  MINGW*)
    cmd.exe //c $0.exe "$@";;
  MSYS_NT*)
    cmd.exe //c $0.exe "$@";;
```
  These two cases are the same, so merge them: `MINGW*|MSYS_NT*)`.
- Patch Set #10, Line 44: echo "Unsupported OS ${OS}"
  Send this to stderr too.

To view, visit change 3990128. To unsubscribe, or for help writing mail filters, visit settings.

Junji Watanabe (Gerrit)

unread,

Nov 7, 2022, 1:47:01 AM11/7/22

to Richard Wang, Fumitoshi Ukai, Takuto Ikuta, Philipp Wollermann, Mark Mentovai, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Mark Mentovai.

View Change

15 comments:

Patchset:
- Patch Set #11:
  Thank you for your thoughtful feedback!
  PTAL
File third_party/ninja/ninja:

- Patch Set #10, Line 1: #!/usr/bin/env bash
  Just `#!/bin/sh`. You’re not using any bash extensions.

- Done

- Patch Set #10, Line 3: # Copyright (c) 2022 Google Inc. All rights reserved.

- This is not the current boilerplate form, and you’ll trip over some presubmit at some point if you u […]
  Done

- Patch Set #10, Line 6:
  Best to run under `set -e`. Even better to run under `set -eu`.

- Done

- Patch Set #10, Line 8: ARCH="$(uname -m)"

- I wouldn’t bother grabbing this until you’re in the Darwin case below. […]
  Done

- Patch Set #10, Line 12: cat <<-EOF

- You haven’t used tab characters in the here-document, so the `<<-` form doesn’t get you anything. […]
  Done

- Patch Set #10, Line 19: EOF
  Send to stderr: `>&2`.

- Done

- Patch Set #10, Line 22: case "$OS" in
  `"${OS}"`

- Done

- Patch Set #10, Line 26: case "$ARCH" in
  `"${ARCH}"`

- Done

- Patch Set #10, Line 32: echo "Unsupported architecture ${ARCH}"
  Use stderr: `>&2`.

- Done

- Patch Set #10, Line 38: exec cmd.exe /c $(cygpath -t windows $0).exe "$@";;

- 1. Use `THIS_DIR`. […]
  removed

- Patch Set #10, Line 40: cmd.exe //c $0.exe "$@";;

- 1. No `exec`? […]
  removed

Patch Set #10, Line 39:

  MINGW*)
    cmd.exe //c $0.exe "$@";;
  MSYS_NT*)
    cmd.exe //c $0.exe "$@";;

These two cases are the same, so merge them: `MINGW*|MSYS_NT*)`.

- removed

Patch Set #10, Line 37:

  CYGWIN*)
    exec cmd.exe /c $(cygpath -t windows $0).exe "$@";;
  MINGW*)
    cmd.exe //c $0.exe "$@";;
  MSYS_NT*)
    cmd.exe //c $0.exe "$@";;

- If you’re not entirely certain about the Cygwin, MinGW, or MSYS parts, I think it’s fine to leave th […]
  Sounds good. I removed those cases.

- Patch Set #10, Line 44: echo "Unsupported OS ${OS}"
  Send this to stderr too.

- Done

To view, visit change 3990128. To unsubscribe, or for help writing mail filters, visit settings.

Mark Mentovai (Gerrit)

unread,

Nov 7, 2022, 9:19:16 AM11/7/22

to Junji Watanabe, Richard Wang, Fumitoshi Ukai, Takuto Ikuta, Philipp Wollermann, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Junji Watanabe.

Patch set 11:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #11:
  Thanks!

To view, visit change 3990128. To unsubscribe, or for help writing mail filters, visit settings.

Crashpad LUCI CQ (Gerrit)

unread,

Nov 7, 2022, 8:32:52 PM11/7/22

to Junji Watanabe, Mark Mentovai, Richard Wang, Fumitoshi Ukai, Takuto Ikuta, Philipp Wollermann, crashp...@chromium.org

Crashpad LUCI CQ submitted this change.

View Change

Approvals:
  Junji Watanabe: Commit
  Mark Mentovai: Looks good to me

Install ninja to crashpad using DEPS

Ninja will be installed to the following paths.
- Linux: third_party/ninja/linux/ninja
- Mac: third_party/ninja/mac/ninja
- Windows: third_party/ninja/ninja.exe

This supports a workflow with VMs on the same host machine.

On Unix, `ninja` command wrapper in depot_tools will trigger third_party/ninja/ninja, which call linux or mac ninja.
On Windows, the depot_tools wrapper will trigger third_party/ninja/ninja.exe.

See the the discussions on the previous CL https://crrev.com/c/3924593 for more context.

See also chromium/src's CL https://crrev.com/c/3869740 for CIPD ninja migration.

Bug: chromium:1340825
Change-Id: Ia4ff83b4fdc5cb07b5c737cb9d00eaa167f0ffb0
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3990128
Commit-Queue: Junji Watanabe <jw...@google.com>
Reviewed-by: Mark Mentovai <ma...@chromium.org>
---
M .gitignore
M DEPS
A third_party/ninja/README.crashpad
A third_party/ninja/ninja
4 files changed, 138 insertions(+), 0 deletions(-)

To view, visit change 3990128. To unsubscribe, or for help writing mail filters, visit settings.

Junji Watanabe (Gerrit)

unread,

Nov 7, 2022, 8:33:34 PM11/7/22

to Mark Mentovai, Richard Wang, Fumitoshi Ukai, Takuto Ikuta, Philipp Wollermann, Crashpad LUCI CQ, crashp...@chromium.org

Patch set 11:Commit-Queue +2

View Change

1 comment:

Patchset:
- Patch Set #11:
  Thank you!

To view, visit change 3990128. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward