Make static (preloaded) security state generation part of the build process.

Make static (preloaded) security state generation part of the build process.

This CL adds a build step to generate transport_security_state_static.h when
required.

BUG=595493

[martijnc, 2016-12-14 22:12:23]

Description was changed from

==========
Make Static (preloaded) security state generation part of the build process.

BUG=
==========

to

==========
Make Static (preloaded) security state generation part of the build process.

BUG=595493
==========

[martijnc, 2016-12-14 22:13:21]

Description was changed from

==========
Make Static (preloaded) security state generation part of the build process.

BUG=595493
==========

to

==========
Make static (preloaded) security state generation part of the build process.

BUG=595493
==========

[martijnc, 2016-12-14 22:16:04]

Description was changed from

==========
Make static (preloaded) security state generation part of the build process.

BUG=595493
==========

to

==========
Make static (preloaded) security state generation part of the build process.

This CL adds a build step to generate transport_security_state_static.h when
required.

BUG=595493
==========

[Ryan Sleevi, 2016-12-15 01:39:34]

rsleevi@chromium.org changed reviewers:
+ brettw@chromium.org, rsleevi@chromium.org

[Ryan Sleevi, 2016-12-15 01:39:35]

Brett: Could you look at something odd re: GN & deps here, to help us trace down
the weird linker issues

https://codereview.chromium.org/2574413002/diff/1/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2574413002/diff/1/net/BUILD.gn#newcode124
net/BUILD.gn:124: "//net:generate_preload_domain_security_state",
The problem here is that net/http/transport_security_state_static.h is still a
dependency of the .cc, and the .cc is compiled under the NaCl targets (
https://cs.chromium.org/chromium/src/net/http/transport_security_state.cc?rcl...
)

Option 1:
Since transport_security_state.cc is part of net_nacl_common_sources, make the
build step an unconditional dependency

Option 2:
Since compiling under NaCl results in an OS_NACL define, and since you won't be
generating the .h for non-NaCl builds, wrap the #include in:
if !defined(OS_NACL)
#include ".../transport_security_state_static.h"
#endif

(see places like net/cert/ct_known_logs.h/.cc for an example of a
shared-but-not-quite example)

Your PS#2 is effectively Option 1, and I think that's good/sane/OK for now
(there's a broader question of whether we can/should trim that for NaCl, and we
probably can, but I'm not trying to treat that in this CL)

https://codereview.chromium.org/2574413002/diff/10001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2574413002/diff/10001/net/BUILD.gn#newcode2091
net/BUILD.gn:2091: tool = "//net:domain_security_preload_generator"
Sorry, I missed this with agl@'s approval

For the BUILD.gn and related, you should considering adding brettw@ or dpranke@.
I believe we want to structure this more specifically to be

//net/tools/domain_security_preload_generator (with its own BUILD.gn) rather
than //net/BUILD.gn (but check with them), but that was their advice for the
registry controlled domain stuff.

From the error, it looks like it's propagating deps from //base(host_toolchain)
into //net(target_toolchain), and then virally on to remoting. (That is, the
glib dependencies in remoting - and the flags - are coming from //base), which
seems really weird/buggy.

[brettw, 2016-12-15 19:02:06]

I'd recommend
  gn desc --blame <build dir> "<target with problem>"
to see where the undesirable flag is coming from. To find the full target name
try
  gn ls --all-toolchains <build dir>
with grep

https://codereview.chromium.org/2574413002/diff/10001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2574413002/diff/10001/net/BUILD.gn#newcode2100
net/BUILD.gn:2100: rebase_path("http/transport_security_state_static.json",
root_build_dir),
I think it's bad for to duplicate the paths, and the .template file isn't listed
as an input so changes to that won't cause a rebuild. One way to address both of
these is:

# Inputs in order expected by the command line of the tool
inputs = [
  "http/transport_security_state_static.json",
  "http/transport_security_state_static.pins",
  "tools/domain_security_preload_generator/resources/transport_security_st
      ate_static.template"
]
outputs = ...

args = rebase_path(inputs, root_build_dir) + rebase_path(outputs,
root_build_dir)

[lgarron, 2016-12-16 02:04:00]

lgarron@chromium.org changed reviewers:
+ lgarron@chromium.org

[lgarron, 2016-12-16 04:15:54]

lgarron@chromium.org changed reviewers:
- lgarron@chromium.org

[martijnc, 2016-12-16 20:08:40]

Patchset #3 (id:20001) has been deleted

[martijnc, 2016-12-16 22:32:45]

I've dug into this a bit more and got the same result and suspect this might be
a GN issue. I've filed https://crbug.com/675224 with more details.

brettw: rsleevi suggested I discuss moving the generator to its own BUILD.gn in
net/tools/domain_security_preload_generator with you or dpranke@. This would
make it similar to how //net/base/registry_controlled_domains is done. I also
came across https://crbug.com/642165 which indicates the same intent.

Would do you think?

https://codereview.chromium.org/2574413002/diff/1/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2574413002/diff/1/net/BUILD.gn#newcode124
net/BUILD.gn:124: "//net:generate_preload_domain_security_state",
On 2016/12/15 at 01:39:35, Ryan Sleevi wrote:
> The problem here is that net/http/transport_security_state_static.h is still a
dependency of the .cc, and the .cc is compiled under the NaCl targets (
https://cs.chromium.org/chromium/src/net/http/transport_security_state.cc?rcl...
)
> 
> Option 1:
> Since transport_security_state.cc is part of net_nacl_common_sources, make the
build step an unconditional dependency
> 
> Option 2:
> Since compiling under NaCl results in an OS_NACL define, and since you won't
be generating the .h for non-NaCl builds, wrap the #include in:
> if !defined(OS_NACL)
> #include ".../transport_security_state_static.h"
> #endif
> 
> (see places like net/cert/ct_known_logs.h/.cc for an example of a
shared-but-not-quite example)
> 
> Your PS#2 is effectively Option 1, and I think that's good/sane/OK for now
(there's a broader question of whether we can/should trim that for NaCl, and we
probably can, but I'm not trying to treat that in this CL)

Thanks for your feedback! I'll go for Option 1 and try to fix the linker issues.

https://codereview.chromium.org/2574413002/diff/10001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2574413002/diff/10001/net/BUILD.gn#newcode2100
net/BUILD.gn:2100: rebase_path("http/transport_security_state_static.json",
root_build_dir),
On 2016/12/15 at 19:02:05, brettw (ping after 24h) wrote:
> I think it's bad for to duplicate the paths, and the .template file isn't
listed as an input so changes to that won't cause a rebuild. One way to address
both of these is:
> 
> # Inputs in order expected by the command line of the tool
> inputs = [
>   "http/transport_security_state_static.json",
>   "http/transport_security_state_static.pins",
>   "tools/domain_security_preload_generator/resources/transport_security_st
>       ate_static.template"
> ]
> outputs = ...
> 
> args = rebase_path(inputs, root_build_dir) + rebase_path(outputs,
root_build_dir)

Done.

[martijnc, 2017-01-05 17:19:58]

Patchset #6 (id:60001) has been deleted

[martijnc, 2017-01-13 18:39:13]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 18:39:46]

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

[martijnc, 2017-01-13 20:03:40]

The bots seem happier now.

rsleevi: can you review this as a net owner?
brettw: can you take a look and see if you have comments on the restructuring of
the BUILD.gn files (I moved the GN bits for the generator into it's own BUILD.gn
file as rsleevi suggested earlier)?

PS7 also includes the removal of net/http/transport_security_state_static.h but
that wasn't uploaded because the diff is too large.

[Ryan Sleevi, 2017-01-13 20:13:32]

I'm not sure if we want to make it fully automatic until the tests land, but I
could be convinced that this is acceptable.

Right now, with it being a manual process, my expectation is that people will be
manually reviewing changes produced by the generator. This, however, may be
naive and unrealistic - which is why I indicated I could be convinced otherwise.
However, by moving it fully automatic, without tests for the generator in place
yet, it's possible that incorrect code will be generated and no one will review.

But I'll give an LGTM with the hope that Brett would give his take (on whether
to require tests for the generator first before automating) and we go with that.

