[Chromium] Update Android NDK to r7b, automatically install 64-bit linker
Created attachment 129232 [details] Patch
Comment on attachment 129232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129232&action=review > Source/WebKit/chromium/DEPS:177 > + # Directly referring to chromium_deps variable isn't possible, as the > + # dependency is not listed in Chromium's main DEPS file. > + 'third_party/aosp': > + 'http://src.chromium.org/svn/trunk/deps/third_party/aosp@122156', Is maintaining this version number going to be a hassle? Currently we just need to increment one number in this file. Can we refactor Chromium's main DEPS file or improve gclient so we can get the right version automatically?
(In reply to comment #2) > Is maintaining this version number going to be a hassle? Currently we just need to increment one number in this file. Can we refactor Chromium's main DEPS file or improve gclient so we can get the right version automatically? Android's DEPS additions are being controlled in a separate gyp repository here: http://git.chromium.org/gitweb/?p=chromium/tools/android.deps.git;a=summary An alternative approach would be to include this repository in the gclient configuration file (update-webkit-chromium:58) in case the checkout is for Android. The issue here is that this follows Chromium's path structure, thus prefixing everything with "src/", which we don't want for WebKit. Chromium's WebKit port solves this by referring to the DEPS file in the online repository and pulling the dependency from there using our own naming. I've tried getting this to work with an online version of the DEPS file, but to no avail (http://git.chromium.org/gitweb/?p=chromium/tools/android.deps.git;a=blob_plain;h=ff1118) -- it seems to try to svn checkout a git directory which obviously is not going to work. I'll look into gclient to see if I can somehow get it to check out the android.deps.git file and base the other dependencies off that.
The File() function in DEPS files is used to check out a single file from a subversion repository. The Android-specific DEPS file we're requiring is only available on git, so that won't fly in it's current form. Checking out a single file from a git repository requires us to clone it, and while that may be fine in this (uncommon) case, it could result in tons of unnecessary traffic elsewhere. A third option would be to add a "Url()" method to gclient which would download the page over HTTP, through the web-viewer. These options are both quite severe and would require quite some changes in gclient. Making the DEPS file (probably the entire repository) available over SVN would be another option, but this would have to be mirrored with the GIT repository as the master (given that Chromium depends on GIT right now). Given that the linker shouldn't require (frequent) updates I think that, for now, it's fine. I'll discuss internally to see what the ideal way would be to get WebKit to be able to access Android's DEPS file.
Created attachment 131659 [details] Patch
I moved the update to the Android NDK r7b to Bug 81005. This patch has been tested with both versions of the NDK. At least for now, I don't foresee many changes to the Android-specific DEPS file which would cause too much duplication. Furthermore, since it[1] will be updated on Chromium's schedule, referring to it directly may pull in different versions of files for which WebKit's Chromium version has not yet been updated. Until we've got more insight in what the DEPS file will be used for (as seen from WebKit's interest) aside from the custom linker and Freetype repository, I think we should be fine for now. [1] http://git.chromium.org/gitweb/?p=chromium/tools/android.deps.git;a=blob;f=DEPS
It seems like the proper way to solve this problem is to follow the dependency chain: Use the chromium_rev in Source/WebKit/chromium/DEPS to get the proper version of android.deps/DEPS and then look in that file to get the proper version of aosp. Having the version number hard-coded here is going to be a maintenance burden.
Yes, and there lies the problem: android.deps/DEPS is only available over git, and gclient has no way of referring to that. Adding dpranke; Dirk, would you have any idea on how to solve this in a clean way?
If we're going to use git to store DEPS files, it seems like we need to teach gclient how to use git. Another solution is to store the DEPS file in an SVN repo.
Comment on attachment 131659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131659&action=review > Source/WebKit/chromium/DEPS:176 > + 'http://src.chromium.org/svn/trunk/deps/third_party/aosp@122156', Can you get this using the "From()" feature in gclient? I've never used it, but I think the syntax is something like 'android_deps': File('http://git.chromium.org/gitweb/?p=chromium/tools/android.deps.git;a=blob;f=DEPS'), 'third_party/aosp': From('android_deps', 'third_party/aosp'), or something like that (there other are examples in this file). However, it seems like you still need to fix a revision of the chromium git repo (since we wouldn't want to just pick HEAD), and since you can't use the svn revision for this, you will still have two numbers to update, so I'm not sure that it's an improvement. It seems bad to me to have a git-only thing in the chromium repo, since everything else works for either svn or git.
Tony knows the webkit / gclient hooks better than I; Tony, any ideas?
(In reply to comment #4) > Checking out a single file from a git repository requires us to clone it, and while that may be fine in this (uncommon) case, it could result in tons of unnecessary traffic elsewhere. This seems reasonable to me. You would just need to update File() to understand git. I agree that cloning the whole repository seems like a waste, but you could use --depth 1 and -n to avoid pulling the whole history or writing a bunch of files to disk (reference: http://stackoverflow.com/a/2466755 ). How does it work for Chromium checkouts? Is it a separate solution entry in your .gclient file? Alternately, maybe we can add the git clone of android.deps.git to update-webkit-chromium (or maybe this script can add it to the .gclient file).
Comment on attachment 131659 [details] Patch We're fixing infrastructure to do this the right way, the first patches are already in-flight (elsewhere). I'll upload a new patch once that has been done.
Created attachment 144802 [details] Patch
New patch, I would appreciate it if you could take a look! We're now ready to start deprecating the android.deps.git repository in favor of using gclient's new target_os feature and the main DEPS file. As part of that, the versioning issue has been resolved and we now inherit the revision data from Chromium, as pretty much all other dependencies do.
Comment on attachment 144802 [details] Patch Thanks.
Peter, thanks for doing this the right way. I really appreciate it.
(In reply to comment #17) > Peter, thanks for doing this the right way. I really appreciate it. That's ok, it's the approach which should have been taken straight away. Thank you for the review! I'll wait for Ami's Chromium DEPS roll to land after which I'll put the patch on the CQ.
Created attachment 144881 [details] Patch for landing
Committed r119077: <http://trac.webkit.org/changeset/119077>