Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2462333002:
Crazylinker: remove the code to load component build library (Closed)
|
Created:
4 years, 1 month ago by michaelbai Modified:
4 years, 1 month ago Reviewers:
Torne CC:
chromium-reviews, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCrazylinker: remove the code to load component build library
BUG=657093
Committed: https://crrev.com/4595065335bae1a423f64abe3c1b42db75d5fa0f
Cr-Commit-Position: refs/heads/master@{#429664}
Patch Set 1 #Patch Set 2 : Revert "Revert of Crazylinker: remove the code to load component build library (patchset #4 id:6000… #
Total comments: 3
Patch Set 3 : Revert "Revert of Crazylinker: remove the code to load component build library (patchset #4 id:6000… #
Messages
Total messages: 18 (6 generated)
Description was changed from ========== Revert "Revert of Crazylinker: remove the code to load component build library (patchset #4 id:60001 of https://codereview.chromium.org/2406083002/ )" This reverts commit 6e23b4d09d51ef90bfb50cf9763acc1ee2cd381f. BUG= ========== to ========== Revert "Revert of Crazylinker: remove the code to load component build library (patchset #4 id:60001 of https://codereview.chromium.org/2406083002/ )" This reverts commit 6e23b4d09d51ef90bfb50cf9763acc1ee2cd381f. BUG=657093 ==========
michaelbai@chromium.org changed reviewers: + torne@chromium.org
Just having the CL description as revert revert is confusing, especially since this isn't a straight revert and you're ending up with different contents. It wasn't necessary to revert the change in the first place; you could have just fixed the comment in a followup CL. Now that we're here, can you give the CL a description that mentions relanding it with a fixed comment, instead of just the default revert-revert message. https://codereview.chromium.org/2462333002/diff/20001/base/android/linker/con... File base/android/linker/config.gni (right): https://codereview.chromium.org/2462333002/diff/20001/base/android/linker/con... base/android/linker/config.gni:10: # Chromium linker doesn't reliably support loading multiple libraries; This is still replacing the wrong comment. We want to *keep* the comment about instrumentation, and replace the old comment about component builds with the new one.
On 2016/11/01 13:35:51, Torne wrote: > Just having the CL description as revert revert is confusing, especially since > this isn't a straight revert and you're ending up with different contents. > > It wasn't necessary to revert the change in the first place; you could have just > fixed the comment in a followup CL. > > Now that we're here, can you give the CL a description that mentions relanding > it with a fixed comment, instead of just the default revert-revert message. > > https://codereview.chromium.org/2462333002/diff/20001/base/android/linker/con... > File base/android/linker/config.gni (right): > > https://codereview.chromium.org/2462333002/diff/20001/base/android/linker/con... > base/android/linker/config.gni:10: # Chromium linker doesn't reliably support > loading multiple libraries; > This is still replacing the wrong comment. We want to *keep* the comment about > instrumentation, and replace the old comment about component builds with the new > one. The reason I reverted the patch is I don't want to touch the code that I shouldn't, I will update the description, but now, I am confused the comment you preferred now, would you help to provide the whole comment?
https://codereview.chromium.org/2462333002/diff/20001/base/android/linker/con... File base/android/linker/config.gni (right): https://codereview.chromium.org/2462333002/diff/20001/base/android/linker/con... base/android/linker/config.gni:10: # Chromium linker doesn't reliably support loading multiple libraries; What I expected the comments to say: # Chromium linker doesn't reliably support loading multiple libraries; # disable for component builds, see crbug.com/657093. # Chromium linker causes instrumentation to return incorrect results.
https://codereview.chromium.org/2462333002/diff/20001/base/android/linker/con... File base/android/linker/config.gni (right): https://codereview.chromium.org/2462333002/diff/20001/base/android/linker/con... base/android/linker/config.gni:10: # Chromium linker doesn't reliably support loading multiple libraries; On 2016/11/02 09:57:15, Torne wrote: > What I expected the comments to say: > > # Chromium linker doesn't reliably support loading multiple libraries; > # disable for component builds, see crbug.com/657093. > # Chromium linker causes instrumentation to return incorrect results. In this way, b/11379966 is dropped, you think it is not useful ?
On 2016/11/02 15:36:43, michaelbai wrote: > https://codereview.chromium.org/2462333002/diff/20001/base/android/linker/con... > File base/android/linker/config.gni (right): > > https://codereview.chromium.org/2462333002/diff/20001/base/android/linker/con... > base/android/linker/config.gni:10: # Chromium linker doesn't reliably support > loading multiple libraries; > On 2016/11/02 09:57:15, Torne wrote: > > What I expected the comments to say: > > > > # Chromium linker doesn't reliably support loading multiple libraries; > > # disable for component builds, see crbug.com/657093. > > # Chromium linker causes instrumentation to return incorrect results. > > In this way, b/11379966 is dropped, you think it is not useful ? No. If we aren't ever going to care about fixing the component build with the chromium linker, then maintaining a comment pointing to one particular bug that refers to one particular problem with it seems irrelevant when we know there are other problems and we don't intend to fix any of them.
On 2016/11/02 15:37:52, Torne wrote: > On 2016/11/02 15:36:43, michaelbai wrote: > > > https://codereview.chromium.org/2462333002/diff/20001/base/android/linker/con... > > File base/android/linker/config.gni (right): > > > > > https://codereview.chromium.org/2462333002/diff/20001/base/android/linker/con... > > base/android/linker/config.gni:10: # Chromium linker doesn't reliably support > > loading multiple libraries; > > On 2016/11/02 09:57:15, Torne wrote: > > > What I expected the comments to say: > > > > > > # Chromium linker doesn't reliably support loading multiple libraries; > > > # disable for component builds, see crbug.com/657093. > > > # Chromium linker causes instrumentation to return incorrect results. > > > > In this way, b/11379966 is dropped, you think it is not useful ? > > No. If we aren't ever going to care about fixing the component build with the > chromium linker, then maintaining a comment pointing to one particular bug that > refers to one particular problem with it seems irrelevant when we know there are > other problems and we don't intend to fix any of them. OK, it is up to you, you are expert, I will update the comment though I don't think it is proper to drop it.
Description was changed from ========== Revert "Revert of Crazylinker: remove the code to load component build library (patchset #4 id:60001 of https://codereview.chromium.org/2406083002/ )" This reverts commit 6e23b4d09d51ef90bfb50cf9763acc1ee2cd381f. BUG=657093 ========== to ========== Crazylinker: remove the code to load component build library BUG=657093 ==========
PTAL
The bug was inconclusive as to whether it was our fault or Android's, and has been closed by bug bankruptcy anyway. I don't think it adds any info if we're just declaring this unsupported. LGTM.
The CQ bit was checked by michaelbai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Crazylinker: remove the code to load component build library BUG=657093 ========== to ========== Crazylinker: remove the code to load component build library BUG=657093 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Crazylinker: remove the code to load component build library BUG=657093 ========== to ========== Crazylinker: remove the code to load component build library BUG=657093 Committed: https://crrev.com/4595065335bae1a423f64abe3c1b42db75d5fa0f Cr-Commit-Position: refs/heads/master@{#429664} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4595065335bae1a423f64abe3c1b42db75d5fa0f Cr-Commit-Position: refs/heads/master@{#429664} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
