Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: use bash for Windows and build more targets in CI #8865

Merged
merged 4 commits into from
Nov 4, 2019
Merged

Conversation

lizan
Copy link
Member

@lizan lizan commented Nov 3, 2019

Signed-off-by: Lizan Zhou lizan@tetrate.io

Description:
Risk Level: Low
Testing:
Docs Changes:
Release Notes:

Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@mattklein123
Copy link
Member

cc @wrowe PTAL

@lizan
Copy link
Member Author

lizan commented Nov 4, 2019

@wrowe this addresses the issue you see in #8754 (comment), powershell by default treating command write to stderr as a failure so I just switch to bash (which is Windows native, not WSL, already available in azp) for a couple more reasons (leveraging existing scripts such as setting up caches).

@wrowe
Copy link
Contributor

wrowe commented Nov 4, 2019

Good stuff, thanks @lizan. This is a noop relative to the original correction to bazel/boringssl_static.patch in PR 8754 which is still needed. The proposal looks good - we only launch from powershell so can't comment on launching bazel from bash, but this is supposed to work well.

Editorial nit; missed a trailing newline in ci/windows_ci_steps.sh

@wrowe
Copy link
Contributor

wrowe commented Nov 4, 2019

switch to bash (which is Windows native, not WSL, already available in azp)

I'm rather confused, I searched azp and found only reference to the ubuntu (wsl) flavor of bash. Pointer please? This may be fine in any case, I'm not sure it has been evaluated by the bazel authors but I'm fine with beginning experimental use of it in place of msys2. But bash isn't the only linux tool required of msys2 for building bazel, nevermind envoy.

@wrowe
Copy link
Contributor

wrowe commented Nov 4, 2019

Last observation, the -fPIC change is good, but missed this change that's still needed to undo the recent breakage with pic/pie;

--- a/bazel/envoy_binary.bzl
+++ b/bazel/envoy_binary.bzl
@@ -71,6 +71,7 @@ def _envoy_linkopts():
         ],
     }) + select({
         "@envoy//bazel:boringssl_fips": [],
+        "@envoy//bazel:windows_x86_64": [],
         "//conditions:default": ["-pie"],
     }) + _envoy_select_exported_symbols(["-Wl,-E"])

@wrowe
Copy link
Contributor

wrowe commented Nov 4, 2019

Re: pie/pic on Windows;

https://docs.microsoft.com/en-us/cpp/build/reference/dynamicbase-use-address-space-layout-randomization?view=vs-2019

confirms that ASLR-compatible .exe objects are the default for Windows (this was obviously true of .dll files, but also true of static .exe's by default since at least as far back as Visual Studio 2015.)

@lizan
Copy link
Member Author

lizan commented Nov 4, 2019

switch to bash (which is Windows native, not WSL, already available in azp)

I'm rather confused, I searched azp and found only reference to the ubuntu (wsl) flavor of bash. Pointer please? This may be fine in any case, I'm not sure it has been evaluated by the bazel authors but I'm fine with beginning experimental use of it in place of msys2. But bash isn't the only linux tool required of msys2 for building bazel, nevermind envoy.

From azp log:

[command]"C:\Program Files\Git\bin\bash.exe" --noprofile --norc -c pwd
/d/a/_temp

========================== Starting Command Output ===========================
[command]"C:\Program Files\Git\bin\bash.exe" --noprofile --norc /d/a/_temp/0c683591-eb2e-4fdc-a52f-de48257c41a6.sh
disk space at beginning of build:
Filesystem            Size  Used Avail Use% Mounted on
C:/Program Files/Git  128G  110G   19G  86% /
D:                     14G  1.9G   13G  14% /d
No remote cache bucket is set, skipping setup remote cache.
2019/11/03 06:42:37 Downloading https://releases.bazel.build/1.1.0/release/bazel-1.1.0-windows-x86_64.exe...

Seems azp agent is using bash under git installation, and from what bazelisk downloads it is obviously using native Windows bazel. (We don't install Linux binary of bazelisk anywhere either).

I'll update pie soon

Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan lizan merged commit a3bc473 into master Nov 4, 2019
@lizan lizan deleted the windows_ci branch November 4, 2019 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants