[Fuchsia] Run fuchsia-gn-sdk from chromium [crashpad/crashpad : main]
Zijie He (Gerrit)
Zijie He added 1 comment![]( "Open in Gerrit")
Related details
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He added 1 comment![]( "Open in Gerrit")
import("//third_party/fuchsia-gn-sdk/src/gn_configs.gni")The fix is not very ideal, but at least it works. The problem here is that crashpad highly relies on the mini_chromium, but later one has no access to most of the shared components in chromium.
Related details
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 7 comments![]( "Open in Gerrit")
/third_party/fuchsia-gn-sdkIf you do an ASCII sort, `-` is before `/`.
'rm',How’s this work on Windows?
You might want a `condition` at least.
is_fuchsia = crashpad_is_fuchsiaWorth a comment saying why you need this, because we don’t normally define things like this out of the `crashpad_` namespace.
def Main():Accept an `args` parameter, which you’d pass in from the bottom of this script as `sys.argv[1:]`. Use that on line 46.
with subprocess.Popen(sys.argv[1:]) as proc:Why not just `return subprocess.call(…)`?
Related details
- Zijie He
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He added 7 comments![]( "Open in Gerrit")
If you do an ASCII sort, `-` is before `/`.
Done
How’s this work on Windows?
You might want a `condition` at least.
Ooops, I didn't know windows was supported.
Worth a comment saying why you need this, because we don’t normally define things like this out of the `crashpad_` namespace.
Accept an `args` parameter, which you’d pass in from the bottom of this script as `sys.argv[1:]`. Use that on line 46.
Done
Just `main`, lowercase m.
Done
with subprocess.Popen(sys.argv[1:]) as proc:Why not just `return subprocess.call(…)`?
So if this process is killed (Process.kill in dart [1], likely also other similar commands), the processes it spawns won't receive the sigterm and may break their functionalities. So the proc.terminate() explicitly sends the sigterm to the spawned processes.
I think it may be OK in crashpad, but this file is almost copied from other repos like dart [2]. I simply kept them consistent.
[1]: https://api.flutter.dev/flutter/dart-io/Process/kill.html
[2]: https://source.corp.google.com/h/dart/sdk/+/main:build/fuchsia/with_envs.py;bpv=1;bpt=0
Related details
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
with subprocess.Popen(sys.argv[1:]) as proc:Zijie HeWhy not just `return subprocess.call(…)`?
So if this process is killed (Process.kill in dart [1], likely also other similar commands), the processes it spawns won't receive the sigterm and may break their functionalities. So the proc.terminate() explicitly sends the sigterm to the spawned processes.
I think it may be OK in crashpad, but this file is almost copied from other repos like dart [2]. I simply kept them consistent.
[1]: https://api.flutter.dev/flutter/dart-io/Process/kill.html
[2]: https://source.corp.google.com/h/dart/sdk/+/main:build/fuchsia/with_envs.py;bpv=1;bpt=0
So if this process is killed (Process.kill in dart [1], likely also other similar commands), the processes it spawns won't receive the sigterm and may break their functionalities. So the proc.terminate() explicitly sends the sigterm to the spawned processes.
- Zijie He
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zijie He
| Commit-Queue | +1 |
1 comment
with subprocess.Popen(sys.argv[1:]) as proc:Zijie HeWhy not just `return subprocess.call(…)`?
Mark MentovaiSo if this process is killed (Process.kill in dart [1], likely also other similar commands), the processes it spawns won't receive the sigterm and may break their functionalities. So the proc.terminate() explicitly sends the sigterm to the spawned processes.
I think it may be OK in crashpad, but this file is almost copied from other repos like dart [2]. I simply kept them consistent.
[1]: https://api.flutter.dev/flutter/dart-io/Process/kill.html
[2]: https://source.corp.google.com/h/dart/sdk/+/main:build/fuchsia/with_envs.py;bpv=1;bpt=0
So if this process is killed (Process.kill in dart [1], likely also other similar commands), the processes it spawns won't receive the sigterm and may break their functionalities. So the proc.terminate() explicitly sends the sigterm to the spawned processes.
I don’t think that’s relevant here, and I’d prefer the simpler form.
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
3 comments
from typing import ListWe don’t really do this here.
os.path.join(os.path.dirname(__file__), '../'))Is the trailing slash necessary? `os.pardir` if not.
Related details
- Zijie He
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He added 2 comments![]( "Open in Gerrit")
We don’t really do this here.
Oh, I see.
Is the trailing slash necessary? `os.pardir` if not.
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zijie He (Gerrit)
Zijie He voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zijie He
| Commit-Queue | +2 |
1 comment
Related details
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Crashpad LUCI CQ (Gerrit)
Crashpad LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
9 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: build/fuchsia_envs.py
Insertions: 2, Deletions: 3.
@@ -18,10 +18,9 @@
import platform
import subprocess
import sys
-from typing import List
-def main(args: List[str]):
+def main(args):
"""
Executes the test-scripts with required environment variables. It acts like
/usr/bin/env, but provides some extra functionality to dynamically set up
@@ -31,7 +30,7 @@
args: the command line arguments without the script name itself.
"""
os.environ['SRC_ROOT'] = os.path.abspath(
- os.path.join(os.path.dirname(__file__), '../'))
+ os.path.join(os.path.dirname(__file__), '..'))
assert platform.system() == 'Linux', 'Unsupported OS ' + platform.system()
os.environ['FUCHSIA_SDK_ROOT'] = os.path.join(
```
Change information
[Fuchsia] Run fuchsia-gn-sdk from chromium
The latest build rules have the ffuchsia-api-level.
Need https://crrev.com/c/5586319.
- M .gitignore
- M DEPS
- M build/BUILDCONFIG.gn
- A build/config/fuchsia/gn_configs.gni
- M build/crashpad_buildconfig.gni
- D build/fuchsia/gen_build_defs.py
- A build/fuchsia_envs.py
Code-Review: +1 by Mark Mentovai
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |