Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2858043006:
Test for Gradle project generation. (Closed)
DescriptionTest for Gradle project generation.
BUG=webrtc:7608
Review-Url: https://codereview.webrtc.org/2858043006
Cr-Commit-Position: refs/heads/master@{#18063}
Committed: https://chromium.googlesource.com/external/webrtc/+/2e3574dd1828aef31b2e5cdaeb3e1376ea8001be
Patch Set 1 : Test for Gradle project generation. #
Total comments: 7
Patch Set 2 : Address kjellander's comments. #Patch Set 3 : Import gradle in DEPS. #
Total comments: 1
Messages
Total messages: 25 (13 generated)
Patchset #1 (id:1) has been deleted
sakal@webrtc.org changed reviewers: + kjellander@webrtc.org
PTAL, does this look like something we could run on the bots?
The gradle stuff needs to be put in a third_party directory, similar to https://chromium.googlesource.com/external/webrtc/+/master/webrtc/examples/an... etc. Preferably we would even have it DEPSed in if possible, but that requires us to have a Git mirror for it in Chrome infra. Where is the original location you pulled these files from? That third_party dir should also contain a README.webrtc file, similar to the README.chromium files in the Chromium repo, which I'm surprised to not find in the autobahn dir. https://codereview.webrtc.org/2858043006/diff/20001/webrtc/examples/androidte... File webrtc/examples/androidtests/gradle_project_test.py (right): https://codereview.webrtc.org/2858043006/diff/20001/webrtc/examples/androidte... webrtc/examples/androidtests/gradle_project_test.py:25: "Two blank lines between top-level definitions, one blank line between method definitions." https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines https://codereview.webrtc.org/2858043006/diff/20001/webrtc/examples/androidte... webrtc/examples/androidtests/gradle_project_test.py:63: _RunCommand([GRADLEW_BIN, 'assembleDebug'], project_dir) Is this going to build AppRTCMobile or everything? Either way, it'll be take significant time to perform this build, so if we can make it use Goma it would be good. Is that possible? https://codereview.webrtc.org/2858043006/diff/20001/webrtc/examples/androidte... webrtc/examples/androidtests/gradle_project_test.py:65: shutil.rmtree(project_dir, True) Please put lines 59-63 within a a try: block and put this one inside finally:
Gradle source code is available here: https://github.com/gradle/gradle If you can get that to the Chrome infra, I think we can pull it using DEPS files. I generated the files in here following instructions here: https://docs.gradle.org/current/userguide/gradle_wrapper.html#sec:wrapper_gen... I copied a license file to the folder from here now: https://github.com/gradle/gradle/blob/master/LICENSE https://codereview.webrtc.org/2858043006/diff/20001/webrtc/examples/androidte... File webrtc/examples/androidtests/gradle_project_test.py (right): https://codereview.webrtc.org/2858043006/diff/20001/webrtc/examples/androidte... webrtc/examples/androidtests/gradle_project_test.py:25: On 2017/05/05 07:30:13, kjellander_webrtc wrote: > "Two blank lines between top-level definitions, one blank line between method > definitions." > https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines Done. https://codereview.webrtc.org/2858043006/diff/20001/webrtc/examples/androidte... webrtc/examples/androidtests/gradle_project_test.py:63: _RunCommand([GRADLEW_BIN, 'assembleDebug'], project_dir) On 2017/05/05 07:30:13, kjellander_webrtc wrote: > Is this going to build AppRTCMobile or everything? Either way, it'll be take > significant time to perform this build, so if we can make it use Goma it would > be good. Is that possible? This is building AppRTCMobile. It is copying pre-compiled native library from the GN-build so only the Java code is built. This takes roughly 15 seconds on my computer. I don't think it is possible to run gradle builds on goma? We have to ensure AppRTCMobile has been built in the GN-output folder before running this script so that the native library is available. Is this something we can do easily or should this script execute ninja? https://codereview.webrtc.org/2858043006/diff/20001/webrtc/examples/androidte... webrtc/examples/androidtests/gradle_project_test.py:65: shutil.rmtree(project_dir, True) On 2017/05/05 07:30:13, kjellander_webrtc wrote: > Please put lines 59-63 within a a try: block and put this one inside finally: Done.
Please file a ticket similar to https://bugs.chromium.org/p/chromium/issues/detail?id=689036 and we'll get a repo setup at https://chromium.googlesource.com/external/github.com/gradle/gradle which we can use from the DEPS file (in the deps_os section for 'android'). That seems to be the best approach. https://codereview.webrtc.org/2858043006/diff/20001/webrtc/examples/androidte... File webrtc/examples/androidtests/gradle_project_test.py (right): https://codereview.webrtc.org/2858043006/diff/20001/webrtc/examples/androidte... webrtc/examples/androidtests/gradle_project_test.py:63: _RunCommand([GRADLEW_BIN, 'assembleDebug'], project_dir) On 2017/05/05 07:53:26, sakal wrote: > On 2017/05/05 07:30:13, kjellander_webrtc wrote: > > Is this going to build AppRTCMobile or everything? Either way, it'll be take > > significant time to perform this build, so if we can make it use Goma it would > > be good. Is that possible? > > This is building AppRTCMobile. It is copying pre-compiled native library from > the GN-build so only the Java code is built. This takes roughly 15 seconds on my > computer. I don't think it is possible to run gradle builds on goma? That's fine, let's just build this now, 15 seconds is no big deal. > We have to ensure AppRTCMobile has been built in the GN-output folder before > running this script so that the native library is available. Is this something > we can do easily or should this script execute ninja? Since we'll run this as a test, it happens after compile, which compiles everything, so that should be fine.
Patchset #3 (id:60001) has been deleted
On 2017/05/05 08:38:46, kjellander_webrtc wrote: > Please file a ticket similar to > https://bugs.chromium.org/p/chromium/issues/detail?id=689036 and we'll get a > repo setup at > https://chromium.googlesource.com/external/github.com/gradle/gradle which we can > use from the DEPS file (in the deps_os section for 'android'). That seems to be > the best approach. Done, I updated the CL to use the cloned repo.
lgtm but please create a bug to track rolling this out, as it will also involve more CLs adding the test to the bots etc.
Description was changed from ========== Test for Gradle project generation. BUG= ========== to ========== Test for Gradle project generation. BUG=7608 ==========
sakal@webrtc.org changed reviewers: + magjed@webrtc.org
Added Magnus for androidtests ownership. PTAL
The CQ bit was checked by sakal@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Test for Gradle project generation. BUG=7608 ========== to ========== Test for Gradle project generation. BUG=webrtc:7608 ==========
On 2017/05/09 08:40:01, sakal wrote: > Added Magnus for androidtests ownership. PTAL I added webrtc: prefix to the BUG, it's needed if you want links and bugdroid posting to work.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by sakal@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494320899922810,
"parent_rev": "9ec47b1e9264d6d95921cd96cc72f87e5f29144e", "commit_rev":
"2e3574dd1828aef31b2e5cdaeb3e1376ea8001be"}
Message was sent while issue was closed.
Description was changed from ========== Test for Gradle project generation. BUG=webrtc:7608 ========== to ========== Test for Gradle project generation. BUG=webrtc:7608 Review-Url: https://codereview.webrtc.org/2858043006 Cr-Commit-Position: refs/heads/master@{#18063} Committed: https://chromium.googlesource.com/external/webrtc/+/2e3574dd1828aef31b2e5cdae... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/2e3574dd1828aef31b2e5cdae...
Message was sent while issue was closed.
https://codereview.webrtc.org/2858043006/diff/80001/webrtc/examples/androidte... File webrtc/examples/androidtests/gradle_project_test.py (right): https://codereview.webrtc.org/2858043006/diff/80001/webrtc/examples/androidte... webrtc/examples/androidtests/gradle_project_test.py:70: shutil.rmtree(project_dir, True) Hmm, I was going to try this script locally and I didn't read the instructions carefully. Because of this I interpreted the build_dir_android directory to be the one where the output would be generated. Then after running and seeing it fail due to missing the build_vars.txt file in it, I passed --project_dir=. thinking maybe the script just wants a path to my checkout (don't know why I thought that). To my surprise my whole src/ dir was wiped! Luckily I don't think I had any unfinished work on it (that isn't at least in Rietveld as well), but maybe we can add a check for a .git directory in it and raise an error instead of wiping it? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