[commit-bot: I haz the power, 2017-01-13 20:56:03]

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

[commit-bot: I haz the power, 2017-01-13 20:56:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[martijnc, 2017-03-18 20:49:41]

Patchset #8 (id:90001) has been deleted

[martijnc, 2017-03-18 20:52:30]

Patchset #7 (id:80001) has been deleted

[martijnc, 2017-03-18 20:58:39]

Patchset #7 (id:100001) has been deleted

[martijnc, 2017-03-19 14:25:10]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-03-19 14:25:17]

Dry run: 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, 2017-03-19 14:28:11]

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

[commit-bot: I haz the power, 2017-03-19 14:28:13]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[martijnc, 2017-03-19 14:41:04]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-03-19 14:41:10]

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

[martijnc, 2017-03-19 14:41:15]

Patchset #7 (id:110001) has been deleted

[commit-bot: I haz the power, 2017-03-19 15:42:43]

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

[commit-bot: I haz the power, 2017-03-19 15:42:45]

Dry run: This issue passed the CQ dry run.

[martijnc, 2017-03-21 18:58:06]

Patchset #7 (id:120001) has been deleted

[martijnc, 2017-03-21 20:41:49]

Patchset #7 (id:130001) has been deleted

[martijnc, 2017-03-21 20:42:31]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 20:43:19]

Dry run: 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, 2017-03-21 21:40:17]

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

[commit-bot: I haz the power, 2017-03-21 21:40:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[martijnc, 2017-04-06 18:10:45]

Patchset #4 (id:40001) has been deleted

[martijnc, 2017-04-06 18:10:50]

Patchset #4 (id:50001) has been deleted

[martijnc, 2017-04-06 18:10:56]

Patchset #4 (id:70001) has been deleted

[martijnc, 2017-04-06 18:11:00]

Patchset #4 (id:150001) has been deleted

[martijnc, 2017-04-06 18:12:32]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 18:13:43]

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

[martijnc, 2017-04-06 18:24:30]

martijn@martijnc.be changed reviewers:
- brettw@chromium.org

[martijnc, 2017-04-06 18:24:31]

rsleevi: are you OK with this landing now that the tests have all landed?

[commit-bot: I haz the power, 2017-04-06 19:21:17]

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

[commit-bot: I haz the power, 2017-04-06 19:21:20]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2017-04-07 16:10:05]

lgtm

[martijnc, 2017-04-07 16:45:26]

martijn@martijnc.be changed reviewers:
+ lgarron@chromium.org

[martijnc, 2017-04-07 16:45:27]

lgarron: can you land this? This change includes the removal of
transport_security_state_static.h but that diff is to large to upload and I'm
not a committer so I can't land this manually.

PS4 is the latest (rebased) version.

Thanks!

[chromium-reviews, 2017-04-07 17:01:39]

Could you try `git cl upload --gerrit`?
On Fri, Apr 7, 2017 at 09:45 martijn@martijnc.be via codereview.chromium.org
<reply@chromiumcodereview-hr.appspotmail.com> wrote:

> lgarron: can you land this? This change includes the removal of
> transport_security_state_static.h but that diff is to large to upload and
> I'm
> not a committer so I can't land this manually.
>
> PS4 is the latest (rebased) version.
>
> Thanks!
>
> https://codereview.chromium.org/2574413002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[martijnc, 2017-04-07 17:25:40]

On 2017/04/07 at 17:01:39, chromium-reviews wrote:
> Could you try `git cl upload --gerrit`?

Yes, that worked.

Uploaded to Gerrit: https://chromium-review.googlesource.com/c/471529/.

[martijnc, 2017-04-07 18:12:53]

The CQ bit was checked by martijn@martijnc.be to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 18:13:31]

Dry run: 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, 2017-04-07 19:23:31]

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

[commit-bot: I haz the power, 2017-04-07 19:23:32]

Dry run: This issue passed the CQ dry run.