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

Issue 1606393002: Revert of Reland 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
Reviewers:
Nico
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

Revert of Reland of: Allow base to depend on allocator (patchset #2 id:20001 of https://codereview.chromium.org/1605023004/ ) Reason for revert: The Win bot is also broken. I give up for today. https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN/builds/10231/steps/compile/logs/stdio Original issue's description: > Reland of: Allow base to depend on allocator > > Reason for reland: > Fix empty gyp target edge case, that caused the breakage. > > Original CL: http://crrev.com/1584893002 > Revert CL: http://crrev.com/1610673002 > Breakage: https://build.chromium.org/p/chromium.mac/builders/iOS_Device/builds/35574 > > 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 > BUG=564618 > > Committed: https://crrev.com/717238e9faac7bdd5a7a63fffdcee77e93d48e1f > Cr-Commit-Position: refs/heads/master@{#370438} TBR=thakis@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=564618 Committed: https://crrev.com/d1cb127a27a232f4406e03e7a482ba6ee26c4492 Cr-Commit-Position: refs/heads/master@{#370441}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+51 lines, -41 lines) Patch
M base/BUILD.gn View 3 chunks +1 line, -4 lines 0 comments Download
M base/allocator/BUILD.gn View 2 chunks +13 lines, -13 lines 0 comments Download
M base/allocator/allocator.gyp View 3 chunks +19 lines, -22 lines 0 comments Download
M base/base.gyp View 3 chunks +18 lines, -2 lines 0 comments Download

Messages

Total messages: 7 (3 generated)
Primiano Tucci (use gerrit)
Created Revert of Reland of: Allow base to depend on allocator
4 years, 11 months ago (2016-01-20 19:38:12 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1606393002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1606393002/1
4 years, 11 months ago (2016-01-20 19:41:22 UTC) #3
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 11 months ago (2016-01-20 19:43:56 UTC) #5
commit-bot: I haz the power
4 years, 11 months ago (2016-01-20 19:46:03 UTC) #7
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/d1cb127a27a232f4406e03e7a482ba6ee26c4492
Cr-Commit-Position: refs/heads/master@{#370441}

Powered by Google App Engine
This is Rietveld 408576698