Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1444113003:
GN: asset_location -> android_assets() for mojo_runner (Closed)
|
Created:
5 years, 1 month ago by agrieve Modified:
5 years ago CC:
chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, mikecase+watch_chromium.org, klundberg+watch_chromium.org, ben+mojo_chromium.org, jbudorick+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@html_viewer-assets Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGN: asset_location -> android_assets() for mojo_runner
This adds a ${target_name}_assets target for each mojo_applictation() to make it easier to add them as assets to other targets.
BUG=547162
Committed: https://crrev.com/f73e9466cd17834ad61daf4f71e8cdc4b395e922
Cr-Commit-Position: refs/heads/master@{#362578}
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : review comments #
Total comments: 3
Patch Set 4 : rebase #Patch Set 5 : rebase #
Messages
Total messages: 24 (7 generated)
agrieve@chromium.org changed reviewers: + pkotwicz@chromium.org, sky@chromium.org
pkotwicz - please review changes to apkbuilder.py sky - please review changes to mojo/
Description was changed from
==========
GN: asset_location -> android_assets() for mojo_runner
BUG=547162
==========
to
==========
GN: asset_location -> android_assets() for mojo_runner
This adds a ${target_name}_assets target for each mojo_applictation() to make it
easier to add them as assets to other targets.
BUG=547162
==========
I'm going to wait until the apkbuilder.py have been reviewed, then I'll look at the mojo vchanges. On Mon, Nov 16, 2015 at 7:27 PM, <agrieve@chromium.org> wrote: > Reviewers: sky, pkotwicz > CL: https://codereview.chromium.org/1444113003/ > > Message: > pkotwicz - please review changes to apkbuilder.py > > sky - please review changes to mojo/ > > Description: > GN: asset_location -> android_assets() for mojo_runner > > BUG=547162 > > Base URL: > https://chromium.googlesource.com/chromium/src.git@html_viewer-assets > > Affected files (+100, -97 lines): > M build/android/gyp/apkbuilder.py > M mojo/public/mojo_application.gni > M mojo/runner/BUILD.gn > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
apkbuilder.py LGTM https://codereview.chromium.org/1444113003/diff/20001/build/android/gyp/apkbu... File build/android/gyp/apkbuilder.py (right): https://codereview.chromium.org/1444113003/diff/20001/build/android/gyp/apkbu... build/android/gyp/apkbuilder.py:98: ret.append((f, os.path.join(dest_path, f[len(src_path) + 1:]))) I think that using os.path.basename() would be clearer https://codereview.chromium.org/1444113003/diff/20001/mojo/public/mojo_applic... File mojo/public/mojo_application.gni (right): https://codereview.chromium.org/1444113003/diff/20001/mojo/public/mojo_applic... mojo/public/mojo_application.gni:157: [ "${base_target_name}/${base_target_name}.mojo" ] Nit: ${base_target_name}.mojo -> ${output} https://codereview.chromium.org/1444113003/diff/20001/mojo/runner/BUILD.gn File mojo/runner/BUILD.gn (right): https://codereview.chromium.org/1444113003/diff/20001/mojo/runner/BUILD.gn#ne... mojo/runner/BUILD.gn:312: ":mojo_runner_apptests_assets", G is for Google A is for alphabetical order
https://codereview.chromium.org/1444113003/diff/20001/build/android/gyp/apkbu... File build/android/gyp/apkbuilder.py (right): https://codereview.chromium.org/1444113003/diff/20001/build/android/gyp/apkbu... build/android/gyp/apkbuilder.py:98: ret.append((f, os.path.join(dest_path, f[len(src_path) + 1:]))) On 2015/11/17 19:34:25, pkotwicz wrote: > I think that using os.path.basename() would be clearer It's not adding the past path component, but rather stripping the path prefix (there can be multiple directory levels in src_path after the stripping) https://codereview.chromium.org/1444113003/diff/20001/mojo/public/mojo_applic... File mojo/public/mojo_application.gni (right): https://codereview.chromium.org/1444113003/diff/20001/mojo/public/mojo_applic... mojo/public/mojo_application.gni:157: [ "${base_target_name}/${base_target_name}.mojo" ] On 2015/11/17 19:34:25, pkotwicz wrote: > Nit: ${base_target_name}.mojo -> ${output} Done. https://codereview.chromium.org/1444113003/diff/20001/mojo/runner/BUILD.gn File mojo/runner/BUILD.gn (right): https://codereview.chromium.org/1444113003/diff/20001/mojo/runner/BUILD.gn#ne... mojo/runner/BUILD.gn:312: ":mojo_runner_apptests_assets", On 2015/11/17 19:34:25, pkotwicz wrote: > G is for Google > A is for alphabetical order :) done.
On 2015/11/21 01:47:26, agrieve wrote: > https://codereview.chromium.org/1444113003/diff/20001/build/android/gyp/apkbu... > File build/android/gyp/apkbuilder.py (right): > > https://codereview.chromium.org/1444113003/diff/20001/build/android/gyp/apkbu... > build/android/gyp/apkbuilder.py:98: ret.append((f, os.path.join(dest_path, > f[len(src_path) + 1:]))) > On 2015/11/17 19:34:25, pkotwicz wrote: > > I think that using os.path.basename() would be clearer > > It's not adding the past path component, but rather stripping the path prefix > (there can be multiple directory levels in src_path after the stripping) > > https://codereview.chromium.org/1444113003/diff/20001/mojo/public/mojo_applic... > File mojo/public/mojo_application.gni (right): > > https://codereview.chromium.org/1444113003/diff/20001/mojo/public/mojo_applic... > mojo/public/mojo_application.gni:157: [ > "${base_target_name}/${base_target_name}.mojo" ] > On 2015/11/17 19:34:25, pkotwicz wrote: > > Nit: ${base_target_name}.mojo -> ${output} > > Done. > > https://codereview.chromium.org/1444113003/diff/20001/mojo/runner/BUILD.gn > File mojo/runner/BUILD.gn (right): > > https://codereview.chromium.org/1444113003/diff/20001/mojo/runner/BUILD.gn#ne... > mojo/runner/BUILD.gn:312: ":mojo_runner_apptests_assets", > On 2015/11/17 19:34:25, pkotwicz wrote: > > G is for Google > > A is for alphabetical order > > :) done. ping brettw ✾ ✾ ✾
On 2015/11/23 20:32:27, agrieve wrote: > On 2015/11/21 01:47:26, agrieve wrote: > > > https://codereview.chromium.org/1444113003/diff/20001/build/android/gyp/apkbu... > > File build/android/gyp/apkbuilder.py (right): > > > > > https://codereview.chromium.org/1444113003/diff/20001/build/android/gyp/apkbu... > > build/android/gyp/apkbuilder.py:98: ret.append((f, os.path.join(dest_path, > > f[len(src_path) + 1:]))) > > On 2015/11/17 19:34:25, pkotwicz wrote: > > > I think that using os.path.basename() would be clearer > > > > It's not adding the past path component, but rather stripping the path prefix > > (there can be multiple directory levels in src_path after the stripping) > > > > > https://codereview.chromium.org/1444113003/diff/20001/mojo/public/mojo_applic... > > File mojo/public/mojo_application.gni (right): > > > > > https://codereview.chromium.org/1444113003/diff/20001/mojo/public/mojo_applic... > > mojo/public/mojo_application.gni:157: [ > > "${base_target_name}/${base_target_name}.mojo" ] > > On 2015/11/17 19:34:25, pkotwicz wrote: > > > Nit: ${base_target_name}.mojo -> ${output} > > > > Done. > > > > https://codereview.chromium.org/1444113003/diff/20001/mojo/runner/BUILD.gn > > File mojo/runner/BUILD.gn (right): > > > > > https://codereview.chromium.org/1444113003/diff/20001/mojo/runner/BUILD.gn#ne... > > mojo/runner/BUILD.gn:312: ":mojo_runner_apptests_assets", > > On 2015/11/17 19:34:25, pkotwicz wrote: > > > G is for Google > > > A is for alphabetical order > > > > :) done. > > ping brettw ✾ ✾ ✾ Whoops, ping sky that is...
https://codereview.chromium.org/1444113003/diff/40001/mojo/public/mojo_applic... File mojo/public/mojo_application.gni (right): https://codereview.chromium.org/1444113003/diff/40001/mojo/public/mojo_applic... mojo/public/mojo_application.gni:8: import("//build/config/android/rules.gni") Can we make it so android/rules.gni early outs if not android?
https://codereview.chromium.org/1444113003/diff/40001/mojo/public/mojo_applic... File mojo/public/mojo_application.gni (right): https://codereview.chromium.org/1444113003/diff/40001/mojo/public/mojo_applic... mojo/public/mojo_application.gni:8: import("//build/config/android/rules.gni") On 2015/11/24 22:38:38, sky wrote: > Can we make it so android/rules.gni early outs if not android? Although I didn't start this pattern, I do agree with it because: - Having it "early out" would require indenting the entire rules.gni file - It's slower to include & parse a file just to no-op - It's clear at the call-site that it's a no-op when not is_android.
sky@chromium.org changed reviewers: + brettw@chromium.org
+brettw for thoughts of always including platform specific files, but have the
file early out if appropriate.
Basically I'm hoping to avoid:
if (is_android) {
import("//build/config/android/rules.gni")
}
everywhere the file might be needed. Instead always include the file and have
the file early out if appropriate.
https://codereview.chromium.org/1444113003/diff/40001/mojo/public/mojo_applic...
File mojo/public/mojo_application.gni (right):
https://codereview.chromium.org/1444113003/diff/40001/mojo/public/mojo_applic...
mojo/public/mojo_application.gni:8: import("//build/config/android/rules.gni")
On 2015/11/25 01:07:24, agrieve wrote:
> On 2015/11/24 22:38:38, sky wrote:
> > Can we make it so android/rules.gni early outs if not android?
>
> Although I didn't start this pattern, I do agree with it because:
I'm reminded of this discussion:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/bLBEUc-1U34/ZGwYG...
.
> - Having it "early out" would require indenting the entire rules.gni file
Having to reindent a file is hardly a reason not to do something.
> - It's slower to include & parse a file just to no-op
You're assuming gn reads the file every time the file is imported. I'm not sure
that is the case.
> - It's clear at the call-site that it's a no-op when not is_android.
I agree. OTOH as build files get larger it's pretty uncommon to notice what's at
the top. For example, do you really look at includes in your .cc files?
On 2015/11/25 17:02:45, sky wrote:
> +brettw for thoughts of always including platform specific files, but have the
> file early out if appropriate.
>
> Basically I'm hoping to avoid:
>
> if (is_android) {
> import("//build/config/android/rules.gni")
> }
Why do you want to avoid this?
It's a bit of an established pattern atm, so I don't think it makes sense to
discuss it in the context of this code-review. It would be better to bring it
discuss on gn-dev and address with a follow-up.
>
> everywhere the file might be needed. Instead always include the file and have
> the file early out if appropriate.
>
>
https://codereview.chromium.org/1444113003/diff/40001/mojo/public/mojo_applic...
> File mojo/public/mojo_application.gni (right):
>
>
https://codereview.chromium.org/1444113003/diff/40001/mojo/public/mojo_applic...
> mojo/public/mojo_application.gni:8: import("//build/config/android/rules.gni")
> On 2015/11/25 01:07:24, agrieve wrote:
> > On 2015/11/24 22:38:38, sky wrote:
> > > Can we make it so android/rules.gni early outs if not android?
> >
> > Although I didn't start this pattern, I do agree with it because:
>
> I'm reminded of this discussion:
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/bLBEUc-1U34/ZGwYG...
> .
>
> > - Having it "early out" would require indenting the entire rules.gni file
>
> Having to reindent a file is hardly a reason not to do something.
>
> > - It's slower to include & parse a file just to no-op
>
> You're assuming gn reads the file every time the file is imported. I'm not
sure
> that is the case.
I'm not. It takes time to parse it even once compared to not at all.
>
> > - It's clear at the call-site that it's a no-op when not is_android.
>
> I agree. OTOH as build files get larger it's pretty uncommon to notice what's
at
> the top. For example, do you really look at includes in your .cc files?
On 2015/11/25 18:02:32, agrieve wrote:
> On 2015/11/25 17:02:45, sky wrote:
> > +brettw for thoughts of always including platform specific files, but have
the
> > file early out if appropriate.
> >
> > Basically I'm hoping to avoid:
> >
> > if (is_android) {
> > import("//build/config/android/rules.gni")
> > }
>
> Why do you want to avoid this?
> It's a bit of an established pattern atm, so I don't think it makes sense to
> discuss it in the context of this code-review. It would be better to bring it
> discuss on gn-dev and address with a follow-up.
>
>
> >
> > everywhere the file might be needed. Instead always include the file and
have
> > the file early out if appropriate.
> >
> >
>
https://codereview.chromium.org/1444113003/diff/40001/mojo/public/mojo_applic...
> > File mojo/public/mojo_application.gni (right):
> >
> >
>
https://codereview.chromium.org/1444113003/diff/40001/mojo/public/mojo_applic...
> > mojo/public/mojo_application.gni:8:
import("//build/config/android/rules.gni")
> > On 2015/11/25 01:07:24, agrieve wrote:
> > > On 2015/11/24 22:38:38, sky wrote:
> > > > Can we make it so android/rules.gni early outs if not android?
> > >
> > > Although I didn't start this pattern, I do agree with it because:
> >
> > I'm reminded of this discussion:
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/bLBEUc-1U34/ZGwYG...
> > .
> >
> > > - Having it "early out" would require indenting the entire rules.gni file
> >
> > Having to reindent a file is hardly a reason not to do something.
> >
> > > - It's slower to include & parse a file just to no-op
> >
> > You're assuming gn reads the file every time the file is imported. I'm not
> sure
> > that is the case.
> I'm not. It takes time to parse it even once compared to not at all.
>
> >
> > > - It's clear at the call-site that it's a no-op when not is_android.
> >
> > I agree. OTOH as build files get larger it's pretty uncommon to notice
what's
> at
> > the top. For example, do you really look at includes in your .cc files?
ping sky / brettw
I'm waiting for Brett to comment. On Tue, Dec 1, 2015 at 6:38 AM, <agrieve@chromium.org> wrote: > On 2015/11/25 18:02:32, agrieve wrote: >> >> On 2015/11/25 17:02:45, sky wrote: >> > +brettw for thoughts of always including platform specific files, but >> > have > > the >> >> > file early out if appropriate. >> > >> > Basically I'm hoping to avoid: >> > >> > if (is_android) { >> > import("//build/config/android/rules.gni") >> > } > > >> Why do you want to avoid this? >> It's a bit of an established pattern atm, so I don't think it makes sense >> to >> discuss it in the context of this code-review. It would be better to bring >> it >> discuss on gn-dev and address with a follow-up. > > > >> > >> > everywhere the file might be needed. Instead always include the file and > > have >> >> > the file early out if appropriate. >> > >> > > > > https://codereview.chromium.org/1444113003/diff/40001/mojo/public/mojo_applic... >> >> > File mojo/public/mojo_application.gni (right): >> > >> > > > > https://codereview.chromium.org/1444113003/diff/40001/mojo/public/mojo_applic... >> >> > mojo/public/mojo_application.gni:8: > > import("//build/config/android/rules.gni") >> >> > On 2015/11/25 01:07:24, agrieve wrote: >> > > On 2015/11/24 22:38:38, sky wrote: >> > > > Can we make it so android/rules.gni early outs if not android? >> > > >> > > Although I didn't start this pattern, I do agree with it because: >> > >> > I'm reminded of this discussion: >> > > > > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/bLBEUc-1U34/ZGwYG... >> >> > . >> > >> > > - Having it "early out" would require indenting the entire rules.gni >> > > file >> > >> > Having to reindent a file is hardly a reason not to do something. >> > >> > > - It's slower to include & parse a file just to no-op >> > >> > You're assuming gn reads the file every time the file is imported. I'm >> > not >> sure >> > that is the case. >> I'm not. It takes time to parse it even once compared to not at all. > > >> > >> > > - It's clear at the call-site that it's a no-op when not is_android. >> > >> > I agree. OTOH as build files get larger it's pretty uncommon to notice > > what's >> >> at >> > the top. For example, do you really look at includes in your .cc files? > > > ping sky / brettw > > https://codereview.chromium.org/1444113003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
We're pretty set in the pattern of conditionally including this file. We can discuss it, but there's more than 100 cases already so I think we should separate discussions about that from specific code reviews. I general I think making such things always be includable is reasonable, but I'm more hesitant for this case because the file is 2200 lines long and indenting the entire file makes it a little harder to follow. Originally, I was also thinking I wanted to avoid unnecessarily loading files (parsing this big file is complicated and will happen regardless of whether there's a big if() around it. This is probably over-optimization. Also, this pattern matches what we do in C++. So I lean slightly toward keeping the status quo. If the consensus is that we should change, that is also file with me, but we should do that as a separate step.
LGTM
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1444113003/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1444113003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1444113003/80001
Message was sent while issue was closed.
Description was changed from
==========
GN: asset_location -> android_assets() for mojo_runner
This adds a ${target_name}_assets target for each mojo_applictation() to make it
easier to add them as assets to other targets.
BUG=547162
==========
to
==========
GN: asset_location -> android_assets() for mojo_runner
This adds a ${target_name}_assets target for each mojo_applictation() to make it
easier to add them as assets to other targets.
BUG=547162
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from
==========
GN: asset_location -> android_assets() for mojo_runner
This adds a ${target_name}_assets target for each mojo_applictation() to make it
easier to add them as assets to other targets.
BUG=547162
==========
to
==========
GN: asset_location -> android_assets() for mojo_runner
This adds a ${target_name}_assets target for each mojo_applictation() to make it
easier to add them as assets to other targets.
BUG=547162
Committed: https://crrev.com/f73e9466cd17834ad61daf4f71e8cdc4b395e922
Cr-Commit-Position: refs/heads/master@{#362578}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f73e9466cd17834ad61daf4f71e8cdc4b395e922 Cr-Commit-Position: refs/heads/master@{#362578} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
