Add the mini_installer_syzygy GN target.

Add the mini_installer_syzygy GN target.

This moves most of the mini_installer GN code into a template so it can be reused to generate the mini_installer_syzygy target.

It almost works except that syzygy\mini_installer.exe (which is an archive) contain 'syzygy/chrome.packed.7z' instead of 'chrome.packed.7z', I'm not sure why (the flags passed to create_installer_archive.py are the same than with GYP).

I'm not so familiar with GN so there's probably a better/cleaner way to do this.

BUG=525752

[Sébastien Marchand, 2016-06-23 23:48:11]

Description was changed from

==========
Add the mini_installer_syzygy GN target.
==========

to

==========
Add the mini_installer_syzygy GN target.

This move most of the mini_installer GN code into a template so it can be reused
to generate the mini_installer_syzygy target.

It almost work except that syzygy\mini_installer.exe (which is an archive)
contain 'syzygy/chrome.packed.7z' instead of 'chrome.packed.7z', I'm not sure
why (the flags passed to create_installer_archive.py are the same than with
GYP).

I'm not so familiar with GN so there's probably a better/cleaner way to do this.
==========

[Sébastien Marchand, 2016-06-23 23:48:11]

sebmarchand@chromium.org changed reviewers:
+ brucedawson@chromium.org, dpranke@chromium.org, gab@chromium.org

[Sébastien Marchand, 2016-06-23 23:48:43]

sebmarchand@chromium.org changed reviewers:
+ brettw@chromium.org
- gab@chromium.org

[Sébastien Marchand, 2016-06-23 23:49:51]

Patchset #6 (id:100001) has been deleted

[Sébastien Marchand, 2016-06-23 23:49:59]

Patchset #2 (id:20001) has been deleted

[Sébastien Marchand, 2016-06-23 23:50:10]

Patchset #4 (id:80001) has been deleted

[Sébastien Marchand, 2016-06-23 23:50:16]

Patchset #3 (id:60001) has been deleted

[Sébastien Marchand, 2016-06-24 00:01:45]

Here's my prototype for the mini_installer_syzygy target. Note that I'm not
really familiar with the mini_installer code and with GN...

As mentioned in the CL description it doesn't really work yet (it creates
syzygy/mini_installer.exe) but it's not directly usable.

I'll be OOO until Monday (Friday 24th is a holiday day in Quebec)

[brucedawson, 2016-06-24 21:04:03]

Description was changed from

==========
Add the mini_installer_syzygy GN target.

This move most of the mini_installer GN code into a template so it can be reused
to generate the mini_installer_syzygy target.

It almost work except that syzygy\mini_installer.exe (which is an archive)
contain 'syzygy/chrome.packed.7z' instead of 'chrome.packed.7z', I'm not sure
why (the flags passed to create_installer_archive.py are the same than with
GYP).

I'm not so familiar with GN so there's probably a better/cleaner way to do this.
==========

to

==========
Add the mini_installer_syzygy GN target.

This moves most of the mini_installer GN code into a template so it can be
reused to generate the mini_installer_syzygy target.

It almost works except that syzygy\mini_installer.exe (which is an archive)
contain 'syzygy/chrome.packed.7z' instead of 'chrome.packed.7z', I'm not sure
why (the flags passed to create_installer_archive.py are the same than with
GYP).

I'm not so familiar with GN so there's probably a better/cleaner way to do this.
==========

[brucedawson, 2016-06-24 21:07:36]

> As mentioned in the CL description it doesn't really work yet (it creates
> syzygy/mini_installer.exe) but it's not directly usable.

Luckily I figured out this problem already, in a slightly different context.
from:
https://codereview.chromium.org/1893823002/diff/1/chrome/tools/build/win/crea...

# It is important to use abspath to create the path to the directory because
# if you use a relative path without any .. sequences then 7za.exe uses the
# entire relative path as part of the file paths in the archive. If you have
# a .. sequence or an absolute path then only the last directory is stored as
# part of the file paths in the archive, which is what we want.

So, you just need to pass an absolute path instead of a relative path and then
7za.exe will record just the file name, instead of the relative path which you
passed.

[brucedawson, 2016-06-24 21:47:43]

Description was changed from

==========
Add the mini_installer_syzygy GN target.

This moves most of the mini_installer GN code into a template so it can be
reused to generate the mini_installer_syzygy target.

It almost works except that syzygy\mini_installer.exe (which is an archive)
contain 'syzygy/chrome.packed.7z' instead of 'chrome.packed.7z', I'm not sure
why (the flags passed to create_installer_archive.py are the same than with
GYP).

I'm not so familiar with GN so there's probably a better/cleaner way to do this.
==========

to

==========
Add the mini_installer_syzygy GN target.

This moves most of the mini_installer GN code into a template so it can be
reused to generate the mini_installer_syzygy target.

It almost works except that syzygy\mini_installer.exe (which is an archive)
contain 'syzygy/chrome.packed.7z' instead of 'chrome.packed.7z', I'm not sure
why (the flags passed to create_installer_archive.py are the same than with
GYP).

I'm not so familiar with GN so there's probably a better/cleaner way to do this.

BUG=525752
==========

[brettw, 2016-06-24 23:51:44]

Overall this approach seems like the best way. I left some comments.

https://codereview.chromium.org/2085813004/diff/140001/chrome/installer/mini_...
File chrome/installer/mini_installer/BUILD.gn (right):

https://codereview.chromium.org/2085813004/diff/140001/chrome/installer/mini_...
chrome/installer/mini_installer/BUILD.gn:117: "$output_dir/chrome.dll",
This seems confusing. The out_dir is documented to be where the output goes but
here you use it as the input. Can we clarify this somehow?

Since there are only two calls and this seems very specific to this one case,
maybe we should delete custom_deps and make two inputs:
  chrome_dll_target
  chrome_dll_file
and use them for the deps and the input, respectively?

https://codereview.chromium.org/2085813004/diff/140001/chrome/tools/build/win...
File chrome/tools/build/win/syzygy/BUILD.gn (right):

https://codereview.chromium.org/2085813004/diff/140001/chrome/tools/build/win...
chrome/tools/build/win/syzygy/BUILD.gn:65: if (defined(invoker.deps)) {
You can replace both of these defined checks with:
  forward_variables_from(invoker, ["deps", "public_deps" "data_deps"])
(I added public_deps for completeness)

https://codereview.chromium.org/2085813004/diff/140001/chrome/tools/build/win...
chrome/tools/build/win/syzygy/BUILD.gn:100: #     ${binary_name} is assumed to
be in the output directory and must be
Just delete the $ and {} on this now (before it was trying to say it appended
".dll" to what you gave it).

https://codereview.chromium.org/2085813004/diff/140001/chrome/tools/build/win...
chrome/tools/build/win/syzygy/BUILD.gn:162: if (defined(invoker.deps)) {
Ditto for these three.

https://codereview.chromium.org/2085813004/diff/140001/chrome/tools/build/win...
chrome/tools/build/win/syzygy/BUILD.gn:205: copy("chrome_child_dll_syzygy_copy")
{
The copy target is defined inside the syzygy_asan target. They shouldn't be
bested (I'm surprised it works).

[brettw, 2016-06-24 23:55:30]

I'm going to LGTM this so Dirk can start testing the bot. Can you follow up with
the requested tweaks?

[Dirk Pranke, 2016-06-24 23:57:18]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2016-06-24 23:57:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-25 00:11:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-25 00:11:31]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Dirk Pranke, 2016-06-25 00:14:56]

On 2016/06/25 00:11:31, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

Will fix this in a separate CL that I'll upload, since I can't upload a patch to
this CL.

[Dirk Pranke, 2016-06-25 22:53:58]

https://codereview.chromium.org/2085813004/diff/140001/chrome/installer/mini_...
File chrome/installer/mini_installer/BUILD.gn (right):

https://codereview.chromium.org/2085813004/diff/140001/chrome/installer/mini_...
chrome/installer/mini_installer/BUILD.gn:256: if (target_cpu == "x86" &&
!is_clang && !is_win_fastlink) {
This also needs to depend on syzygy_optimize (else the files won't exist).

https://codereview.chromium.org/2085813004/diff/140001/chrome/tools/build/win...
File chrome/tools/build/win/syzygy/BUILD.gn (right):

https://codereview.chromium.org/2085813004/diff/140001/chrome/tools/build/win...
chrome/tools/build/win/syzygy/BUILD.gn:65: if (defined(invoker.deps)) {
On 2016/06/24 23:51:43, brettw wrote:
> You can replace both of these defined checks with:
>   forward_variables_from(invoker, ["deps", "public_deps" "data_deps"])
> (I added public_deps for completeness)

Acknowledged.

https://codereview.chromium.org/2085813004/diff/140001/chrome/tools/build/win...
chrome/tools/build/win/syzygy/BUILD.gn:100: #     ${binary_name} is assumed to
be in the output directory and must be
On 2016/06/24 23:51:43, brettw wrote:
> Just delete the $ and {} on this now (before it was trying to say it appended
> ".dll" to what you gave it).

Acknowledged.

https://codereview.chromium.org/2085813004/diff/140001/chrome/tools/build/win...
chrome/tools/build/win/syzygy/BUILD.gn:162: if (defined(invoker.deps)) {
On 2016/06/24 23:51:43, brettw wrote:
> Ditto for these three.

Acknowledged.

https://codereview.chromium.org/2085813004/diff/140001/chrome/tools/build/win...
chrome/tools/build/win/syzygy/BUILD.gn:205: copy("chrome_child_dll_syzygy_copy")
{
On 2016/06/24 23:51:43, brettw wrote:
> The copy target is defined inside the syzygy_asan target. They shouldn't be
> bested (I'm surprised it works).

s/bested/nested :).

Ack.

[Dirk Pranke, 2016-06-25 23:20:01]

On 2016/06/24 21:07:36, brucedawson wrote:
> > As mentioned in the CL description it doesn't really work yet (it creates
> > syzygy/mini_installer.exe) but it's not directly usable.
> 
> Luckily I figured out this problem already, in a slightly different context.
> from:
>
https://codereview.chromium.org/1893823002/diff/1/chrome/tools/build/win/crea...
> 
> # It is important to use abspath to create the path to the directory because
> # if you use a relative path without any .. sequences then 7za.exe uses the
> # entire relative path as part of the file paths in the archive. If you have
> # a .. sequence or an absolute path then only the last directory is stored as
> # part of the file paths in the archive, which is what we want.
> 
> So, you just need to pass an absolute path instead of a relative path and then
> 7za.exe will record just the file name, instead of the relative path which you
> passed.

I don't understand the problem here or the suggestion. Does something in this CL
need
to be changed to fix this? If so, what?

[brucedawson, 2016-06-27 23:37:00]

> I don't understand the problem here or the suggestion. Does something in this
CL
> need
> to be changed to fix this? If so, what?

This was in response to this sentence from the CL description:

> syzygy\mini_installer.exe (which is an archive) contain
'syzygy/chrome.packed.7z' instead of 'chrome.packed.7z'

Presumably this patch causes 7za to be passed "zyzygy/chrome.packed.7z" as a
patch to be added to the archive, which leads to this erroneous behavior. The
fix is to pass a fully qualified path to chrome.packed.7z, at which point 7za
will not add a directory as part of the path in the archive.

[Dirk Pranke, 2016-06-27 23:42:25]

On 2016/06/27 23:37:00, brucedawson wrote:
> > I don't understand the problem here or the suggestion. Does something in
this
> CL
> > need
> > to be changed to fix this? If so, what?
> 
> This was in response to this sentence from the CL description:
> 
> > syzygy\mini_installer.exe (which is an archive) contain
> 'syzygy/chrome.packed.7z' instead of 'chrome.packed.7z'
> 
> Presumably this patch causes 7za to be passed "zyzygy/chrome.packed.7z" as a
> patch to be added to the archive, which leads to this erroneous behavior. The
> fix is to pass a fully qualified path to chrome.packed.7z, at which point 7za
> will not add a directory as part of the path in the archive.

Ah, so change //chrome/tools/build/win/create_installer_archive.py:73 to
os.path.abspath(compressed_file) or something like that ... got it.

[Sébastien Marchand, 2016-06-27 23:58:47]

Yep, I have https://codereview.chromium.org/2106563002/ ready for review now

[Sébastien Marchand, 2016-06-28 14:46:10]

Closing this one as this has been committed in
https://codereview.chromium.org/2091023007