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.

Sign up for GitHub

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

Try to not reinstall tools in mingw CI #125546

Merged
merged 1 commit into from
May 26, 2024
Merged

Try to not reinstall tools in mingw CI #125546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions src/ci/scripts/install-mingw.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ source "$(cd "$(dirname "$0")" && pwd)/../shared.sh"
MINGW_ARCHIVE_32="i686-12.2.0-release-posix-dwarf-rt_v10-rev0.7z"
MINGW_ARCHIVE_64="x86_64-12.2.0-release-posix-seh-rt_v10-rev0.7z"

if isWindows; then
if isWindows && isKnownToBeMingwBuild; then
case "${CI_JOB_NAME}" in
*i686*)
bits=32
Expand All @@ -39,10 +39,7 @@ if isWindows; then
esac

if [[ "${CUSTOM_MINGW:-0}" == 0 ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is just a dead code so a followup PR could get rid of all CUSTOM_MINGW occurrences.

pacboy -S --noconfirm toolchain:p
# According to the comment in the Windows part of install-clang.sh, in the future we might
# want to do this instead:
# pacboy -S --noconfirm clang:p ...
pacman -S --noconfirm --needed mingw-w64-$arch-toolchain
else
mingw_dir="mingw${bits}"

Expand Down
38 changes: 2 additions & 36 deletions src/ci/scripts/install-msys2.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/bin/bash
# Clean up and prepare the MSYS2 installation. MSYS2 is needed primarily for
# the test suite (run-make), but is also used by the MinGW toolchain for assembling things.
# Clean up and prepare the MSYS2 installation.
# MSYS2 is used by the MinGW toolchain for assembling things.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see MSYS2 anywhere in this script apart from the comment and filename, probably could be renamed to setup-windows-python.sh or something like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm not convinced this script is still needed at all but I didn't want to change too much at once. Or if it is just needed by the mingw targets then it could be merged into the other script.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think all Windows jobs need Python setup one way or another and after these changes this script is basically a custom actions/setup-python step: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python#specifying-a-python-version

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, that says "To maintain consistent behavior with other runners and to allow Python to be used out-of-the-box without the setup-python action, GitHub adds a few versions from the tools cache to PATH"

Consistent behaviour with other runners sounds like something we can rely on, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

We use custom Docker images on Linux builders, no idea about Mac. So this consistency won't really apply here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. I think I'll experiment with maybe using the action in a follow up PR.


set -euo pipefail
IFS=$'\n\t'
Expand All @@ -24,38 +24,4 @@ if isWindows; then
fi
ciCommandAddPath "C:\\hostedtoolcache\\windows\\Python\\${native_python_version}\\x64"
ciCommandAddPath "C:\\hostedtoolcache\\windows\\Python\\${native_python_version}\\x64\\Scripts"

# Install pacboy for easily installing packages
pacman -S --noconfirm pactoys

# Remove these pre-installed tools so we can't accidentally use them, because we are using the
# MSYS2 setup action versions instead. Because `rm -r`-ing them is slow, we mv them off path
# instead.
# Remove pre-installed version of MSYS2
echo "Cleaning up existing tools in PATH"
notpath="/c/NOT/ON/PATH/"
mkdir --parents "$notpath"
mv -t "$notpath" "/c/msys64/"
# Remove Strawberry Perl, which contains a version of mingw
mv -t "$notpath" "/c/Strawberry/"
# Remove these other copies of mingw, I don't even know where they come from.
mv -t "$notpath" "/c/mingw64/"
mv -t "$notpath" "/c/mingw32/"
echo "Finished cleaning up tools in PATH"

if isKnownToBeMingwBuild; then
# Use the mingw version of CMake for mingw builds.
# However, the MSVC build needs native CMake, as it fails with the mingw one.
# Delete native CMake
rm -r "/c/Program Files/CMake/"
# Install mingw-w64-$arch-cmake
pacboy -S --noconfirm cmake:p

# It would be nice to use MSYS's git in MinGW builds so that it's tested and known to
# work. But it makes everything extremely slow, so it's commented out for now.
# # Delete Windows-Git
# rm -r "/c/Program Files/Git/"
# # Install MSYS2 git
# pacman -S --noconfirm git
fi
fi
Loading