Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1489403002:
allocator cleanup: Reorganize allocator.gyp for cleanup (Closed)
|
Created:
5 years ago by Primiano Tucci (use gerrit) Modified:
5 years 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. |
Descriptionallocator cleanup: Reorganize allocator.gyp for cleanup
This is the first step to move all the allocator.gyp conditionals inside
allocator.gyp, rather than distributing them across all the targets.
This is documented in bit.ly/allocator-cleanup under "Phase 1".
This CL just moves things around as is expected to be a noop in terms
of ninja files generated.
The next CLs (still part oh Phase 1) will:
- Remove the deprecated windows-specific tc-malloc bits and fix the
situation on Mac (which mistakenly says use_allocator=tcmalloc)
- Remove the conditions from the hundreds gyp targets.
Rationale for the CL split: the first CLs can be landed to check for
unexpected breakages without having to deal with merge conflicts on
hundrends gyp files in the case of a revert.
I did some manual testing, checking the effect of this CL on the
ninja files produced by gyp. I used these scripts to diff the ninja:
https://gist.github.com/primiano/183b31f17801f34a32e2
https://gist.github.com/primiano/fe3deb678e0a57067e7d
This CL does not cause any diff in the final ninja files*,
for the following configurations:
OS=linux
OS=linux asan=1
OS=linux component=shared_library
OS=linux use_allocator=none
OS=linux use_allocator=none component=shared_library
OS=android
OS=android component=shared_library
OS=mac
OS=mac component=shared_library
OS=mac use_allocator=none
OS=mac use_allocator=none component=shared_library
OS=win
OS=win component=shared_library
OS=win clang=1 asan=1
OS=win GYP_MSVS_VERSION=2015
OS=win GYP_MSVS_VERSION=2015 component=shared_library
*excluding the build.ninja file itself that is unstable due to
crbug.com/557851. It just includes the various sub-ninja files
though.
BUG=564618
Committed: https://crrev.com/b0a58cabac7fe2dfe9373b113088322d9f5ce3cf
Cr-Commit-Position: refs/heads/master@{#363164}
Patch Set 1 : Add comment #
Total comments: 14
Patch Set 2 : Nico review #23. Still WIP to get a zero ninja diff and make the other changes in another CL #Patch Set 3 : zero diff attempt #Patch Set 4 : In this CL use win and shared_library to keep a zero diff for win asan #Patch Set 5 : one line thing #
Total comments: 2
Patch Set 6 : s/less/fewer #Patch Set 7 : somehow lost track of dynamic_annotations along the way #Patch Set 8 : remove obsolete comment #Messages
Total messages: 49 (31 generated)
ssid@chromium.org changed reviewers: + ssid@chromium.org
https://codereview.chromium.org/1489403002/diff/40001/base/allocator/allocato... File base/allocator/allocator.gyp (right): https://codereview.chromium.org/1489403002/diff/40001/base/allocator/allocato... base/allocator/allocator.gyp:29: '../..', I guess this cannot work because tcmalloc will start including base. The order should be preserved for tcmalloc. https://codereview.chromium.org/1489403002/diff/40001/base/allocator/allocato... base/allocator/allocator.gyp:96: ['use_allocator=="tcmalloc" and os_posix==1 and OS!="mac" and OS!="ios" and OS!="android"', { This would mean we cannot include tcmalloc on windows. Previously if you build with use_alocator=tcmalloc on windows, if it is static_library build it will include allocator which will get all tcmalloc source. This would mean win includes tcmalloc. This behavior is not preserved here. https://codereview.chromium.org/1489403002/diff/40001/base/allocator/allocato... base/allocator/allocator.gyp:102: '../third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations', similarly in windows this target will not be included anymore. https://codereview.chromium.org/1489403002/diff/40001/base/allocator/allocato... base/allocator/allocator.gyp:342: ['use_vtable_verify==1', { lines 342:353 are included without any condition. This would get added all targets. Is that intended? https://codereview.chromium.org/1489403002/diff/40001/base/allocator/allocato... base/allocator/allocator.gyp:376: 'target_name': 'libcmt', I think this target is not needed if win_use_allocator_shim is 0. Why check for shim condition above and check for shared_library condition here?
Description was changed from ========== WIP allocator BUG= ========== to ========== WIP allocator Diff on Windows: http://pastebin.com/imqUzNc9 BUG= ==========
wfh@chromium.org changed reviewers: + wfh@chromium.org
https://codereview.chromium.org/1489403002/diff/40001/base/allocator/allocato... File base/allocator/allocator.gyp (right): https://codereview.chromium.org/1489403002/diff/40001/base/allocator/allocato... base/allocator/allocator.gyp:96: ['use_allocator=="tcmalloc" and os_posix==1 and OS!="mac" and OS!="ios" and OS!="android"', { On 2015/12/02 13:58:19, ssid wrote: > This would mean we cannot include tcmalloc on windows. Previously if you build > with use_alocator=tcmalloc on windows, if it is static_library build it will > include allocator which will get all tcmalloc source. This would mean win > includes tcmalloc. This behavior is not preserved here. Using tcmalloc on windows is a totally unsupported configuration - the allocator shims don't even wire in tcmalloc any more. It's a Good Thing that this is being removed.
> Using tcmalloc on windows is a totally unsupported configuration - the allocator > shims don't even wire in tcmalloc any more. It's a Good Thing that this is being > removed. Great to hear, in this case I'll nuke also the windows-specific defines to support tcmalloc there (PERFTOOLS_DLL_DECL).
Description was changed from ========== WIP allocator Diff on Windows: http://pastebin.com/imqUzNc9 BUG= ========== to ========== Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. GYP_DEFINES | ninja diffs* ------------------------------------+------------ OS=linux | none OS=linux component=shared_library | none OS=android | none OS=android component=shared_library | none OS=windows | http://pastebin.com/VLnvAFpd OS=windows component=shared_library | none OS=mac | http://pastebin.com/GxULW0W6 OS=mac component=shared_library | http://pastebin.com/R3baCz15 *excluding the build.ninja file itself that is unstable due to crbug.com/557851 and just includes the various sub-ninja files. BUG=564618 ==========
Description was changed from ========== Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. GYP_DEFINES | ninja diffs* ------------------------------------+------------ OS=linux | none OS=linux component=shared_library | none OS=android | none OS=android component=shared_library | none OS=windows | http://pastebin.com/VLnvAFpd OS=windows component=shared_library | none OS=mac | http://pastebin.com/GxULW0W6 OS=mac component=shared_library | http://pastebin.com/R3baCz15 *excluding the build.ninja file itself that is unstable due to crbug.com/557851 and just includes the various sub-ninja files. BUG=564618 ========== to ========== allocator cleanup: Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". A follow up CL (still part oh Phase 1) will remove the conditions from the gyp target. If everything is right, that CL will be a noop from the ninja viewpoint. Rationale for the CL split: this CL can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files. If this flies, the next removal CL will be big but a no-brainer. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. GYP_DEFINES | ninja diffs * ------------------------------------+------------ OS=linux | none OS=linux component=shared_library | none OS=android | none OS=android component=shared_library | none OS=windows | http://pastebin.com/VLnvAFpd OS=windows component=shared_library | none OS=mac | http://pastebin.com/GxULW0W6 OS=mac component=shared_library | http://pastebin.com/R3baCz15 The biggest diff is on mac. Note that the only diff there is the fact that the allocator target itself is not built anymore, but nothing is actually depending on it. *excluding the build.ninja file itself that is unstable due to crbug.com/557851 and just includes the various sub-ninja files. BUG=564618 ==========
primiano@chromium.org changed reviewers: + mark@chromium.org
Ok I think this is closer to what I want. The only thing I have to sort out is if I should fix now mac in common.gypi which says use_allocator=tcmalloc, which is effectively not true all the allocator.gyp dependencies are masked by either OS=linux or OS=win, so nothing really depends on that. But I wonder if there is a subtle reason why on Mac we say use_allocator=tcmalloc without actually meaning to link our forked copy of tcmalloc. Probably to keep building and running the unittest? Will take a look tomorrow, in the meantime +mark, in case he as an idea on the latter point.
Patchset #7 (id:120001) has been deleted
Patchset #6 (id:100001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:140001) has been deleted
Description was changed from ========== allocator cleanup: Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". A follow up CL (still part oh Phase 1) will remove the conditions from the gyp target. If everything is right, that CL will be a noop from the ninja viewpoint. Rationale for the CL split: this CL can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files. If this flies, the next removal CL will be big but a no-brainer. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. GYP_DEFINES | ninja diffs * ------------------------------------+------------ OS=linux | none OS=linux component=shared_library | none OS=android | none OS=android component=shared_library | none OS=windows | http://pastebin.com/VLnvAFpd OS=windows component=shared_library | none OS=mac | http://pastebin.com/GxULW0W6 OS=mac component=shared_library | http://pastebin.com/R3baCz15 The biggest diff is on mac. Note that the only diff there is the fact that the allocator target itself is not built anymore, but nothing is actually depending on it. *excluding the build.ninja file itself that is unstable due to crbug.com/557851 and just includes the various sub-ninja files. BUG=564618 ========== to ========== allocator cleanup: Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". A follow up CL (still part oh Phase 1) will remove the conditions from the hundreds gyp targets. If everything is right, that CL will be a noop from the ninja viewpoint. Rationale for the CL split: this CL can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. In other words, if this sticks, the next removal CL will be big but a no-brainer. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. GYP_DEFINES | ninja diffs * ------------------------------------+------------ OS=linux | none OS=linux component=shared_library | none OS=android | none OS=android component=shared_library | none OS=windows | http://pastebin.com/VLnvAFpd OS=windows component=shared_library | none OS=mac | http://pastebin.com/GxULW0W6 OS=mac component=shared_library | http://pastebin.com/R3baCz15 The biggest diff is on mac. Note that the only diff there is the fact that the allocator target itself is not built anymore, but nothing is actually depending on it. *excluding the build.ninja file itself that is unstable due to crbug.com/557851 and just includes the various sub-ninja files. BUG=564618 ==========
primiano@chromium.org changed reviewers: + thakis@chromium.org
Also +thakis as a gyp-ninja and ninja-ninja
Probably a bug, I don’t think we ever used tcmalloc on Mac.
This mostly organizes things around, right? lgtm. Maybe the rotating around could've been done in a completely behavior-preserving change first and the actual change in a follow-up, then the diff would be a bit easier to verity. https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... File base/allocator/allocator.gyp (left): https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... base/allocator/allocator.gyp:299: 'libcmt', where did this go? https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... File base/allocator/allocator.gyp (right): https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... base/allocator/allocator.gyp:26: 'type': 'static_library', Maybe this should be type none by default and only type static_library in cases where it's real? Else you end up with a source-less static_library target which confuses at least some generators (but I suppose that's not new in this CL) https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... base/allocator/allocator.gyp:30: #################################################################### ########################################################### # MAYBE THIS COULD BE IN LOWER CASE; NOT SURE THE #### LINES # ADD MUCH EITHER ########################################################### https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... base/allocator/allocator.gyp:80: # TODO(primiano): remove the per-os conditionals one common.gypi is s/one/once https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... base/allocator/allocator.gyp:93: # base/ mini-fork. Do not factor these out in the allocator target. I'm not sure what the second sentence means. We are in the allocator target. "do not factor these out of"? https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... base/allocator/allocator.gyp:341: 'disable_debugallocation%': 0, moving this into a configuration subtly changes behavior on when this is merge. If some gyp file set this to 0 in a gypi, I think this would now no longer overwrite this default var. Using GYP_DEFINES to set it still works. Probably doesn't matter. https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... base/allocator/allocator.gyp:371: ], # condditions of 'allocator' target. s/condditions/conditions
> This mostly organizes things around, right? Right. > Maybe the rotating around could've been done in a completely behavior-preserving change first and the actual change in a follow-up, then the diff would be a bit easier to verity. Ok, let me give it a try. Might reduce the amount of brain power required here if I can get a pure zero-diff. https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... File base/allocator/allocator.gyp (left): https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... base/allocator/allocator.gyp:299: 'libcmt', On 2015/12/02 21:32:21, Nico wrote: > where did this go? On line 42. You could argue: "wait but !shared_library is not the same of win_use_allocator_shim" (in fact use_shim is 1 only if static + MSVS2013), while !shared_library is true also for MSVS 2015 static builds. However this entire target is depended on by the various win targets using an if (win and win_use_allocator_shim), so this more general condition should never be hit and be bool-equivalent to checking for win_shim. As a matter of facts, I will add the MSVS=2015 to the list of the ninja diff cross-checks. https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... File base/allocator/allocator.gyp (right): https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... base/allocator/allocator.gyp:26: 'type': 'static_library', On 2015/12/02 21:32:21, Nico wrote: > Maybe this should be type none by default and only type static_library in cases > where it's real? Else you end up with a source-less static_library target which > confuses at least some generators (but I suppose that's not new in this CL) OK, if you agree let's do this. I'm putting a TODO right now and will do once I make the outer pass on the other gyp files and remove most of the conditionals inside here. Rationale: right now the only way I see for doing that is to either: - have a condition section for the type which is the union of the conditions below. - Have the type: static library inside each conditionals. Both of them look a bit fragile and easy to get wrong during the refactorings. Ideally I'd like to end up in a stage here where: if use_allocator == none -> target type = none However this is not the situation today, as use_allocator right now (with or without these refactorings) is a big lie which just sometimes happens to be true. https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... base/allocator/allocator.gyp:30: #################################################################### On 2015/12/02 21:32:21, Nico wrote: > ########################################################### > # MAYBE THIS COULD BE IN LOWER CASE; NOT SURE THE #### LINES > # ADD MUCH EITHER > ########################################################### Ok, ok, agree. this was mostly driven my yesterday frustration in scrolling this file up and down all the day. https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... base/allocator/allocator.gyp:80: # TODO(primiano): remove the per-os conditionals one common.gypi is On 2015/12/02 21:32:21, Nico wrote: > s/one/once Done. https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... base/allocator/allocator.gyp:93: # base/ mini-fork. Do not factor these out in the allocator target. On 2015/12/02 21:32:21, Nico wrote: > I'm not sure what the second sentence means. We are in the allocator target. "do > not factor these out of"? Reworded. What I mean here is that it would be temping to say: include_dir: ['.', '../..'] in the allocator target, outside of the conditions (rationale: this is the same for win shim), and here (inside this tcmalloc condition) just add the two <(tcmalloc_dir) includes. But that doesn't work, so each condition section has to repeat the include paths (unless there is some gyp trick to enforce the order) https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... base/allocator/allocator.gyp:341: 'disable_debugallocation%': 0, On 2015/12/02 21:32:21, Nico wrote: > moving this into a configuration subtly changes behavior on when this is merge. > If some gyp file set this to 0 in a gypi, I think this would now no longer > overwrite this default var. Using GYP_DEFINES to set it still works. Probably > doesn't matter. I can move it to the variables on top, but I think that then I have to do the variables { variables { variables trick to make it expand here. Honestly, not sure who still uses this, and am pretty sure that this is only intended to be passed by GYP_DEFINES (don't see this to be mentioned anywhere else) https://codereview.chromium.org/1489403002/diff/160001/base/allocator/allocat... base/allocator/allocator.gyp:371: ], # condditions of 'allocator' target. On 2015/12/02 21:32:21, Nico wrote: > s/condditions/conditions Done.
Description was changed from ========== allocator cleanup: Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". A follow up CL (still part oh Phase 1) will remove the conditions from the hundreds gyp targets. If everything is right, that CL will be a noop from the ninja viewpoint. Rationale for the CL split: this CL can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. In other words, if this sticks, the next removal CL will be big but a no-brainer. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. GYP_DEFINES | ninja diffs * ------------------------------------+------------ OS=linux | none OS=linux component=shared_library | none OS=android | none OS=android component=shared_library | none OS=windows | http://pastebin.com/VLnvAFpd OS=windows component=shared_library | none OS=mac | http://pastebin.com/GxULW0W6 OS=mac component=shared_library | http://pastebin.com/R3baCz15 The biggest diff is on mac. Note that the only diff there is the fact that the allocator target itself is not built anymore, but nothing is actually depending on it. *excluding the build.ninja file itself that is unstable due to crbug.com/557851 and just includes the various sub-ninja files. BUG=564618 ========== to ========== allocator cleanup: Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - remove the conditions from the hundreds gyp targets. Rationale for the CL split: this CL can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. In other words, if this sticks, the next removal CL will be big but a no-brainer. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ==========
Description was changed from ========== allocator cleanup: Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - remove the conditions from the hundreds gyp targets. Rationale for the CL split: this CL can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. In other words, if this sticks, the next removal CL will be big but a no-brainer. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ========== to ========== allocator cleanup: Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - remove the conditions from the hundreds gyp targets. Rationale for the CL split: this CL can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ==========
Description was changed from ========== allocator cleanup: Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - remove the conditions from the hundreds gyp targets. Rationale for the CL split: this CL can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ========== to ========== allocator cleanup: Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - Remove the conditions from the hundreds gyp targets. Rationale for the CL split: this CL can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ==========
Description was changed from ========== allocator cleanup: Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - Remove the conditions from the hundreds gyp targets. Rationale for the CL split: this CL can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ========== to ========== allocator cleanup: Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - Remove the conditions from the hundreds gyp targets. Rationale for the CL split: this CL can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux asan=1 OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ==========
Description was changed from ========== allocator cleanup: Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - Remove the conditions from the hundreds gyp targets. Rationale for the CL split: this CL can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux asan=1 OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ========== to ========== allocator cleanup: Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - Remove the conditions from the hundreds gyp targets. Rationale for the CL split: the first CLs can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux asan=1 OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ==========
Description was changed from ========== allocator cleanup: Add target conditions to allocator.gyp This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - Remove the conditions from the hundreds gyp targets. Rationale for the CL split: the first CLs can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux asan=1 OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ========== to ========== allocator cleanup: Reorganize allocator.gyp for cleanup This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". This CL just moves things around as is expected to be a noop in terms of ninja files generated. The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - Remove the conditions from the hundreds gyp targets. Rationale for the CL split: the first CLs can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux asan=1 OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ==========
Description was changed from ========== allocator cleanup: Reorganize allocator.gyp for cleanup This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". This CL just moves things around as is expected to be a noop in terms of ninja files generated. The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - Remove the conditions from the hundreds gyp targets. Rationale for the CL split: the first CLs can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux asan=1 OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ========== to ========== allocator cleanup: Reorganize allocator.gyp for cleanup This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". This CL just moves things around as is expected to be a noop in terms of ninja files generated. The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - Remove the conditions from the hundreds gyp targets. Rationale for the CL split: the first CLs can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux asan=1 OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win OS=win component=shared_library OS=win clang=1 asan=1 *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ==========
Patchset #5 (id:240001) has been deleted
Nico, I reworked this CL. AFAICT this causes pure zero-ninja-diff CL, and the next changes that I marked here with TODOs should be very easy to review. wfh@ mind taking a look now? This one itself should be non controversial in this state.
lgtm! https://codereview.chromium.org/1489403002/diff/260001/base/allocator/allocat... File base/allocator/allocator.gyp (right): https://codereview.chromium.org/1489403002/diff/260001/base/allocator/allocat... base/allocator/allocator.gyp:32: # (crbug.com/564618) bring this file to a saner state (less conditions). s/less/fewer/ i think (but i'm not a native speaker)
https://codereview.chromium.org/1489403002/diff/260001/base/allocator/allocat... File base/allocator/allocator.gyp (right): https://codereview.chromium.org/1489403002/diff/260001/base/allocator/allocat... base/allocator/allocator.gyp:32: # (crbug.com/564618) bring this file to a saner state (less conditions). Nico wrote: > s/less/fewer/ i think (but i'm not a native speaker) Affirmed.
Description was changed from ========== allocator cleanup: Reorganize allocator.gyp for cleanup This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". This CL just moves things around as is expected to be a noop in terms of ninja files generated. The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - Remove the conditions from the hundreds gyp targets. Rationale for the CL split: the first CLs can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux asan=1 OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win OS=win component=shared_library OS=win clang=1 asan=1 *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ========== to ========== allocator cleanup: Reorganize allocator.gyp for cleanup This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". This CL just moves things around as is expected to be a noop in terms of ninja files generated. The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - Remove the conditions from the hundreds gyp targets. Rationale for the CL split: the first CLs can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux asan=1 OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win OS=win component=shared_library OS=win clang=1 asan=1 OS=win GYP_MSVS_VERSION=2015 OS=win GYP_MSVS_VERSION=2015 component=shared_library *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ==========
lgtm
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1489403002/#ps320001 (title: "remove obsolete comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1489403002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1489403002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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/1489403002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1489403002/320001
Message was sent while issue was closed.
Description was changed from ========== allocator cleanup: Reorganize allocator.gyp for cleanup This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". This CL just moves things around as is expected to be a noop in terms of ninja files generated. The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - Remove the conditions from the hundreds gyp targets. Rationale for the CL split: the first CLs can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux asan=1 OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win OS=win component=shared_library OS=win clang=1 asan=1 OS=win GYP_MSVS_VERSION=2015 OS=win GYP_MSVS_VERSION=2015 component=shared_library *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ========== to ========== allocator cleanup: Reorganize allocator.gyp for cleanup This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". This CL just moves things around as is expected to be a noop in terms of ninja files generated. The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - Remove the conditions from the hundreds gyp targets. Rationale for the CL split: the first CLs can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux asan=1 OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win OS=win component=shared_library OS=win clang=1 asan=1 OS=win GYP_MSVS_VERSION=2015 OS=win GYP_MSVS_VERSION=2015 component=shared_library *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== allocator cleanup: Reorganize allocator.gyp for cleanup This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". This CL just moves things around as is expected to be a noop in terms of ninja files generated. The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - Remove the conditions from the hundreds gyp targets. Rationale for the CL split: the first CLs can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux asan=1 OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win OS=win component=shared_library OS=win clang=1 asan=1 OS=win GYP_MSVS_VERSION=2015 OS=win GYP_MSVS_VERSION=2015 component=shared_library *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 ========== to ========== allocator cleanup: Reorganize allocator.gyp for cleanup This is the first step to move all the allocator.gyp conditionals inside allocator.gyp, rather than distributing them across all the targets. This is documented in bit.ly/allocator-cleanup under "Phase 1". This CL just moves things around as is expected to be a noop in terms of ninja files generated. The next CLs (still part oh Phase 1) will: - Remove the deprecated windows-specific tc-malloc bits and fix the situation on Mac (which mistakenly says use_allocator=tcmalloc) - Remove the conditions from the hundreds gyp targets. Rationale for the CL split: the first CLs can be landed to check for unexpected breakages without having to deal with merge conflicts on hundrends gyp files in the case of a revert. I did some manual testing, checking the effect of this CL on the ninja files produced by gyp. I used these scripts to diff the ninja: https://gist.github.com/primiano/183b31f17801f34a32e2 https://gist.github.com/primiano/fe3deb678e0a57067e7d This CL does not cause any diff in the final ninja files*, for the following configurations: OS=linux OS=linux asan=1 OS=linux component=shared_library OS=linux use_allocator=none OS=linux use_allocator=none component=shared_library OS=android OS=android component=shared_library OS=mac OS=mac component=shared_library OS=mac use_allocator=none OS=mac use_allocator=none component=shared_library OS=win OS=win component=shared_library OS=win clang=1 asan=1 OS=win GYP_MSVS_VERSION=2015 OS=win GYP_MSVS_VERSION=2015 component=shared_library *excluding the build.ninja file itself that is unstable due to crbug.com/557851. It just includes the various sub-ninja files though. BUG=564618 Committed: https://crrev.com/b0a58cabac7fe2dfe9373b113088322d9f5ce3cf Cr-Commit-Position: refs/heads/master@{#363164} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b0a58cabac7fe2dfe9373b113088322d9f5ce3cf Cr-Commit-Position: refs/heads/master@{#363164}
Message was sent while issue was closed.
Ssid, just realized I forgot to answer to a comment of yours:
>base/allocator/allocator.gyp:342: ['use_vtable_verify==1', {
>lines 342:353 are included without any condition. This would get added all
>targets. Is that intended?
Yes, I had a chat with the original author. That should apply to whatever
allocator we use (and is only explicitly enabled by developers, so shouldn't
have any side effects)
>base/allocator/allocator.gyp:376: 'target_name': 'libcmt',
>I think this target is not needed if win_use_allocator_shim is 0.
>Why check for shim condition above and check for shared_library condition here?
I reverted that change here to obtain a zero-diff. will re-do in the next CLs
I think all the other comments instead got addressed
|
