Skip to content
  • primiano's avatar
    Revert of Reland of: Allow base to depend on allocator (patchset #2 id:20001... · d1cb127a
    primiano authored
    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
    
    Review URL: https://codereview.chromium.org/1606393002
    
    Cr-Commit-Position: refs/heads/master@{#370441}
    d1cb127a