[fuchsia] [packaging] Layout debug symbols for Fuchsia by iskakaushik · Pull Request #13338 · flutter/engine

iskakaushik · 2019-10-24T21:25:44Z

This packages and moves the relevant symbols to the right
locations in root_out_dir, but doesn't upload them to CIPD
yet. That will be done in a following change.

Refer: go/flutter-fuchsia-packaging for more information.

This packages and moves the relevant symbols to the right locations in root_out_dir, but doesn't upload them to CIPD yet. That will be done in a following change. Refer: go/flutter-fuchsia-packaging for more information.

chinmaygarde

All minor nits. Looks good overall.

chinmaygarde · 2019-10-24T22:11:56Z

tools/fuchsia/debug_symbols.gni

+
+# Takes a binary and generates its debug symbols following
+# the Fuchsia packaging convention.
+template("debug_symbols") {


fuchsia_debug_symbols for clarity. Also, please document the invoker arguments necessary.

Makes sense

chinmaygarde · 2019-10-24T22:16:16Z

tools/fuchsia/debug_symbols.gni

+           ]
+
+    outputs = [
+      "${_dest_base}/.${target_name}_success",


I think the convention is .stamp. Also, why is the file hidden?

I was making these hidden to avoid the noise when you try to inspect the directories. .stamp seems to have a special meaning as far as gn is concerned - there is a tool (" "stamp": Tool for creating stamp files") that gn uses to generate these, so I tried to avoid that. If that is the convention, i'm happy to change it to .stamp.

chinmaygarde · 2019-10-24T22:17:28Z

tools/fuchsia/debug_symbols.gni

+  }
+
+  action(target_name) {
+    testonly = defined(invoker.testonly) && invoker.testonly


Here and elsewhere, prefer forward_variables_from

chinmaygarde · 2019-10-24T22:18:15Z

tools/fuchsia/copy_debug_symbols.py

+def main():
+  parser = argparse.ArgumentParser()
+
+  parser.add_argument(


Can you add help= arguments to these please?

chinmaygarde · 2019-10-24T22:19:40Z

tools/fuchsia/copy_debug_symbols.py

+
+
+def GetBuildIdParts(exec_path):
+  file_out = subprocess.check_output(['file', exec_path])


Dang, is there no other way to get the build ID for an ELF binary?

This doesn't seem to work on cirrus for some reason. I'm wondering if file is sandboxed away from us. I will look for alternatives.

The Fuchsia toolchain includes a version of llvm-readelf.

Try extracting the build ID with:
llvm-readelf --hex-dump=.note.gnu.build-id [executable]

Great idea, I switched to using this,

chinmaygarde · 2019-10-24T22:20:11Z

tools/fuchsia/copy_debug_symbols.py

+  assert os.path.exists(args.dest)
+
+  parts = GetBuildIdParts(args.exec_path)
+  dbg_prefix_base = '%s/%s' % (args.dest, parts['prefix_dir'])


Prefer os.path.join. Here and elsewhere.

chinmaygarde · 2019-10-24T22:20:46Z

tools/fuchsia/copy_debug_symbols.py

+
+  parser.add_argument('--stripped', dest='stripped', action='store_true')
+  parser.add_argument('--unstripped', dest='stripped', action='store_false')
+  parser.set_defaults(stripped=True)


You can specify the default in the add_argument line itself.

chinmaygarde · 2019-10-24T22:21:50Z

tools/fuchsia/copy_debug_symbols.py

+
+  args = parser.parse_args()
+  assert os.path.exists(args.exec_path)
+  assert os.path.exists(args.dest)


This is redundant with required=True.

required=True says that the argument exists, not the path.

chinmaygarde · 2019-10-24T22:23:06Z

tools/fuchsia/copy_debug_symbols.py

+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+""" Gather the build_id, prefix_dir, and exec_name given the path to executable
+    also copies to the specified destination.


Can you also add a line about how the build ID directory is structure. That section from the presentation will work great :)

chinmaygarde · 2019-10-24T22:23:46Z

tools/fuchsia/debug_symbols.gni

+  bin_path = invoker.binary_path
+
+  _args = []
+  if (defined(invoker.unstripped) && invoker.unstripped) {


I'd generally about optional invoker arguments. Especially in this case where the template is private.

…er/engine#13338)

git@github.com:flutter/engine.git/compare/7a9c86b8d5e9...89731ae git log 7a9c86b..89731ae --no-merges --oneline 2019-10-25 ditman@gmail.com Intercept SystemSound.play platform message before it's sent. (flutter/engine#13342) 2019-10-25 iska.kaushik@gmail.com [fuchsia] [packaging] Layout debug symbols for Fuchsia (flutter/engine#13338) 2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from Dr9GE... to fHxWy... (flutter/engine#13358) 2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from KC8wX... to Tbg2V... (flutter/engine#13357) 2019-10-25 ariaye@google.com Bump dart/language_model to 9fJQZ0TrnAGQKrEtuL3-AXbUfPzYxqpN_OBHr9P4hE4C (flutter/engine#13337) 2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia a7f3157ac012..d0a404e84d47 (6 commits) (flutter/engine#13356) 2019-10-25 bkonyi@google.com Roll src/third_party/dart d576ce69e1..5ba6fb73ec (3 commits) 2019-10-25 jason-simmons@users.noreply.github.com Create a separate directory for the intermediate outputs of each Fuchsia archive build action (flutter/engine#13341) 2019-10-25 jason-simmons@users.noreply.github.com Fix the output filename of the Fuchsia archive build template (flutter/engine#13339) 2019-10-25 bkonyi@google.com Roll src/third_party/dart b42c2af535..d576ce69e1 (10 commits) 2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia 24a409611f24..a7f3157ac012 (1 commits) (flutter/engine#13353) 2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia 28a8f28b3eaf..24a409611f24 (2 commits) (flutter/engine#13352) 2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from 9m5ec... to Dr9GE... (flutter/engine#13351) 2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from yuA3r... to KC8wX... (flutter/engine#13350) 2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia 6f1c20f01fa9..28a8f28b3eaf (2 commits) (flutter/engine#13348) 2019-10-25 chris@bracken.jp Expose platform view ID on embedder semantics node (flutter/engine#13345) 2019-10-25 chris@bracken.jp Remove TODO on embedder a11y unit tests (flutter/engine#13346) 2019-10-25 bkonyi@google.com Roll src/third_party/dart 1bc9fba660..b42c2af535 (12 commits) 2019-10-25 matthew-carroll@users.noreply.github.com Android embedding API updates for plugin ecosystem - plugin facade, split Lifecycle, save state callbacks to plugins (#43241, #43242, #43295) (flutter/engine#13280) 2019-10-24 skia-flutter-autoroll@skia.org Roll src/third_party/skia 740f85949db2..6f1c20f01fa9 (16 commits) (flutter/engine#13343) 2019-10-24 bkonyi@google.com Roll src/third_party/dart 1bd6e20d76..1bc9fba660 (14 commits) 2019-10-24 skia-flutter-autoroll@skia.org Roll src/third_party/skia 4ab4e641f151..740f85949db2 (12 commits) (flutter/engine#13336) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC aaclarke@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

git@github.com:flutter/engine.git/compare/7a9c86b8d5e9...89731ae git log 7a9c86b..89731ae --no-merges --oneline 2019-10-25 ditman@gmail.com Intercept SystemSound.play platform message before it's sent. (flutter/engine#13342) 2019-10-25 iska.kaushik@gmail.com [fuchsia] [packaging] Layout debug symbols for Fuchsia (flutter/engine#13338) 2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from Dr9GE... to fHxWy... (flutter/engine#13358) 2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from KC8wX... to Tbg2V... (flutter/engine#13357) 2019-10-25 ariaye@google.com Bump dart/language_model to 9fJQZ0TrnAGQKrEtuL3-AXbUfPzYxqpN_OBHr9P4hE4C (flutter/engine#13337) 2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia a7f3157ac012..d0a404e84d47 (6 commits) (flutter/engine#13356) 2019-10-25 bkonyi@google.com Roll src/third_party/dart d576ce69e1..5ba6fb73ec (3 commits) 2019-10-25 jason-simmons@users.noreply.github.com Create a separate directory for the intermediate outputs of each Fuchsia archive build action (flutter/engine#13341) 2019-10-25 jason-simmons@users.noreply.github.com Fix the output filename of the Fuchsia archive build template (flutter/engine#13339) 2019-10-25 bkonyi@google.com Roll src/third_party/dart b42c2af535..d576ce69e1 (10 commits) 2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia 24a409611f24..a7f3157ac012 (1 commits) (flutter/engine#13353) 2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia 28a8f28b3eaf..24a409611f24 (2 commits) (flutter/engine#13352) 2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from 9m5ec... to Dr9GE... (flutter/engine#13351) 2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from yuA3r... to KC8wX... (flutter/engine#13350) 2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia 6f1c20f01fa9..28a8f28b3eaf (2 commits) (flutter/engine#13348) 2019-10-25 chris@bracken.jp Expose platform view ID on embedder semantics node (flutter/engine#13345) 2019-10-25 chris@bracken.jp Remove TODO on embedder a11y unit tests (flutter/engine#13346) 2019-10-25 bkonyi@google.com Roll src/third_party/dart 1bc9fba660..b42c2af535 (12 commits) 2019-10-25 matthew-carroll@users.noreply.github.com Android embedding API updates for plugin ecosystem - plugin facade, split Lifecycle, save state callbacks to plugins (flutter#43241, flutter#43242, flutter#43295) (flutter/engine#13280) 2019-10-24 skia-flutter-autoroll@skia.org Roll src/third_party/skia 740f85949db2..6f1c20f01fa9 (16 commits) (flutter/engine#13343) 2019-10-24 bkonyi@google.com Roll src/third_party/dart 1bd6e20d76..1bc9fba660 (14 commits) 2019-10-24 skia-flutter-autoroll@skia.org Roll src/third_party/skia 4ab4e641f151..740f85949db2 (12 commits) (flutter/engine#13336) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC aaclarke@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

[fuchsia] [packaging] Layout debug symbols for Fuchsia

4047c00

This packages and moves the relevant symbols to the right locations in root_out_dir, but doesn't upload them to CIPD yet. That will be done in a following change. Refer: go/flutter-fuchsia-packaging for more information.

googlebot added the cla: yes label Oct 24, 2019

iskakaushik requested review from chinmaygarde, gw280 and jason-simmons October 24, 2019 21:25

iskakaushik added the CQ+1 label Oct 24, 2019

Add retry to makedirs

3409874

iskakaushik added CQ+1 and removed CQ+1 labels Oct 24, 2019

chinmaygarde reviewed Oct 24, 2019

View reviewed changes

Kaushik Iska added 5 commits October 24, 2019 15:49

use forward_args_from and also rename to "fuchsia_debug_symbols"

90fff54

add help statements and use os.path.join the copy debug symbols script

230b8bc

Add debug logs for "file" command

cc7fc9f

use llvm-readelf from Fuchsia toolchain

ab7ca90

make them hidden again

65e1777

chinmaygarde approved these changes Oct 25, 2019

View reviewed changes

iskakaushik merged commit 2250c74 into flutter:master Oct 25, 2019

iskakaushik deleted the debug-symbols-for-fuchsia branch October 25, 2019 19:08

engine-flutter-autoroll mentioned this pull request Oct 25, 2019

Roll engine 7a9c86b8d5e9..89731aefca5b (22 commits) flutter/flutter#43541

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 25, 2019

2250c74 [fuchsia] [packaging] Layout debug symbols for Fuchsia (flutt…

c9e0197

…er/engine#13338)



		def GetBuildIdParts(exec_path):
		file_out = subprocess.check_output(['file', exec_path])

Comments

Conversation

iskakaushik commented Oct 24, 2019

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants