Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
228463003:
Make isolate_driver.py process build.ninja and extract dependencies. (Closed)
|
Created:
6 years, 8 months ago by M-A Ruel Modified:
5 years, 11 months ago CC:
chromium-reviews, Ken Russell (switch to Gerrit) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake isolate_driver.py process build.ninja and extract dependencies.
This uses a few assumption:
- This basically breaks non-ninja build for component builds. This never worked
anyway.
- This assumes the file format of .ninja files. This will likely be quite
obvious when this breaks.
- It makes some assumptions about the build steps, for example '.so.TOC' ->
'.so'.
On the other hand, it creates a deterministic dependency tree, which is awesome.
Technically it would work as well for non-component builds but I don't want to
go this far yet. But in the end, that's the goal that nobody has to enter the
binary dependencies in the .isolate files.
R=csharp@chromium.org
BUG=360223, 333473
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263058
Patch Set 1 #Patch Set 2 : better doc #Patch Set 3 : Now works on OSX and Windows #Patch Set 4 : Lower verbosity #
Total comments: 16
Patch Set 5 : address review comments #
Total comments: 4
Patch Set 6 : better log message #Patch Set 7 : Fix exception #Patch Set 8 : Need to replace \ with / #Patch Set 9 : Testing to figure out what is blowing up on win_x64_rel #Patch Set 10 : Add rules_seen #Patch Set 11 : Remove logging message now that it works #Messages
Total messages: 32 (1 generated)
Here's the base_unittests.isolate files generated when building
base_unittests_run in component build on each OS:
On linux:
# Warning: this file was AUTOGENERATED.
# DO NO EDIT.
{
'variables': {
'isolate_dependency_tracked': [
'<(PRODUCT_DIR)/lib/libbase.so',
'<(PRODUCT_DIR)/lib/libbase_i18n.so',
'<(PRODUCT_DIR)/lib/libbase_prefs.so',
'<(PRODUCT_DIR)/lib/libicui18n.so',
'<(PRODUCT_DIR)/lib/libicuuc.so',
'<(PRODUCT_DIR)/lib/libicuuc.so',
'<(PRODUCT_DIR)/xdisplaycheck',
],
},
'includes': [
'../../../../base/base_unittests.isolate',
],
}
On OSX:
# Warning: this file was AUTOGENERATED.
# DO NO EDIT.
{
'variables': {
'isolate_dependency_tracked': [
'<(PRODUCT_DIR)/libbase.dylib',
'<(PRODUCT_DIR)/libbase_i18n.dylib',
'<(PRODUCT_DIR)/libbase_prefs.dylib',
'<(PRODUCT_DIR)/libicui18n.dylib',
'<(PRODUCT_DIR)/libicuuc.dylib',
'<(PRODUCT_DIR)/libicuuc.dylib',
],
},
'includes': [
'../../../../base/base_unittests.isolate',
],
}
On Windows:
DEBUG:root:# Warning: this file was AUTOGENERATED.
# DO NO EDIT.
{
'variables': {
'isolate_dependency_tracked': [
'<(PRODUCT_DIR)/base.dll',
'<(PRODUCT_DIR)/base.dll',
'<(PRODUCT_DIR)/base_unittests.exe',
'<(PRODUCT_DIR)/icui18n.dll',
'<(PRODUCT_DIR)/icuuc.dll',
'<(PRODUCT_DIR)/icuuc.dll',
],
},
'includes': [
'../../../../base/base_unittests.isolate',
],
}
Awesome!
https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py File tools/isolate_driver.py (right): https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:41: the <200ms range for a complete chromium tree. As such the code is layed out layed->laid https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:54: if line[-1:] == '$': drop :? (since line[-1:] == line[-1], right?) https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:65: # Save the dependency list as a raw string. Only the lines needed will What do you mean" only the lines needed will be processes"? Where is the logic that determines that? https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:67: build_target, line = line[6:].split(': ', 1) Can you change this to "build_target, dependencies" here. Line 74 really confused me because I didn't realize line had changed value within this if statement https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:84: # TODO(maruel): Skip the files known to not be needed. It's save an aweful "it's save" -> "It saves" https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:139: logging.warning('MIA: %s', target) Why use MIA here, I'm not sure I understand how this could be missing https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:198: # It's a big assumption here that the name of the isolate file matches the Can we assert this somewhere in the code? https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:223: isolate_format.print_all(comment, isolate_dict, out) why don't we write directly to the file anymore?
https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py File tools/isolate_driver.py (right): https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:41: the <200ms range for a complete chromium tree. As such the code is layed out On 2014/04/08 18:05:37, csharp wrote: > layed->laid Done. https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:54: if line[-1:] == '$': On 2014/04/08 18:05:37, csharp wrote: > drop :? (since line[-1:] == line[-1], right?) Done. While editing I hadn't done line 51-52 so this was throwing but it's fine now. https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:65: # Save the dependency list as a raw string. Only the lines needed will On 2014/04/08 18:05:37, csharp wrote: > What do you mean" only the lines needed will be processes"? Where is the logic > that determines that? Changed to: Save the dependency list as a raw string. Only the lines needed will be processed with raw_build_to_deps(). https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:67: build_target, line = line[6:].split(': ', 1) On 2014/04/08 18:05:37, csharp wrote: > Can you change this to "build_target, dependencies" here. Line 74 really > confused me because I didn't realize line had changed value within this if > statement Done. https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:84: # TODO(maruel): Skip the files known to not be needed. It's save an aweful On 2014/04/08 18:05:37, csharp wrote: > "it's save" -> "It saves" Done. https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:139: logging.warning('MIA: %s', target) On 2014/04/08 18:05:37, csharp wrote: > Why use MIA here, I'm not sure I understand how this could be missing It used to happen in the code as I was making progress but not anymore so it should be fine and removed it. Will add back if necessary. https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:198: # It's a big assumption here that the name of the isolate file matches the On 2014/04/08 18:05:37, csharp wrote: > Can we assert this somewhere in the code? It's really an assumption, there's no way to assert that it's what is going to be executed. https://codereview.chromium.org/228463003/diff/60001/tools/isolate_driver.py#... tools/isolate_driver.py:223: isolate_format.print_all(comment, isolate_dict, out) On 2014/04/08 18:05:37, csharp wrote: > why don't we write directly to the file anymore? So it's content can be logged at line 228, which is super useful for debugging.
https://codereview.chromium.org/228463003/diff/80001/tools/isolate_driver.py File tools/isolate_driver.py (right): https://codereview.chromium.org/228463003/diff/80001/tools/isolate_driver.py#... tools/isolate_driver.py:149: logging.info('MIA: %s', dependency) Is this still possible?
https://codereview.chromium.org/228463003/diff/80001/tools/isolate_driver.py File tools/isolate_driver.py (right): https://codereview.chromium.org/228463003/diff/80001/tools/isolate_driver.py#... tools/isolate_driver.py:149: logging.info('MIA: %s', dependency) On 2014/04/08 20:04:57, csharp wrote: > Is this still possible? I didn't see it again but it's likely possible. There's many configurations so I can't test them all sadly.
lgtm with logging nit https://codereview.chromium.org/228463003/diff/80001/tools/isolate_driver.py File tools/isolate_driver.py (right): https://codereview.chromium.org/228463003/diff/80001/tools/isolate_driver.py#... tools/isolate_driver.py:149: logging.info('MIA: %s', dependency) On 2014/04/08 20:12:43, M-A Ruel wrote: > On 2014/04/08 20:04:57, csharp wrote: > > Is this still possible? > > I didn't see it again but it's likely possible. There's many configurations so I > can't test them all sadly. Is there a more useful message we can provide explaining why this could happen and why is it ok?
https://codereview.chromium.org/228463003/diff/80001/tools/isolate_driver.py File tools/isolate_driver.py (right): https://codereview.chromium.org/228463003/diff/80001/tools/isolate_driver.py#... tools/isolate_driver.py:149: logging.info('MIA: %s', dependency) On 2014/04/08 21:02:25, csharp wrote: > On 2014/04/08 20:12:43, M-A Ruel wrote: > > On 2014/04/08 20:04:57, csharp wrote: > > > Is this still possible? > > > > I didn't see it again but it's likely possible. There's many configurations so > I > > can't test them all sadly. > > Is there a more useful message we can provide explaining why this could happen > and why is it ok? Changed to logging.info('Failed to find a build step to generate: %s', dependency) I'll see what happens in practice and will act accordingly.
The CQ bit was checked by maruel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maruel@chromium.org/228463003/100001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maruel@chromium.org/228463003/100001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maruel@chromium.org/228463003/100001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maruel@chromium.org/228463003/100001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maruel@chromium.org/228463003/100001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maruel@chromium.org/228463003/100001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maruel@chromium.org/228463003/100001
On 2014/04/08 23:31:47, M-A Ruel wrote: > I'll see what happens in practice and will act accordingly. As a matter of fact, it try/except is needed. I could do a lookup but I think it's faster to do the try/except instead, I haven't benchmarked. Otherwise it fails for items that do not have a build step defined, for example: http://build.chromium.org/p/tryserver.chromium/builders/win_x64_rel/builds/92...
The CQ bit was checked by maruel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maruel@chromium.org/228463003/110002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maruel@chromium.org/228463003/110002
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_x64_rel for step(s) base_unittests, chrome_elf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
The CQ bit was checked by maruel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maruel@chromium.org/228463003/130001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_x64_rel for step(s) base_unittests, chrome_elf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
The CQ bit was checked by maruel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maruel@chromium.org/228463003/190001
Message was sent while issue was closed.
Change committed as 263058
Message was sent while issue was closed.
On 2014/04/10 20:04:19, I haz the power (commit-bot) wrote: > Change committed as 263058 FYI: Reverted r263072
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
uh, this looks very sketchy to me. why not recursively look at the output of ldd / otool -L / whatever the windows thing is? That should be way faster, looks at the actual source of truth, and doesn't require a hacked-up .ninja parser. |
