|
|
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. |
DescriptionReland (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 #
Messages
Total messages: 16 (11 generated)
Description was changed from ========== Reland (2) of: Allow base to depend on allocator Reason for reland: Fixed clearkeycdmadapter GN target which caused a breakage in https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN/builds/10231 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 ========== to ========== Reland (2) of: Allow base to depend on allocator Reason for reland: Fixed clearkeycdmadapter GN target which caused a breakage in https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN/builds/10231 Rationale for the fix: clearkeycdmadapter indirectly depends on base across a shared library boundary, i.e. clearkeycdmadapter (.dll) -> clearkeycdm (.dll) -> base (.lib) The indirect base dependency causes the original cmtlib to be stripped (good) without receiving the shim-layer (which caused the breakage). The direct base dependency will give shim layer to clearkeycdmadapter. 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 ==========
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Description was changed from ========== Reland (2) of: Allow base to depend on allocator Reason for reland: Fixed clearkeycdmadapter GN target which caused a breakage in https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN/builds/10231 Rationale for the fix: clearkeycdmadapter indirectly depends on base across a shared library boundary, i.e. clearkeycdmadapter (.dll) -> clearkeycdm (.dll) -> base (.lib) The indirect base dependency causes the original cmtlib to be stripped (good) without receiving the shim-layer (which caused the breakage). The direct base dependency will give shim layer to clearkeycdmadapter. 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 ========== to ========== 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 BUG=564618 ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== 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 BUG=564618 ========== to ========== 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 ==========
primiano@chromium.org changed reviewers: + kbr@chromium.org
The reason for TBR: I am now in the state where this CL passes the CQ but gets reverted on the waterfall breaking bots which are not covered by CQ (yes, I am filing infra bugs). I am attemping again in EMEA time, so if something breaks I won't disrupt MTV too much and revert before sun rises there. If you are unhappy about the changes in PatchSet 1 here (which is the diff w.r.t. the CL that was previously reviewed) I'm happy to fix whatever needed or revert the CL fully. But, unfortunately, this is my only way to figure out if there is something else that would break or this is all.
The CQ bit was checked by primiano@chromium.org
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
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20002)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e97b9201e99ad1a0c22b631762d0bb5f3fdd3d83 Cr-Commit-Position: refs/heads/master@{#370694}
Message was sent while issue was closed.
lgtm after the fact |