DevTools: Refactor build script to copy module files in debug_devtools mode

DevTools: Refactor build script to copy module files in debug_devtools mode

Currently, all module files are copied by the 'copies' GYP actions.
This patch makes build_applications.py copy the app HTML and directories
into the output directory.

R=dgozman, pfeldman@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183664

[loislo, 2014-10-13 11:50:11]

On 2014/10/13 11:33:43, apavlov wrote:

Does it fixes the problem with debug_devtools=1 on windows?
It was a build error because we hit command line length limit.

[apavlov, 2014-10-13 12:10:10]

On 2014/10/13 11:50:11, loislo wrote:
> On 2014/10/13 11:33:43, apavlov wrote:
> 
> Does it fixes the problem with debug_devtools=1 on windows?
> It was a build error because we hit command line length limit.

That one is totally unrelated.

[dgozman, 2014-10-13 13:01:52]

https://codereview.chromium.org/646413002/diff/1/Source/devtools/devtools.gyp
File Source/devtools/devtools.gyp (right):

https://codereview.chromium.org/646413002/diff/1/Source/devtools/devtools.gyp...
Source/devtools/devtools.gyp:287: # Copy Runtime.js and non-module directories
here.
"Copy runtime core and ..."

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
File Source/devtools/scripts/concatenate_application_code.py (right):

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:57: src_name =
path.join(src, name)
src_name = path.join(root, name)

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:58: dest_name =
path.join(dest, name)
dest_name = path.join(dest, path.relpath(root, src))

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:64:
hardlink_or_copy_dir(path.join(src, name), path.join(dest, name))
os.walk is already recursive.

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:67: class AppBuilder:
You should probably have |build_app| function defined in this base class?

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:76: #   app_name.html
Maybe <app_name>.html and <app_name>.js ?

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:94: with
open(path.join(self.application_dir, html_name), 'r') as app_input_html:
This whole processing tells us that we should generate html from template, and
not have multiple *.html files.

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:199: #   app_name.html
as-is
<app_name>.html ?

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:220:
write_file(path.join(self.output_dir, html_name), output.getvalue())
Why not shutil.copy?

[apavlov, 2014-10-13 15:02:21]

https://codereview.chromium.org/646413002/diff/1/Source/devtools/devtools.gyp
File Source/devtools/devtools.gyp (right):

https://codereview.chromium.org/646413002/diff/1/Source/devtools/devtools.gyp...
Source/devtools/devtools.gyp:287: # Copy Runtime.js and non-module directories
here.
On 2014/10/13 13:01:52, dgozman wrote:
> "Copy runtime core and ..."

Done.

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
File Source/devtools/scripts/concatenate_application_code.py (right):

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:57: src_name =
path.join(src, name)
On 2014/10/13 13:01:52, dgozman wrote:
> src_name = path.join(root, name)

Done.

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:58: dest_name =
path.join(dest, name)
On 2014/10/13 13:01:52, dgozman wrote:
> dest_name = path.join(dest, path.relpath(root, src))

dest_name = path.join(dest, path.join(path.relpath(src, root), name))
actually, since we have to specify the target name, not the directory name

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:64:
hardlink_or_copy_dir(path.join(src, name), path.join(dest, name))
On 2014/10/13 13:01:52, dgozman wrote:
> os.walk is already recursive.

Acknowledged.

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:67: class AppBuilder:
On 2014/10/13 13:01:52, dgozman wrote:
> You should probably have |build_app| function defined in this base class?

We'd like to get a clear error thrown in case build_app() is not defined in the
concrete subclass, and I don't feel like mangling around with the ugly ABC
support...

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:76: #   app_name.html
On 2014/10/13 13:01:52, dgozman wrote:
> Maybe <app_name>.html and <app_name>.js ?

Ugh, it's a bit outdated patch... I've got this locally

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:94: with
open(path.join(self.application_dir, html_name), 'r') as app_input_html:
On 2014/10/13 13:01:52, dgozman wrote:
> This whole processing tells us that we should generate html from template, and
> not have multiple *.html files.

This is just code moved from generate_devtools_html.py (sorry, forgot to delete
the file)

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:199: #   app_name.html
as-is
On 2014/10/13 13:01:52, dgozman wrote:
> <app_name>.html ?

