New issue
Advanced search Search tips

Issue 843470 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug



Sign in to add a comment

Remove unnecessary explicit dependency from generated build file

Project Member Reported by tikuta@chromium.org, May 16 2018

Issue description

Status: Assigned (was: Untriaged)

Comment 2 by tikuta@chromium.org, May 17 2018

Cc: dpranke@chromium.org sdefresne@chromium.org
Status: Started (was: Assigned)
I found the reason why the bot failed.

GN's create_bundle target sometimes does not make direct dependency to its deps/public_deps targets.
But recursive hard deps keeps such dependency from indirect relation until my change is landed.

In the issue 'Google Chrome Framework.framework/Google Chrome Framework' is generated in 'chrome_framework_package' and create_bandle('chrome_framework') has dependency to 'chrome_framework_package'.
https://chromium.googlesource.com/chromium/src/+/43fcb5eb66460fb63755f3a9383e4a6131afcc82/chrome/BUILD.gn#1226
https://chromium.googlesource.com/chromium/src/+/43fcb5eb66460fb63755f3a9383e4a6131afcc82/build/config/mac/rules.gni#289

But such dependency is not generated by directly from GN.
We need to consider deps/public_deps in
https://chromium.googlesource.com/chromium/src/+/43fcb5eb66460fb63755f3a9383e4a6131afcc82/tools/gn/ninja_create_bundle_target_writer.cc#54

I'll add such dependency in create_bundle target.
Project Member

Comment 3 by bugdroid1@chromium.org, May 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/75dfa0ea1bf5f385a36e4d696bb5afeb9a6637bf

commit 75dfa0ea1bf5f385a36e4d696bb5afeb9a6637bf
Author: Takuto Ikuta <tikuta@chromium.org>
Date: Sun May 20 22:32:54 2018

GN: Handle deps always in create_bundle target

Stop to use explicit dependency for indirect dependency in the CL broke
Mac builder.
https://chromium-review.googlesource.com/1041506

This CL uses deps/public_deps in create_bundle target always not to
miss the dependency.

Bug:  843470 
Change-Id: Iae66f2a3aec20f67ea2e880bb5fd33d6c2aa2f91
Reviewed-on: https://chromium-review.googlesource.com/1065591
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Takuto Ikuta <tikuta@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560203}
[modify] https://crrev.com/75dfa0ea1bf5f385a36e4d696bb5afeb9a6637bf/tools/gn/ninja_create_bundle_target_writer.cc
[modify] https://crrev.com/75dfa0ea1bf5f385a36e4d696bb5afeb9a6637bf/tools/gn/ninja_create_bundle_target_writer.h
[modify] https://crrev.com/75dfa0ea1bf5f385a36e4d696bb5afeb9a6637bf/tools/gn/ninja_create_bundle_target_writer_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, May 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ae94eab7c33428af8e5dcccf85a90975368fa54

commit 3ae94eab7c33428af8e5dcccf85a90975368fa54
Author: Takuto Ikuta <tikuta@chromium.org>
Date: Tue May 22 09:23:29 2018

Reland "GN: do not make indirect dependency to direct dependency"

This is a reland of e8a3ce9119189ad6ba5ecaa62bb0b53b07c895f4

Fix for breakage  crbug.com/843351  is below.
https://chromium-review.googlesource.com/c/chromium/src/+/1065591


Original change's description:
> GN: do not make indirect dependency to direct dependency
>
> If a target is hard dep, there is no need to have the action targets's
> recursive deps as direct dependency, because such dependency is
> transitive.
>
> This is found by pcc in gn-dev
> https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/BcrSfPQE84E
>
> With this change, generated android's toolchain.ninja size is reduced
> from 262MB to 31MB. And ninja's startup time reduced from 4.9~5.1s to
> 2.6s.
> I used args.gn same with android_n5x_swarming_rel bot.
>
> Also this patch reduced the time of `gn gen` from 9.5~10.3s to 6.8~7.2s
> on my machine.
>
> Change-Id: I0f0214d3abe74143516b263da839e98b3987fb64
> Reviewed-on: https://chromium-review.googlesource.com/1041506
> Reviewed-by: Brett Wilson <brettw@chromium.org>
> Reviewed-by: Dirk Pranke <dpranke@chromium.org>
> Commit-Queue: Takuto Ikuta <tikuta@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#557365}

