Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1103793002:
Add support for escaped target names in isolate driver. (Closed)
DescriptionAdd support for escaped target names in isolate driver.
Currently the isolate_driver.py which creates the dependency files
used by the isolate system, does a simple split on all spaces when
trying to identify targets.
This can fail if the target name contains a space in the name. In
ninja, spaces are escaped with a $-prefix. An example would be
'Content$ Shell$ Helper.app'.
This CL adds support for such target names and ensures that they
stay as one item. Doing this uncovered a few missing dependencies
and a missing file in some .isolate-files for the component build
on Mac.
Original CL: https://codereview.chromium.org/970203003
BUG=462248
Committed: https://crrev.com/86c49590cbae332dc933cd9ae3f67d240fb1f125
Cr-Commit-Position: refs/heads/master@{#328236}
Patch Set 1 #Patch Set 2 : Added chrome_dll dependencies to isolated tests that require it. #Patch Set 3 : Use chrome_main_dll and only on mac #Patch Set 4 : Only add libchrome_main_dll.dylib for component build #Patch Set 5 : Fix sync and ui targets as well #
Total comments: 4
Patch Set 6 : Simplify comment #Patch Set 7 : Move isolate-update to chrome.isolate, and add chrome-dep to sync tests #Patch Set 8 : Remove unnecessary dep for browser_tests_run #
Messages
Total messages: 40 (16 generated)
The CQ bit was checked by nyquist@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/1103793002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nyquist@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/1103793002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nyquist@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/1103793002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nyquist@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/1103793002/60001
The CQ bit was unchecked by nyquist@chromium.org
The CQ bit was checked by nyquist@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/1103793002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nyquist@chromium.org changed reviewers: + thakis@chromium.org, vadimsh@chromium.org
vadimsh: tools/ thakis: chrome/ avi: FYI
tools/ lgtm
rs-lgtm (maruel: do dylib dependencies have to be mentioned in isolate files? I thought those came from the gyp files already?)
maruel@chromium.org changed reviewers: + maruel@chromium.org
On 2015/05/03 20:31:56, Nico wrote: > rs-lgtm > > (maruel: do dylib dependencies have to be mentioned in isolate files? I thought > those came from the gyp files already?) If you look at post_process_deps(), it does try to look at .dylib and inject these but this is still only done in component=shared_library. I recall wanting to enable it all the time around last summer but I can't recall why I haven't done it yet (!) https://codereview.chromium.org/1103793002/diff/80001/tools/isolate_driver.py File tools/isolate_driver.py (right): https://codereview.chromium.org/1103793002/diff/80001/tools/isolate_driver.py... tools/isolate_driver.py:146: # Use a negative lookbehind to not split on a space that is following a $. .. to not split on '$ '. would be easier to read but I don't mind.
nyquist: In that case, the .isolate changes and most of the .gyp changes probably aren't needed? https://codereview.chromium.org/1103793002/diff/80001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/1103793002/diff/80001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:3070: 'chrome_main_dll', This should be unnecessary because chrome should depend on that, and this depends on chrome https://codereview.chromium.org/1103793002/diff/80001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:3089: 'chrome_main_dll', likewise
thakis: agreed. However, patch set 1 shows this change without any other changes than the python file. browser_tests, interactive_ui_tests and sync_integration_tests all fail with lines like: dyld: Library not loaded: @rpath/libchrome_main_dll.dylib Referenced from: /Volumes/data/b/swarm_slave/work/run_tha_testERSrKZ/out/Debug/Chromium.app/Contents/Versions/44.0.2382.0/Chromium Framework.framework/Chromium Framework Reason: image not found See http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_dbg_... for the build log. I could try again to only do the python change, but I would expect it to fail the same way again. Is there any other target I should depend on instead or some other way the dependencies should be fixed instead (if they are in fact wrong)? https://codereview.chromium.org/1103793002/diff/80001/tools/isolate_driver.py File tools/isolate_driver.py (right): https://codereview.chromium.org/1103793002/diff/80001/tools/isolate_driver.py... tools/isolate_driver.py:146: # Use a negative lookbehind to not split on a space that is following a $. On 2015/05/04 13:39:52, M-A Ruel wrote: > .. to not split on '$ '. > would be easier to read but I don't mind. Done.
Hm, should it be listed in chrome/chrome.isolate at least then?
The CQ bit was checked by nyquist@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1103793002/#ps140001 (title: "Remove unnecessary dep for browser_tests_run")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1103793002/140001
thakis: I asked maruel, and he also did not know why your suggestion works. So now I'm unsure what to do. Should I add the inclusion of chrome.isolate to those test-isolate files for good measure? Or submit the change as is?
Try landing as-is (at patch set 8). If that fixes all known problems, then we're happy I think.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nyquist@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1103793002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/86c49590cbae332dc933cd9ae3f67d240fb1f125 Cr-Commit-Position: refs/heads/master@{#328236}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1129493003/ by nyquist@chromium.org. The reason for reverting is: Broke interactive_ui_tests and sync_integration_tests on Mac. See first breakage: http://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29... Possibly need to include the chrome.isolate in the isolates for browser_tests, and the two test suites mentioned above.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
