Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16031003:
Build both v1 and v2 apps on the bots. (Closed)
|
Created:
7 years, 7 months ago by weitao Modified:
7 years, 6 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, weitaosu+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionBuild both v1 and v2 apps on the bots. Change #1: building both V1 and V2 apps at the same time. V1 app goes to remoting/remoting.webapp and V1 app goes to remoting/remoting.webapp.V2.
BUG=239941
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202998
Patch Set 1 #
Total comments: 9
Patch Set 2 : Removing remoting_use_apps_v2 #Patch Set 3 : Don't build appsv2 webapp on Windows #Patch Set 4 : Updating comment #
Total comments: 3
Patch Set 5 : #
Messages
Total messages: 14 (0 generated)
Typo in CL description: last "V1" should be "V2". https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp File remoting/remoting.gyp (right): https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp#newcode... remoting/remoting.gyp:2084: # Also we need to move the plugin into the me2mom nit: s/me2mom/webapp I'm not sure this whole comment is useful any more? It seems to be justifying why we use a script rather than just using GYP 'copies'. But the build-webapp.py script is doing more than just copying files. I vote for removing this comment - let's see if Jamie agrees. https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp#newcode... remoting/remoting.gyp:2122: '<@(remoting_webapp_apps_v2_js_files)', Remove apps_v2_js_files from here? You've already removed it from the corresponding 'inputs' list. Also, I think it's worth alphabetizing the ..._files entries in both the 'inputs' and 'action' lists.
drive-by question https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp File remoting/remoting.gyp (right): https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp#newcode... remoting/remoting.gyp:2029: 'target_name': 'remoting_webapp', Why build both webapps as part of the same target? I think it would be better to have a separate target remoting_webapp_v2 that builds v2 app.
On 2013/05/24 21:39:42, Sergey Ulanov wrote: > drive-by question > > https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp > File remoting/remoting.gyp (right): > > https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp#newcode... > remoting/remoting.gyp:2029: 'target_name': 'remoting_webapp', > Why build both webapps as part of the same target? I think it would be better to > have a separate target remoting_webapp_v2 that builds v2 app. I wondered about that as well. I think that if, long term, we are going to build two webapps, it makes sense to have separate GYP targets. If this is just a temporary situation (the v2 webapp will eventually become the one-and-only webapp), I think it makes sense to keep the single target.
https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp File remoting/remoting.gyp (right): https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp#newcode... remoting/remoting.gyp:2029: 'target_name': 'remoting_webapp', On 2013/05/24 21:39:42, Sergey Ulanov wrote: > Why build both webapps as part of the same target? I think it would be better to > have a separate target remoting_webapp_v2 that builds v2 app. I don't feel strongly one way or the other, but one point in favor of this approach is that devs are likely to notice sooner if the break the patch, rather than waiting for a trybot failure to tell them. https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp#newcode... remoting/remoting.gyp:2084: # Also we need to move the plugin into the me2mom On 2013/05/24 19:28:24, Lambros wrote: > nit: s/me2mom/webapp > > I'm not sure this whole comment is useful any more? It seems to be justifying > why we use a script rather than just using GYP 'copies'. But the build-webapp.py > script is doing more than just copying files. I vote for removing this comment - > let's see if Jamie agrees. Sgtm
Besides addressing the CR comments, I also removed the "remoting_use_apps_v2" gyp definition. https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp File remoting/remoting.gyp (right): https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp#newcode... remoting/remoting.gyp:2029: 'target_name': 'remoting_webapp', On 2013/05/24 21:39:42, Sergey Ulanov wrote: > Why build both webapps as part of the same target? I think it would be better to > have a separate target remoting_webapp_v2 that builds v2 app. I will keep the v1 and v2 builds in the same target so that it is easier to catch the changes that accidentally breaks the v2 builds. https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp#newcode... remoting/remoting.gyp:2084: # Also we need to move the plugin into the me2mom On 2013/05/24 19:28:24, Lambros wrote: > nit: s/me2mom/webapp > > I'm not sure this whole comment is useful any more? It seems to be justifying > why we use a script rather than just using GYP 'copies'. But the build-webapp.py > script is doing more than just copying files. I vote for removing this comment - > let's see if Jamie agrees. Removed. https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp#newcode... remoting/remoting.gyp:2122: '<@(remoting_webapp_apps_v2_js_files)', On 2013/05/24 19:28:24, Lambros wrote: > Remove apps_v2_js_files from here? You've already removed it from the > corresponding 'inputs' list. > > Also, I think it's worth alphabetizing the ..._files entries in both the > 'inputs' and 'action' lists. Removed and sorted.
LGTM with nits. https://codereview.chromium.org/16031003/diff/18001/remoting/remoting.gyp File remoting/remoting.gyp (right): https://codereview.chromium.org/16031003/diff/18001/remoting/remoting.gyp#new... remoting/remoting.gyp:2113: # We cannot currently build the appsv2 version of WebApp on Windows as nit: rephrase as we discussed offline. https://codereview.chromium.org/16031003/diff/18001/remoting/remoting.gyp#new... remoting/remoting.gyp:2116: # We define this in a "target_conditions" section because 'plugin_path' nit: Be consistent about single/double quotes.
https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp File remoting/remoting.gyp (right): https://codereview.chromium.org/16031003/diff/1/remoting/remoting.gyp#newcode... remoting/remoting.gyp:2029: 'target_name': 'remoting_webapp', On 2013/05/25 03:30:45, Jamie wrote: > On 2013/05/24 21:39:42, Sergey Ulanov wrote: > > Why build both webapps as part of the same target? I think it would be better > to > > have a separate target remoting_webapp_v2 that builds v2 app. > > I don't feel strongly one way or the other, but one point in favor of this > approach is that devs are likely to notice sooner if the break the patch, rather > than waiting for a trybot failure to tell them. Well, then we could have two targets remoting_webapp_v[12] and then one dummy target remoting_webapp that builds both versions. Having them as separate targets will make it easier to see what each version dependends on. It would also allow building v2 app without building the host plugin and all other stuff it depends on. Beside that, having a separate target makes this patch shorter and more readable.
Please don't consider this and my previous comments as blocking - feel free to land it as is (but I didn't review all details myself). https://codereview.chromium.org/16031003/diff/18001/remoting/remoting.gyp File remoting/remoting.gyp (right): https://codereview.chromium.org/16031003/diff/18001/remoting/remoting.gyp#new... remoting/remoting.gyp:2119: ['OS != "win"', { FYI, I see patch.exe pulled on windows by perl dependency (third_party\perl\c\bin\patch.exe), you can try using it here.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/weitaosu@chromium.org/16031003/24001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/weitaosu@chromium.org/16031003/24001
Message was sent while issue was closed.
Change committed as 202998 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