Bug:  843470 
Change-Id: I4d3662613b21a1e520517d152865529aa05e813f
Reviewed-on: https://chromium-review.googlesource.com/1065690
Commit-Queue: Takuto Ikuta <tikuta@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560520}
[modify] https://crrev.com/3ae94eab7c33428af8e5dcccf85a90975368fa54/tools/gn/ninja_action_target_writer_unittest.cc
[modify] https://crrev.com/3ae94eab7c33428af8e5dcccf85a90975368fa54/tools/gn/ninja_create_bundle_target_writer_unittest.cc
[modify] https://crrev.com/3ae94eab7c33428af8e5dcccf85a90975368fa54/tools/gn/ninja_target_writer_unittest.cc
[modify] https://crrev.com/3ae94eab7c33428af8e5dcccf85a90975368fa54/tools/gn/target.cc
[modify] https://crrev.com/3ae94eab7c33428af8e5dcccf85a90975368fa54/tools/gn/target.h

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/abde0a4bd9f3bfddebe825cc25cc3bc857e3d088

commit abde0a4bd9f3bfddebe825cc25cc3bc857e3d088
Author: Andrew Grieve <agrieve@chromium.org>
Date: Tue Jun 05 00:22:21 2018

Roll buildtools 893eb86b02..6f4dae280c

  In order to roll GN 0fbf0789d9..84176d72a4 (r555198:r564007) and pick up
  the following changes:

  ce772faa73ab Remove references to exe_and_shlib_deps in src
  8492bbe2ef51 Fix GN bootstrap
  6b9b0c9cc512 Remove memory peak detection code.
  d7ed1f0a9c28 Add exe_and_shlib_deps to executable and shared_library configs
  8aca6d509422 Fix gn bootstrap
  524e60fb52e1 Fix typo in gn documentation
  37097a9654e5 Remove implementation of legacy heap profiler.
  3ae94eab7c33 Reland "GN: do not make indirect dependency to direct dependency"
  3685d34110d2 Fix gn bootstrap
  75dfa0ea1bf5 GN: Handle deps always in create_bundle target
  82a5f004e0e5 Reland "Switch to VS 2017 15.7.1 with 10.0.17134.0 SDK"
  4497a44f0d82 Fix gn CHECK()ing on not_needed() when given undefined identifiers
  7ad7e759dd35 Revert "Switch to VS 2017 15.7.1 with 10.0.17134.0 SDK"
  5a7f3c442684 Switch to VS 2017 15.7.1 with 10.0.17134.0 SDK
  fa3fcf684913 Revert "GN: do not make indirect dependency to direct dependency"
  443b70dbfef2 Fix GN doc formatting.
  e8a3ce911918 GN: do not make indirect dependency to direct dependency
  ea15ea06b514 Fully move PathService into the base namespace.
  ae62ba99a1a7 GN: Make the linker output file depend on the inputs.
  629ba73c96e4 [MessageLoop] Fix message_loop.h includes

TBR=dpranke@chromium.org

Bug:  843470 
Change-Id: I6cde57fde9d867da6ae9f7c8039abcdf545b0b1c
Reviewed-on: https://chromium-review.googlesource.com/1085699
Commit-Queue: agrieve <agrieve@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564311}
[modify] https://crrev.com/abde0a4bd9f3bfddebe825cc25cc3bc857e3d088/DEPS
[modify] https://crrev.com/abde0a4bd9f3bfddebe825cc25cc3bc857e3d088/extensions/browser/api/networking_private/BUILD.gn

Status: Fixed (was: Started)

Sign in to add a comment