Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1892613007:
Refactorying gule/BUILD.gn to make merging easy. (Closed)
|
Created:
4 years, 8 months ago by michaelbai Modified:
4 years, 8 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor gule/BUILD.gn to make merging easy.
The code in the glue directory should be easy to merge from
downstream to upstream, or vice versa.
BUG=
Committed: https://crrev.com/145f47e8b39e505c8663d73115efb9636256ab3f
Cr-Commit-Position: refs/heads/master@{#387980}
Patch Set 1 #Patch Set 2 : fix unused var #
Total comments: 5
Patch Set 3 : add more comments #
Total comments: 4
Patch Set 4 : use is_upstream #
Messages
Total messages: 28 (11 generated)
The CQ bit was checked by michaelbai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892613007/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892613007/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by michaelbai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892613007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892613007/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Refactorying gule/BUILD.gn to make merging easy. The code in the glue directory should be easy to merge from downstream to upstream, or vice versa. BUG= ========== to ========== Refactorying gule/BUILD.gn to make merging easy. The code in the glue directory should be easy to merge from downstream to upstream, or vice versa. BUG= ==========
michaelbai@chromium.org changed reviewers: + agrieve@chromium.org, sgurun@chromium.org
Description was changed from ========== Refactorying gule/BUILD.gn to make merging easy. The code in the glue directory should be easy to merge from downstream to upstream, or vice versa. BUG= ========== to ========== Refactor gule/BUILD.gn to make merging easy. The code in the glue directory should be easy to merge from downstream to upstream, or vice versa. BUG= ==========
I'm not sure what this change is for... https://codereview.chromium.org/1892613007/diff/20001/android_webview/glue/BU... File android_webview/glue/BUILD.gn (right): https://codereview.chromium.org/1892613007/diff/20001/android_webview/glue/BU... android_webview/glue/BUILD.gn:41: if (_target_dir == "//android_webview/glue/glue:glue") { Wouldn't this always evaluate to true? Are you planning on symlinking this file downstream or something? Certainly comment-worthy. Also - it would make more sense to check ":$target_name" == "//android_webview/glue:glue"
torne@chromium.org changed reviewers: + torne@chromium.org
https://codereview.chromium.org/1892613007/diff/20001/android_webview/glue/BU... File android_webview/glue/BUILD.gn (right): https://codereview.chromium.org/1892613007/diff/20001/android_webview/glue/BU... android_webview/glue/BUILD.gn:41: if (_target_dir == "//android_webview/glue/glue:glue") { On 2016/04/16 00:14:59, agrieve wrote: > Wouldn't this always evaluate to true? Are you planning on symlinking this file > downstream or something? Certainly comment-worthy. > > Also - it would make more sense to check ":$target_name" == > "//android_webview/glue:glue" There is a duplicate copy of android_webview/glue downstream which we use to depend on new Android APIs not yet released to the public SDK. Once each Android release comes out we expect to be able to just push all the downstream changes to the public chromium repo, i.e. this is just a temporary fork each release. So, we need a way for this build file to make sense in both locations, to avoid the downstream version being permanently forked. I'm not sure this way is particularly great, but also not sure what else to do...
PTAL, I added comments, let me know if you have any question. https://codereview.chromium.org/1892613007/diff/20001/android_webview/glue/BU... File android_webview/glue/BUILD.gn (right): https://codereview.chromium.org/1892613007/diff/20001/android_webview/glue/BU... android_webview/glue/BUILD.gn:41: if (_target_dir == "//android_webview/glue/glue:glue") { On 2016/04/16 00:14:59, agrieve wrote: > Wouldn't this always evaluate to true? Are you planning on symlinking this file > downstream or something? Certainly comment-worthy. > > Also - it would make more sense to check ":$target_name" == > "//android_webview/glue:glue" We have a copy of this file in downstream, this is used to detect the target is downstream or upstream, then make sure they are compiled with internal or public framework jar respectively. $target_name is short name, it will always be 'glue' in this case.
https://codereview.chromium.org/1892613007/diff/20001/android_webview/glue/BU... File android_webview/glue/BUILD.gn (right): https://codereview.chromium.org/1892613007/diff/20001/android_webview/glue/BU... android_webview/glue/BUILD.gn:41: if (_target_dir == "//android_webview/glue/glue:glue") { On 2016/04/18 16:59:07, michaelbai wrote: > On 2016/04/16 00:14:59, agrieve wrote: > > Wouldn't this always evaluate to true? Are you planning on symlinking this > file > > downstream or something? Certainly comment-worthy. > > > > Also - it would make more sense to check ":$target_name" == > > "//android_webview/glue:glue" > > We have a copy of this file in downstream, this is used to detect the target is > downstream or upstream, then make sure they are compiled with internal or public > framework jar respectively. > > $target_name is short name, it will always be 'glue' in this case. I just mean that it's strange to look for a target that doesn't exist. "//android_webview/glue/glue:glue" does not exist. The target is: "//android_webview/glue:glue" The reason get_label_info is returning the former is because it's interpreting "glue" as a relative directory. You should instead pass ":glue" Maybe simpler to do one of: is_upstream = (target_out_dir == "//out-gn/Debug/obj/android_webview/glue") or is_upstream = (rebase_path(".", "//android_webview/glue") == ".") It might also make sense to put this is_upstream at the top of the file where your newly added comment about it is. https://codereview.chromium.org/1892613007/diff/40001/android_webview/glue/BU... File android_webview/glue/BUILD.gn (right): https://codereview.chromium.org/1892613007/diff/40001/android_webview/glue/BU... android_webview/glue/BUILD.gn:5: # There are two copies of this file in upstream and downstream, all nit: there's only one copy in upstream and downstream right? Maybe say: There are two copies of this file: one upstream and one downstream. https://codereview.chromium.org/1892613007/diff/40001/android_webview/glue/BU... android_webview/glue/BUILD.gn:46: # framwork jar respectively. nit: framwork
Thanks Andrew, PTAL https://codereview.chromium.org/1892613007/diff/20001/android_webview/glue/BU... File android_webview/glue/BUILD.gn (right): https://codereview.chromium.org/1892613007/diff/20001/android_webview/glue/BU... android_webview/glue/BUILD.gn:41: if (_target_dir == "//android_webview/glue/glue:glue") { On 2016/04/18 17:40:56, agrieve wrote: > On 2016/04/18 16:59:07, michaelbai wrote: > > On 2016/04/16 00:14:59, agrieve wrote: > > > Wouldn't this always evaluate to true? Are you planning on symlinking this > > file > > > downstream or something? Certainly comment-worthy. > > > > > > Also - it would make more sense to check ":$target_name" == > > > "//android_webview/glue:glue" > > > > We have a copy of this file in downstream, this is used to detect the target > is > > downstream or upstream, then make sure they are compiled with internal or > public > > framework jar respectively. > > > > $target_name is short name, it will always be 'glue' in this case. > > I just mean that it's strange to look for a target that doesn't exist. > > "//android_webview/glue/glue:glue" does not exist. The target is: > "//android_webview/glue:glue" > > The reason get_label_info is returning the former is because it's interpreting > "glue" as a relative directory. You should instead pass ":glue" > > Maybe simpler to do one of: > is_upstream = (target_out_dir == "//out-gn/Debug/obj/android_webview/glue") > or > is_upstream = (rebase_path(".", "//android_webview/glue") == ".") > > It might also make sense to put this is_upstream at the top of the file where > your newly added comment about it is. > Thanks, learning! https://codereview.chromium.org/1892613007/diff/40001/android_webview/glue/BU... File android_webview/glue/BUILD.gn (right): https://codereview.chromium.org/1892613007/diff/40001/android_webview/glue/BU... android_webview/glue/BUILD.gn:5: # There are two copies of this file in upstream and downstream, all On 2016/04/18 17:40:56, agrieve wrote: > nit: there's only one copy in upstream and downstream right? Maybe say: > > There are two copies of this file: one upstream and one downstream. Done. https://codereview.chromium.org/1892613007/diff/40001/android_webview/glue/BU... android_webview/glue/BUILD.gn:46: # framwork jar respectively. On 2016/04/18 17:40:57, agrieve wrote: > nit: framwork Done.
On 2016/04/18 18:28:55, michaelbai wrote: > Thanks Andrew, PTAL > > https://codereview.chromium.org/1892613007/diff/20001/android_webview/glue/BU... > File android_webview/glue/BUILD.gn (right): > > https://codereview.chromium.org/1892613007/diff/20001/android_webview/glue/BU... > android_webview/glue/BUILD.gn:41: if (_target_dir == > "//android_webview/glue/glue:glue") { > On 2016/04/18 17:40:56, agrieve wrote: > > On 2016/04/18 16:59:07, michaelbai wrote: > > > On 2016/04/16 00:14:59, agrieve wrote: > > > > Wouldn't this always evaluate to true? Are you planning on symlinking this > > > file > > > > downstream or something? Certainly comment-worthy. > > > > > > > > Also - it would make more sense to check ":$target_name" == > > > > "//android_webview/glue:glue" > > > > > > We have a copy of this file in downstream, this is used to detect the target > > is > > > downstream or upstream, then make sure they are compiled with internal or > > public > > > framework jar respectively. > > > > > > $target_name is short name, it will always be 'glue' in this case. > > > > I just mean that it's strange to look for a target that doesn't exist. > > > > "//android_webview/glue/glue:glue" does not exist. The target is: > > "//android_webview/glue:glue" > > > > The reason get_label_info is returning the former is because it's interpreting > > "glue" as a relative directory. You should instead pass ":glue" > > > > Maybe simpler to do one of: > > is_upstream = (target_out_dir == "//out-gn/Debug/obj/android_webview/glue") > > or > > is_upstream = (rebase_path(".", "//android_webview/glue") == ".") > > > > It might also make sense to put this is_upstream at the top of the file where > > your newly added comment about it is. > > > > Thanks, learning! > > https://codereview.chromium.org/1892613007/diff/40001/android_webview/glue/BU... > File android_webview/glue/BUILD.gn (right): > > https://codereview.chromium.org/1892613007/diff/40001/android_webview/glue/BU... > android_webview/glue/BUILD.gn:5: # There are two copies of this file in upstream > and downstream, all > On 2016/04/18 17:40:56, agrieve wrote: > > nit: there's only one copy in upstream and downstream right? Maybe say: > > > > There are two copies of this file: one upstream and one downstream. > > Done. > > https://codereview.chromium.org/1892613007/diff/40001/android_webview/glue/BU... > android_webview/glue/BUILD.gn:46: # framwork jar respectively. > On 2016/04/18 17:40:57, agrieve wrote: > > nit: framwork > > Done. lgtm
The CQ bit was checked by michaelbai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1892613007/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1892613007/60001
Message was sent while issue was closed.
Description was changed from ========== Refactor gule/BUILD.gn to make merging easy. The code in the glue directory should be easy to merge from downstream to upstream, or vice versa. BUG= ========== to ========== Refactor gule/BUILD.gn to make merging easy. The code in the glue directory should be easy to merge from downstream to upstream, or vice versa. BUG= ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Refactor gule/BUILD.gn to make merging easy. The code in the glue directory should be easy to merge from downstream to upstream, or vice versa. BUG= ========== to ========== Refactor gule/BUILD.gn to make merging easy. The code in the glue directory should be easy to merge from downstream to upstream, or vice versa. BUG= Committed: https://crrev.com/145f47e8b39e505c8663d73115efb9636256ab3f Cr-Commit-Position: refs/heads/master@{#387980} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/145f47e8b39e505c8663d73115efb9636256ab3f Cr-Commit-Position: refs/heads/master@{#387980}
Message was sent while issue was closed.
This needs to be merged to downstream glue as it's blocking other merges. There are non-trivial merge conflicts.
Message was sent while issue was closed.
On 2016/04/20 01:26:41, boliu wrote: > This needs to be merged to downstream glue as it's blocking other merges. There > are non-trivial merge conflicts. This patch needs to be tested by the different upstream bots for a while, plan to merge it tomorrow.
Message was sent while issue was closed.
On 2016/04/20 02:26:08, michaelbai wrote: > On 2016/04/20 01:26:41, boliu wrote: > > This needs to be merged to downstream glue as it's blocking other merges. > There > > are non-trivial merge conflicts. > > This patch needs to be tested by the different upstream bots for a while, plan > to merge it tomorrow. Waiting until upstream bots have run it doesn't really make sense here; even if you need to change/revert this change, then you still need to merge this one to downstream (you would just *also* need to merge any followup CL/revert as well). *All* upstream glue changes must be merged to downstream, even if they are going to be reverted or whatever else, and you should be doing this promptly after the upstream change lands, even if that's not required to keep the build green, to make everyone else's life easier. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
