DescriptionRevert 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 #
Messages
Total messages: 7 (3 generated)
|