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

bazel: unify msys #16394

Merged
merged 1 commit into from
May 12, 2021
Merged

bazel: unify msys #16394

merged 1 commit into from
May 12, 2021

Conversation

daixiang0
Copy link
Member

@daixiang0 daixiang0 commented May 8, 2021

Signed-off-by: Long Dai [email protected]

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message:

msys32 and msys64 both occur in examples which may make confused, unify them.

Additional Description:
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Long Dai <[email protected]>
@repokitteh-read-only
Copy link

Hi @daixiang0, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #16394 was opened by daixiang0.

see: more, trace.

@antoniovicente
Copy link
Contributor

cc @envoyproxy/windows-dev

Copy link
Member

@davinci26 davinci26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for improving the docs, LGTM.

@daixiang0
Copy link
Member Author

ping @envoyproxy/windows-dev

Copy link
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth comparing this to the docker image we provide... that logic is;

# msys2 with additional required packages
DownloadAndCheck $env:TEMP\msys2.tar.xz https://github.com/msys2/msys2-installer/releases/download/2021-02-28/msys2-base-x86_64-20210228.tar.xz 3f2ceb097a081789d9d497e0d3df8d99c16a1591b9984b0469440cd5bfa65092
RunAndCheckError "$env:TEMP\7z\7z.exe" @("x", "$env:TEMP\msys2.tar.xz", "-o$env:TEMP\msys2.tar", "-y")
RunAndCheckError "$env:TEMP\7z\7z.exe" @("x", "$env:TEMP\msys2.tar", "-oC:\tools", "-y")
AddToPath C:\tools\msys64\usr\bin
# To ensure msys2 link.exe (GNU link) does not conflict with link.exe from VC Build Tools
mv -Force C:\tools\msys64\usr\bin\link.exe C:\tools\msys64\usr\bin\gnu-link.exe
RunAndCheckError "C:\tools\msys64\usr\bin\bash.exe" @("-c", "pacman-key --init")
RunAndCheckError "C:\tools\msys64\usr\bin\bash.exe" @("-c", "pacman-key --populate msys2")
# Force update of package db
RunAndCheckError "pacman.exe" @("-Syy", "--noconfirm")
# TODO(sunjayBhatia, wrowe): pacman core package update causes building with latest
# Docker to hang between completion of this script and before discarding intermediate
# build container (that is reported as exited). Skipping the existing package updates
# for now until we have a resolution.
# Docker version running in AZP at last check: 19.03.5
# Update core packages (msys2, pacman, bash, etc.)
# RunAndCheckError "pacman.exe" @("-Suu", "--noconfirm")
# Update remaining packages (and package db refresh in case previous step requires it)
# RunAndCheckError "pacman.exe" @("-Syu", "--noconfirm")
RunAndCheckError "pacman.exe" @("-S", "--noconfirm", "--needed", "git", "subversion", "diffutils", "patch", "unzip", "zip")
RunAndCheckError "pacman.exe" @("-Scc", "--noconfirm")

I see this msys2.tz image does expand into msys64/ paths, not msys2, and agree with your change. A question is, have we missed other oddities like link.exe (why did msys2 project not name this ld?) that we compensate for in our toolchain assembly? In any case, LGTM.

@wrowe wrowe merged commit 955f15d into envoyproxy:main May 12, 2021
@daixiang0 daixiang0 deleted the unify-msys branch May 13, 2021 02:02
@daixiang0
Copy link
Member Author

@wrowe hi, I am new to develop on Windows platform, so still explore it, very glad to talk with you in slack!

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.

4 participants