Done.

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:220:
write_file(path.join(self.output_dir, html_name), output.getvalue())
On 2014/10/13 13:01:52, dgozman wrote:
> Why not shutil.copy?

I will add something else into here in a subsequent patch, so this is to set up
the required methods in advance.

[dgozman, 2014-10-14 09:21:08]

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
File Source/devtools/scripts/concatenate_application_code.py (right):

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:94: with
open(path.join(self.application_dir, html_name), 'r') as app_input_html:
On 2014/10/13 15:02:21, apavlov wrote:
> On 2014/10/13 13:01:52, dgozman wrote:
> > This whole processing tells us that we should generate html from template,
and
> > not have multiple *.html files.
> 
> This is just code moved from generate_devtools_html.py (sorry, forgot to
delete
> the file)

I see, but my point about generating is still valid.

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:220:
write_file(path.join(self.output_dir, html_name), output.getvalue())
On 2014/10/13 15:02:21, apavlov wrote:
> On 2014/10/13 13:01:52, dgozman wrote:
> > Why not shutil.copy?
> 
> I will add something else into here in a subsequent patch, so this is to set
up
> the required methods in advance.

Maybe change shutil.copy to this processing in the follow-up patch itself?

https://codereview.chromium.org/646413002/diff/110001/Source/devtools/BUILD.gn
File Source/devtools/BUILD.gn (right):

https://codereview.chromium.org/646413002/diff/110001/Source/devtools/BUILD.g...
Source/devtools/BUILD.gn:17: "<@(devtools_standalone_files)",
These hacks are nasty. Could you fix this in gypi in a separate patch please?

https://codereview.chromium.org/646413002/diff/110001/Source/devtools/BUILD.g...
Source/devtools/BUILD.gn:278: group("copy_debug_modules") {
copy_debug_non_modules

https://codereview.chromium.org/646413002/diff/110001/Source/devtools/scripts...
File Source/devtools/scripts/concatenate_application_code.py (right):

https://codereview.chromium.org/646413002/diff/110001/Source/devtools/scripts...
Source/devtools/scripts/concatenate_application_code.py:58: dest_name =
path.join(dest, path.join(path.relpath(src, root), name))
Shouldn't this be |relpath(root, src)| ?

https://codereview.chromium.org/646413002/diff/110001/Source/devtools/scripts...
Source/devtools/scripts/concatenate_application_code.py:62:
shutil.copy(src_name, dest_name)
Will this create necessary directories? I think you are missing creation of
subdirectories in this processing.

https://codereview.chromium.org/646413002/diff/110001/Source/devtools/scripts...
Source/devtools/scripts/concatenate_application_code.py:111:
write_file(path.join(self.output_dir, script_name), output.getvalue())
Are you missing minify_js call here?

[apavlov, 2014-10-14 10:02:59]

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
File Source/devtools/scripts/concatenate_application_code.py (right):

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:94: with
open(path.join(self.application_dir, html_name), 'r') as app_input_html:
On 2014/10/14 09:21:08, dgozman wrote:
> On 2014/10/13 15:02:21, apavlov wrote:
> > On 2014/10/13 13:01:52, dgozman wrote:
> > > This whole processing tells us that we should generate html from template,
> and
> > > not have multiple *.html files.
> > 
> > This is just code moved from generate_devtools_html.py (sorry, forgot to
> delete
> > the file)
> 
> I see, but my point about generating is still valid.

I'm not questioning this, I'm just saying that we should confine the scope of
this patch to simple code motion and rewriting the multiple 'copies' rules in
Python.

https://codereview.chromium.org/646413002/diff/1/Source/devtools/scripts/conc...
Source/devtools/scripts/concatenate_application_code.py:220:
write_file(path.join(self.output_dir, html_name), output.getvalue())
On 2014/10/14 09:21:08, dgozman wrote:
> On 2014/10/13 15:02:21, apavlov wrote:
> > On 2014/10/13 13:01:52, dgozman wrote:
> > > Why not shutil.copy?
> > 
> > I will add something else into here in a subsequent patch, so this is to set
> up
> > the required methods in advance.
> 
> Maybe change shutil.copy to this processing in the follow-up patch itself?

Done, even though exactly this code used to run when building debug_devtools
HTML, and I was aiming for a plain refactoring in this part.

https://codereview.chromium.org/646413002/diff/110001/Source/devtools/BUILD.gn
File Source/devtools/BUILD.gn (right):

https://codereview.chromium.org/646413002/diff/110001/Source/devtools/BUILD.g...
Source/devtools/BUILD.gn:17: "<@(devtools_standalone_files)",
On 2014/10/14 09:21:08, dgozman wrote:
> These hacks are nasty. Could you fix this in gypi in a separate patch please?

Acknowledged.

https://codereview.chromium.org/646413002/diff/110001/Source/devtools/BUILD.g...
Source/devtools/BUILD.gn:278: group("copy_debug_modules") {
On 2014/10/14 09:21:08, dgozman wrote:
> copy_debug_non_modules

Done.

https://codereview.chromium.org/646413002/diff/110001/Source/devtools/scripts...
File Source/devtools/scripts/concatenate_application_code.py (right):

https://codereview.chromium.org/646413002/diff/110001/Source/devtools/scripts...
Source/devtools/scripts/concatenate_application_code.py:58: dest_name =
path.join(dest, path.join(path.relpath(src, root), name))
On 2014/10/14 09:21:08, dgozman wrote:
> Shouldn't this be |relpath(root, src)| ?

Yes, refactored the entire snippet and tested on nested directories.

https://codereview.chromium.org/646413002/diff/110001/Source/devtools/scripts...
Source/devtools/scripts/concatenate_application_code.py:62:
shutil.copy(src_name, dest_name)
On 2014/10/14 09:21:08, dgozman wrote:
> Will this create necessary directories? I think you are missing creation of
> subdirectories in this processing.

Ah, right, this was supposed to be handled by the original recursive approach. I
wanted to make the function universal, even though we never have subdirectories
more than one level deep. Thanks.

https://codereview.chromium.org/646413002/diff/110001/Source/devtools/scripts...
Source/devtools/scripts/concatenate_application_code.py:111:
write_file(path.join(self.output_dir, script_name), output.getvalue())
On 2014/10/14 09:21:08, dgozman wrote:
> Are you missing minify_js call here?

Done.

[apavlov, 2014-10-14 10:04:05]

Patchset #3 (id:180001) has been deleted

[dgozman, 2014-10-14 10:17:49]

lgtm

https://codereview.chromium.org/646413002/diff/200001/Source/devtools/scripts...
File Source/devtools/scripts/concatenate_application_code.py (right):

https://codereview.chromium.org/646413002/diff/200001/Source/devtools/scripts...
Source/devtools/scripts/concatenate_application_code.py:20: from os.path import
join
Personally, I think that "join" is a too generic name to import it separately.

https://codereview.chromium.org/646413002/diff/200001/Source/devtools/scripts...
Source/devtools/scripts/concatenate_application_code.py:60: src_name =
join(root, name)
nit: renaming |root| to |src_dir| will make this code more readable.

[apavlov, 2014-10-14 10:29:29]

https://codereview.chromium.org/646413002/diff/200001/Source/devtools/scripts...
File Source/devtools/scripts/concatenate_application_code.py (right):

https://codereview.chromium.org/646413002/diff/200001/Source/devtools/scripts...
Source/devtools/scripts/concatenate_application_code.py:20: from os.path import
join
On 2014/10/14 10:17:48, dgozman wrote:
> Personally, I think that "join" is a too generic name to import it separately.

I had thought so, too, until I saw
https://code.google.com/p/chromium/codesearch#search/&q=%22from%20os.path%20i...
(15 matches), which, amazingly, is more that a similar query for "from os import
path" (14 matches for me)

https://codereview.chromium.org/646413002/diff/200001/Source/devtools/scripts...
Source/devtools/scripts/concatenate_application_code.py:60: src_name =
join(root, name)
On 2014/10/14 10:17:48, dgozman wrote:
> nit: renaming |root| to |src_dir| will make this code more readable.

good point!

[apavlov, 2014-10-14 10:29:33]

The CQ bit was checked by apavlov@chromium.org

[commit-bot: I haz the power, 2014-10-14 10:30:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/646413002/220001

[commit-bot: I haz the power, 2014-10-14 11:28:51]

Committed patchset #4 (id:220001) as 183664