Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(962)

Issue 1616793003: Reland (2) of: Allow base to depend on allocator (Closed)

Created:
4 years, 11 months ago by Primiano Tucci (use gerrit)
Modified:
4 years, 11 months ago
CC:
chromium-reviews, wfh+watch_chromium.org, Dai Mikurube (NOT FULLTIME), vmpstr+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Reland (2) of: Allow base to depend on allocator Reason for reland: Some targets (clearkeycdmadapter, osmesa) now fell in the state where they depend indirectly on base (which causes the removal of the default clib) but via a shared_library boundary (which does not propagate the replacement allocator / shim layer code). Fixing those targets by adding a direct base dependency. This causes a breakage in the main waterfall: https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN/builds/10231 Original CL description: > A smaller, yet key, step to move > > From: a situation where mainly executables (but not really) depend on > allocator, and base needs dependencies (to tcmalloc) to be injected > from content (which violates the ODR in component buids). > > To: a situation where only base depends on allocator and the other > targets get recursively the required linked flags. > > In essence this CL is a more gradual approach to the bigger > unreviewable crrev.com/1528013002. > > How is the transition handled? > ------------------------------ > After this CL, the situation will be as follows: > From a build time perspective base will also depend on allocator. > This will not change anything substantial in static builds and introduce > yet another (temporary) ODR violation in Linux component builds. > The big change introduced by this CL is the fact that all the executable > targets that depend on base (virtualy all) will also get another > indirect dependency to allocator. > > In other words, after this CL executable targets will depend on > allocator for two reasons: > - Because they have an explicit dependency to it (the one I am going to > get rid of in the immediate future). > - Because this new transitive path I am introducing in base. > > Rationale of this approach > -------------------------- > This allows to restrict the critical changes in a smaller CL easier to > review, at the cost of the temporary double dependency on base. > The good things are: > - If something will break, this CL will be very easy to revert. > - The next cleanups will be straightforward. > - We have now smoke tests (crrev.com/1577883002) that will help us > realize if something goes wrong. > > Next steps > ---------- > In the next CLs I will: > - Remove the content -> base injection layer, and let base directly use > the tcmalloc functions it needs. > - Remove all the traces of USE_TCMALLOC outside of base. > - Start cleaning up the hundreds use_allocator conditionals in the gyp > files in a way which is easier to review and produce zero ninja diffs > (see crrev.com/1583973002 as an example) > > Ninja diffs caused by this change > --------------------------------- > ### Win, static build, GN: https://paste.ee/p/hvcRp > The missing targets (mostly tests) that previously were > not depending on allocator, now get that by virtue of the transitive > dependency. > > ### Win, static build, GYP: https://paste.ee/p/AGuKR > As above. Just GYP seems to emit the ninja files in a different, > inlined, format. > > ### linux static build, GYP: https://paste.ee/p/kmD7U > As above. Plus the new targets also get the -Wl,-u (keep symbol) > args as expected by allocator.gyp for the tcmalloc heap profiler. > > ### linux shared build, GYP: https://paste.ee/p/FHHNR > Nothing relevant. Just I moved the dependency to allocator from > base_unittests to base, and that is the only thing that reflects in the > ninja files. > > BUG=564618 TBR=wfh@chromium.org,brettw@chromium.org,kbr@chromium.org BUG=564618 Committed: https://crrev.com/e97b9201e99ad1a0c22b631762d0bb5f3fdd3d83 Cr-Commit-Position: refs/heads/master@{#370694}

Patch Set 1 : Original CL #

Patch Set 2 : adding base dependencies to missing targets #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -50 lines) Patch
M base/BUILD.gn View 3 chunks +4 lines, -1 line 0 comments Download
M base/allocator/BUILD.gn View 2 chunks +12 lines, -12 lines 0 comments Download
M base/allocator/allocator.gyp View 4 chunks +22 lines, -19 lines 0 comments Download
M base/base.gyp View 3 chunks +2 lines, -18 lines 0 comments Download
M media/cdm/ppapi/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download
M third_party/mesa/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 16 (11 generated)
Primiano Tucci (use gerrit)
The reason for TBR: I am now in the state where this CL passes the ...
4 years, 11 months ago (2016-01-21 14:31:10 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1616793003/20002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1616793003/20002
4 years, 11 months ago (2016-01-21 14:32:41 UTC) #11
commit-bot: I haz the power
Committed patchset #2 (id:20002)
4 years, 11 months ago (2016-01-21 14:38:46 UTC) #13
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/e97b9201e99ad1a0c22b631762d0bb5f3fdd3d83 Cr-Commit-Position: refs/heads/master@{#370694}
4 years, 11 months ago (2016-01-21 14:40:23 UTC) #15
Ken Russell (switch to Gerrit)
4 years, 11 months ago (2016-01-21 21:20:13 UTC) #16
Message was sent while issue was closed.
lgtm after the fact

Powered by Google App Engine
This is Rietveld 408576698