Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1618243004:
Use gn_helpers to deserialize GN lists. (Closed)
DescriptionUse gn_helpers to deserialize GN lists.
The Android build code doesn't properly unescape the GN lists, use the official Python GN parser instead of ast for the zip script.
Document what should be done for build_utils, but don't fix it due to cast shell build errors as explained in the comment I added.
Update mojo zip script to use the gn helpers.
BUG=573132, 571022
Committed: https://crrev.com/81687d8362fd2dbf0acab8f1deee5b91ee1e5a49
Cr-Commit-Position: refs/heads/master@{#372461}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : Revert zip change #Patch Set 7 : Put back depfile #Patch Set 8 : #
Total comments: 13
Patch Set 9 : #
Dependent Patchsets: Messages
Total messages: 54 (23 generated)
The CQ bit was checked by brettw@chromium.org to run a CQ dry run
brettw@chromium.org changed reviewers: + agrieve@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1618243004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1618243004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Looks like this isn't right. Will do another pass.
Description was changed from ========== Use gn_helpers to deserialize GN lists. The Android build code doesn't properly unescape the GN lists, use the official Python GN parser instead of ast. BUG=573132, 571022 ========== to ========== Use gn_helpers to deserialize GN lists. The Android build code doesn't properly unescape the GN lists, use the official Python GN parser instead of ast for the zip script. Document what should be done for build_utils, but don't fix it due to cast shell build errors as explained in the comment I added. Remove the Mojo zip script and use the shared one in build. The Mojo one had some additions to the Android one that are never used, I assume this is from the old forked Mojo repo. Clean up and document the zip wrapper template and use this in all places. BUG=573132, 571022 ==========
The CQ bit was checked by brettw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1618243004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1618243004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by brettw@chromium.org to run a CQ dry run
This is ready to review now.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1618243004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1618243004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by brettw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1618243004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1618243004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by brettw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1618243004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1618243004/100001
The CQ bit was checked by brettw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1618243004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1618243004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
I'm having trouble figuring out what's up with Android. I'm going to split this up.
Description was changed from ========== Use gn_helpers to deserialize GN lists. The Android build code doesn't properly unescape the GN lists, use the official Python GN parser instead of ast for the zip script. Document what should be done for build_utils, but don't fix it due to cast shell build errors as explained in the comment I added. Remove the Mojo zip script and use the shared one in build. The Mojo one had some additions to the Android one that are never used, I assume this is from the old forked Mojo repo. Clean up and document the zip wrapper template and use this in all places. BUG=573132, 571022 ========== to ========== Use gn_helpers to deserialize GN lists. The Android build code doesn't properly unescape the GN lists, use the official Python GN parser instead of ast for the zip script. Document what should be done for build_utils, but don't fix it due to cast shell build errors as explained in the comment I added. Update mojo zip script to use the gn helpers. BUG=573132, 571022 ==========
The CQ bit was checked by brettw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1618243004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1618243004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Okay, this is ready!
brettw@chromium.org changed reviewers: + dpranke@chromium.org - agrieve@chromium.org
I think agrieve may be out -> dpranke
dpranke@chromium.org changed reviewers: + msw@chromium.org
+msw for a question ... https://codereview.chromium.org/1618243004/diff/140001/build/android/gyp/util... File build/android/gyp/util/build_utils.py (right): https://codereview.chromium.org/1618243004/diff/140001/build/android/gyp/util... build/android/gyp/util/build_utils.py:90: # this function. ick. I suppose fixing the CastShell is part of this is harder? https://codereview.chromium.org/1618243004/diff/140001/build/config/zip.gni File build/config/zip.gni (left): https://codereview.chromium.org/1618243004/diff/140001/build/config/zip.gni#o... build/config/zip.gni:8: set_sources_assignment_filter([]) Are you sure you don't need this line? https://codereview.chromium.org/1618243004/diff/140001/build/gn_helpers.py File build/gn_helpers.py (right): https://codereview.chromium.org/1618243004/diff/140001/build/gn_helpers.py#ne... build/gn_helpers.py:20: file to the build directory.""" nit: change "the build directory" to "//build"; the former is potentially vague. https://codereview.chromium.org/1618243004/diff/140001/mojo/public/tools/gn/z... File mojo/public/tools/gn/zip.py (right): https://codereview.chromium.org/1618243004/diff/140001/mojo/public/tools/gn/z... mojo/public/tools/gn/zip.py:1: #!/usr/bin/env python Is this script actually used somewhere? Can we use the other zip.py in //build instead ?
https://codereview.chromium.org/1618243004/diff/140001/build/android/gyp/util... File build/android/gyp/util/build_utils.py (right): https://codereview.chromium.org/1618243004/diff/140001/build/android/gyp/util... build/android/gyp/util/build_utils.py:90: # this function. On 2016/01/29 21:01:56, Dirk Pranke wrote: > ick. I suppose fixing the CastShell is part of this is harder? I started doing this and got super confused. The Android scripts are all spaghetti. https://codereview.chromium.org/1618243004/diff/140001/build/config/zip.gni File build/config/zip.gni (left): https://codereview.chromium.org/1618243004/diff/140001/build/config/zip.gni#o... build/config/zip.gni:8: set_sources_assignment_filter([]) On 2016/01/29 21:01:56, Dirk Pranke wrote: > Are you sure you don't need this line? There are no "sources" in this template. https://codereview.chromium.org/1618243004/diff/140001/mojo/public/tools/gn/z... File mojo/public/tools/gn/zip.py (right): https://codereview.chromium.org/1618243004/diff/140001/mojo/public/tools/gn/z... mojo/public/tools/gn/zip.py:1: #!/usr/bin/env python You can see in a previous patch I removed this, and got errors. Then I spun off a separate CL to just do that and I didn't get it to work. The usage of this script depends on some specific handling of symbolic links that I don't understand, and I wasted too long on it so I'm leaving as-is.
dpranke@chromium.org changed reviewers: + slan@chromium.org
lgtm. Mike, Stephen, can you maybe help with my questions, below? https://codereview.chromium.org/1618243004/diff/140001/build/android/gyp/util... File build/android/gyp/util/build_utils.py (right): https://codereview.chromium.org/1618243004/diff/140001/build/android/gyp/util... build/android/gyp/util/build_utils.py:90: # this function. On 2016/01/29 21:10:21, brettw wrote: > On 2016/01/29 21:01:56, Dirk Pranke wrote: > > ick. I suppose fixing the CastShell is part of this is harder? > > I started doing this and got super confused. The Android scripts are all > spaghetti. Okay, maybe slan@ can help us get that part straightened out? https://codereview.chromium.org/1618243004/diff/140001/build/config/zip.gni File build/config/zip.gni (left): https://codereview.chromium.org/1618243004/diff/140001/build/config/zip.gni#o... build/config/zip.gni:8: set_sources_assignment_filter([]) On 2016/01/29 21:10:21, brettw wrote: > On 2016/01/29 21:01:56, Dirk Pranke wrote: > > Are you sure you don't need this line? > > There are no "sources" in this template. Good point. https://codereview.chromium.org/1618243004/diff/140001/mojo/public/tools/gn/z... File mojo/public/tools/gn/zip.py (right): https://codereview.chromium.org/1618243004/diff/140001/mojo/public/tools/gn/z... mojo/public/tools/gn/zip.py:1: #!/usr/bin/env python On 2016/01/29 21:10:21, brettw wrote: > You can see in a previous patch I removed this, and got errors. Then I spun off > a separate CL to just do that and I didn't get it to work. > > The usage of this script depends on some specific handling of symbolic links > that I don't understand, and I wasted too long on it so I'm leaving as-is. Mike, can you help us figure out if this is needed?
https://codereview.chromium.org/1618243004/diff/140001/mojo/public/tools/gn/z... File mojo/public/tools/gn/zip.py (right): https://codereview.chromium.org/1618243004/diff/140001/mojo/public/tools/gn/z... mojo/public/tools/gn/zip.py:1: #!/usr/bin/env python I filed a bug https://code.google.com/p/chromium/issues/detail?id=582594 and added a todo in this file with a reference to it.
The CQ bit was checked by brettw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/1618243004/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1618243004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1618243004/160001
rubber stamp lgtm (to at least avoid unchecking your cq bit) https://codereview.chromium.org/1618243004/diff/140001/mojo/public/tools/gn/z... File mojo/public/tools/gn/zip.py (right): https://codereview.chromium.org/1618243004/diff/140001/mojo/public/tools/gn/z... mojo/public/tools/gn/zip.py:1: #!/usr/bin/env python On 2016/01/29 21:48:15, brettw wrote: > I filed a bug https://code.google.com/p/chromium/issues/detail?id=582594 and > added a todo in this file with a reference to it. I don't have any useful offhand knowledge here, but I'll look next week.
https://codereview.chromium.org/1618243004/diff/140001/build/android/gyp/util... File build/android/gyp/util/build_utils.py (right): https://codereview.chromium.org/1618243004/diff/140001/build/android/gyp/util... build/android/gyp/util/build_utils.py:90: # this function. On 2016/01/29 21:38:42, Dirk Pranke wrote: > On 2016/01/29 21:10:21, brettw wrote: > > On 2016/01/29 21:01:56, Dirk Pranke wrote: > > > ick. I suppose fixing the CastShell is part of this is harder? > > > > I started doing this and got super confused. The Android scripts are all > > spaghetti. > > Okay, maybe slan@ can help us get that part straightened out? Sure. Brett, to which script are you referring?
On 2016/01/29 22:37:57, slan wrote: > Sure. Brett, to which script are you referring? The buildbot helpfully deleted the test output from patch 1. After this lands I'll do a patch to implement the TODO I left in build_utils and you can stare at the errors :)
On 2016/01/29 22:46:46, brettw wrote: > stare at the errors :) That's what I do best. Please keep me posted.
On 2016/01/29 22:49:42, slan wrote: > On 2016/01/29 22:46:46, brettw wrote: > > stare at the errors :) > > That's what I do best. Please keep me posted. Actually this isn't cast specific. write_ordered_libraries.py writes a file in json, and strip_library_for_device.py takes this as input to the --libraries command and attempts to parse it via ParseGypList which sees the "[" and treats it as GN formatted. I'll see if just forcing strip_library_for_device to parse it as json fixes the problem.
Message was sent while issue was closed.
Description was changed from ========== Use gn_helpers to deserialize GN lists. The Android build code doesn't properly unescape the GN lists, use the official Python GN parser instead of ast for the zip script. Document what should be done for build_utils, but don't fix it due to cast shell build errors as explained in the comment I added. Update mojo zip script to use the gn helpers. BUG=573132, 571022 ========== to ========== Use gn_helpers to deserialize GN lists. The Android build code doesn't properly unescape the GN lists, use the official Python GN parser instead of ast for the zip script. Document what should be done for build_utils, but don't fix it due to cast shell build errors as explained in the comment I added. Update mojo zip script to use the gn helpers. BUG=573132, 571022 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Use gn_helpers to deserialize GN lists. The Android build code doesn't properly unescape the GN lists, use the official Python GN parser instead of ast for the zip script. Document what should be done for build_utils, but don't fix it due to cast shell build errors as explained in the comment I added. Update mojo zip script to use the gn helpers. BUG=573132, 571022 ========== to ========== Use gn_helpers to deserialize GN lists. The Android build code doesn't properly unescape the GN lists, use the official Python GN parser instead of ast for the zip script. Document what should be done for build_utils, but don't fix it due to cast shell build errors as explained in the comment I added. Update mojo zip script to use the gn helpers. BUG=573132, 571022 Committed: https://crrev.com/81687d8362fd2dbf0acab8f1deee5b91ee1e5a49 Cr-Commit-Position: refs/heads/master@{#372461} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/81687d8362fd2dbf0acab8f1deee5b91ee1e5a49 Cr-Commit-Position: refs/heads/master@{#372461} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
