Skip to content

Uninstall mingw before attempting upgrade#9288

Merged
estesp merged 1 commit intocontainerd:mainfrom
kiashok:updateCI-main
Jan 9, 2024
Merged

Uninstall mingw before attempting upgrade#9288
estesp merged 1 commit intocontainerd:mainfrom
kiashok:updateCI-main

Conversation

@kiashok
Copy link
Copy Markdown
Contributor

@kiashok kiashok commented Oct 23, 2023

Update mingw after uninstall, similar to what is done in hccshimhttps://github.com/microsoft/hcsshim/blob/2feaacb46cf42e17ab81bfe6341d3529ef4cb897/.github/workflows/ci.yml#L421

@k8s-ci-robot
Copy link
Copy Markdown

Hi @kiashok. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kiashok kiashok requested a review from fuweid November 1, 2023 19:58
@kiashok kiashok marked this pull request as ready for review November 1, 2023 19:58
@kiashok
Copy link
Copy Markdown
Contributor Author

kiashok commented Nov 1, 2023

cc @helsaawy @dmcgowan

@kiashok kiashok force-pushed the updateCI-main branch 3 times, most recently from d2ddcf3 to 8afd392 Compare November 1, 2023 20:44
@kiashok kiashok force-pushed the updateCI-main branch 6 times, most recently from 2de3a7a to fc4c78d Compare November 3, 2023 01:42
@estesp
Copy link
Copy Markdown
Member

estesp commented Nov 3, 2023

Looks like a whitespace check failure in the commit git show --check will tell you, but looks like extra spaces on one of the lines after "shell: powershell" in your comment

Copy link
Copy Markdown
Member

@kzys kzys left a comment

Choose a reason for hiding this comment

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

LGTM. Re-running the failed tests.

@kiashok
Copy link
Copy Markdown
Contributor Author

kiashok commented Nov 9, 2023

LGTM. Re-running the failed tests.

The tests are failing due to some mingw + golang version 1.20.3 issue that is causing some dll to not load. Looking at what the other options for us with WS2019 is. Also opened an issue for this in microsoft/go
microsoft/go#1081

image

@dagood
Copy link
Copy Markdown

dagood commented Dec 12, 2023

I looked into this in more detail at microsoft/go#1081, but here's a summary:

  • Updating MinGW is the right thing to do. The current build used in the win2019 image is from 2018 and the provider of this build has been removed from the list maintained at https://www.mingw-w64.org/downloads/.
  • This PR isn't working because the Chocolatey installation isn't actually being used. The win2019 image has the outdated build's PATH entry before Chocolatey's PATH entry, so the outdated build is found regardless of what Chocolatey package is installed.

I see a couple ways forward in the short term:

  • In addition to the Chocolatey version change, manipulate PATH to remove the outdated build, or delete the outdated build at C:\mingw64\bin entirely.
  • Stop depending on Chocolatey and install MinGW yourself.
    • This involves downloading a 7z, verifying its checksum, extracting it (7z is installed on this image already), and prepending it to PATH.
    • https://github.com/niXman/mingw-builds-binaries/releases is the build provider used by GitHub Actions to build the win2022 image, and it works fine in my tests.

@dagood
Copy link
Copy Markdown

dagood commented Dec 14, 2023

I filed an issue upstream to update MinGW in the image, and they think it's too risky, but provided a ready-made example of installing a newer MinGW in a workflow that you can use: actions/runner-images#9009 (comment).

@k8s-ci-robot k8s-ci-robot added size/M and removed size/L labels Jan 8, 2024
@kiashok kiashok force-pushed the updateCI-main branch 5 times, most recently from d275c76 to 318db09 Compare January 9, 2024 06:00
@kiashok kiashok marked this pull request as ready for review January 9, 2024 06:01
@kiashok
Copy link
Copy Markdown
Contributor Author

kiashok commented Jan 9, 2024

@jsturtevant @helsaawy @dagood @fuweid all the tests are passing. Could you please take a look?

@jsturtevant
Copy link
Copy Markdown
Contributor

LGTM

Copy link
Copy Markdown
Contributor

@helsaawy helsaawy left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM, but added one question on what appears to be an empty step (that may be required) but want to make sure I understand

Signed-off-by: Kirtana Ashok <kiashok@microsoft.com>
@estesp estesp added this pull request to the merge queue Jan 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2024
@estesp estesp added this pull request to the merge queue Jan 9, 2024
Merged via the queue into containerd:main with commit dd7f7bd Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants