New issue
Advanced search Search tips

Issue 852347 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Jun 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug

Blocking:
issue 495204



Sign in to add a comment

Make win/cross builds work without the vs tools auto-download

Project Member Reported by thakis@chromium.org, Jun 13 2018

Issue description

For people with access to a google-internal storage bucket, we download the vs toolchain at runhooks time. The win/cross build automatically uses that if available.

For people without access to that internal bucket, there's no easy-to-use way to use crossbuilds yet.

hferreiro@igalia.com is working on this.

The idea is to make it possible to point DEPOT_TOOLS_WIN_TOOLCHAIN to a local zip file created by package_from_installed.py (hferreiro, could you provide a concrete example incantation, probably as a comment on this bug at first, and once everything is in we can update docs/win_cross.md?)


Here's a demo:
https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1098256
https://chromium-review.googlesource.com/c/chromium/src/+/1098257

Adding folks who know this code well. Would you be interested in reviewing the changes above?

Looks like kkinnunen added DEPOT_TOOLS_WIN_TOOLCHAIN_HTTP_BASE_URL fairly recently; I don't know if and how these two things should interact.

 
I created the zip file by running:

$ python src/third_party/depot_tools/win_toolchain/package_from_installed.py 2017 -w 10.0.17134.0

This created `d70f7c054461909517d3c4f7b407a04a9e4a12f1.zip`. Then, using the linked CLs, I set up DEPOT_TOOLS_WIN_TOOLCHAIN=<path to zip file>. Then, `gclient sync` and `ninja` worked as stated in `docs/win_cross.md`.

Another option would be to change DEPOT_TOOLS_WIN_TOOLCHAIN_HTTP_BASE_URL to DEPOT_TOOLS_WIN_TOOLCHAIN_BASE_URL supporting also local files. I tried it by using `file://` instead of `http://` and the original code worked ok.
Yeah, I'd have use for a feature where DEPOT_TOOLS_WIN_TOOLCHAIN_HTTP_BASE_URL would be renamed to DEPOT_TOOLS_WIN_TOOLCHAIN_BASE_URL=<gs:// url, https:// url, file:/// url> and then the three backeds would be special cased in the code. It could default to the google-internal gs://. It'd be great if the file url would be special cased to shutils implementation (normal copy) instead of relying on httplib implementation, since it would then work with windows UNC share urls.


Note, for chromium there is already the GYP_MSVS_HASH_<canonical sha>=<your sha>, which more correct, more cumbersome version of the proposed patches. More correct in the sense that you explicitly have to acknowledge "I know <my sha> toolchain equals to this <canonical sha> toolchain and it will produce correct binaries" as opposed to "Just use this toolchain, it will produce correct binaries". It also supports the the property that the source code revisions define the toolchain, so it supports automatic build systems and bisecting.

So the code should already work with:
DEPOT_TOOLS_WIN_TOOLCHAIN_HTTP_BASE_URL=file:///yourdir
GYP_MSVS_HASH_5454e45bf3764c03d3fc1024b3bf5bc41e3ab62c=d70f7c054461909517d3c4f7b407a04a9e4a12f1
build

It has the added benefit that once the toolchain is upgraded, it will not work anymore, so you know you need to upgrade. The error messages aren't really descriptive.




As an unrelated aside: a bigger, nicer change would be if the clang and msvs would be downloaded in uniform fashion, implemented either in depot_tools or in chromium. This way other projects like goma client which copy chromium build rules would have less chances to make errors that break the third-party ppl.
I knew about GYP_MSVS_HASH_<canonical sha>=<your sha> but I didn't go for it because I found it very inconvenient to have to set it up locally in every toolchain change, but I guess you're right it is the right approach.

What do you mean by this: "clang and msvs would be downloaded in uniform fashion"?
>What do you mean by this: "clang and msvs would be downloaded in uniform fashion"?

Currently msvs is downloaded in a somewhat wayabout way by depot_tools. Moreover, for chromium the third_party/depot_tools downloads it.
Currently clang is downloaded by a downloader that downloads clang.

For example, goma client copied chromium scripts at some point but they bitrotted and were never kept in sync. So even though depot_tools was improved, goma client just breaks. The goma client does not get fixed due to it working for google internal developers.

So if clang and msvs were downloaded by a downloader that downloads toolchains, then it would be harder to get wrong and maybe easier to fix. But this is a minor point (and I cannot make it happen anyway)




Comment 6 by thakis@chromium.org, Jun 14 2018

Let's keep this bug about the win/cross build, not about how the sdk and clang downloaders work.

scottmg, brucedawson: Do you have any opinion on what to do here?

If not, what's the path forward here? Replace DEPOT_TOOLS_WIN_TOOLCHAIN_HTTP_BASE_URL with DEPOT_TOOLS_WIN_TOOLCHAIN_BASE_URL and make that work?
> If not, what's the path forward here? Replace DEPOT_TOOLS_WIN_TOOLCHAIN_HTTP_BASE_URL with DEPOT_TOOLS_WIN_TOOLCHAIN_BASE_URL and make that work?

That sounds fine to me.
Sure, sounds fine.
I have just pushed the agreed changes.
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/c5a26a769e69377391ed9bf71ca74d7eae5e6717

commit c5a26a769e69377391ed9bf71ca74d7eae5e6717
Author: Henrique Ferreiro <hferreiro@igalia.com>
Date: Fri Jun 22 12:44:05 2018

[win-cross] Support using a zip file for the Windows SDK

Renames DEPOT_TOOLS_WIN_TOOLCHAIN_HTTP_BASE_URL to
DEPOT_TOOLS_WIN_TOOLCHAIN_BASE_LOCATION. This environment variable can
now point to a local directory where the Windows SDK zip file is stored.
This allows non-Googlers to cross-compile Chromium for Windows.

Bug:  852347 
Change-Id: I00650e84247f35b4b8cfba204e0f2afd0882b69b
Reviewed-on: https://chromium-review.googlesource.com/1098256
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>

[modify] https://crrev.com/c5a26a769e69377391ed9bf71ca74d7eae5e6717/win_toolchain/get_toolchain_if_necessary.py

Project Member

Comment 11 by bugdroid1@chromium.org, Jun 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/78adb83ddd5b3236bef406ca1404f7606f1d0342

commit 78adb83ddd5b3236bef406ca1404f7606f1d0342
Author: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Jun 22 14:23:21 2018

Roll src/third_party/depot_tools 7999d926809f..c5a26a769e69 (1 commits)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/7999d926809f..c5a26a769e69


git log 7999d926809f..c5a26a769e69 --date=short --no-merges --format='%ad %ae %s'
2018-06-22 hferreiro@igalia.com [win-cross] Support using a zip file for the Windows SDK


Created with:
  gclient setdep -r src/third_party/depot_tools@c5a26a769e69

The AutoRoll server is located here: https://depot-tools-chromium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.



BUG= chromium:852347 
TBR=agable@chromium.org

Change-Id: I3801e60259035c8ffd595364ae65795e7f371be3
Reviewed-on: https://chromium-review.googlesource.com/1111917
Reviewed-by: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#569606}
[modify] https://crrev.com/78adb83ddd5b3236bef406ca1404f7606f1d0342/DEPS

Project Member

Comment 12 by bugdroid1@chromium.org, Jun 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a5be472a0281780e3aa51eb4e916fdc6306a1cc2

commit a5be472a0281780e3aa51eb4e916fdc6306a1cc2
Author: Henrique Ferreiro <hferreiro@igalia.com>
Date: Mon Jun 25 11:49:45 2018

win_cross.md: update instructions for non-Googlers

Bug:  852347 
Change-Id: Ica3e0c748f05c0f7d1c714f006634de25b021f67
Reviewed-on: https://chromium-review.googlesource.com/1098257
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570010}
[modify] https://crrev.com/a5be472a0281780e3aa51eb4e916fdc6306a1cc2/docs/win_cross.md

Status: Fixed (was: Started)
Thanks, hferreiro!
Project Member

Comment 14 by bugdroid1@chromium.org, Jul 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37b2a549fa4876c8a12223cd3e967d331cff78ed

commit 37b2a549fa4876c8a12223cd3e967d331cff78ed
Author: Henrique Ferreiro <hferreiro@igalia.com>
Date: Mon Jul 30 12:19:54 2018

win_cross: fix reference to GYP_MSVS_HASH_<toolchain hash> variable

The documentation had a typo in the variable mapping the hashes.

Bug:  852347 
Change-Id: If69581b2bec94698208f7cf9f3d91b63e8048733
Reviewed-on: https://chromium-review.googlesource.com/1152908
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579018}
[modify] https://crrev.com/37b2a549fa4876c8a12223cd3e967d331cff78ed/docs/win_cross.md

Sign in to add a comment