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

Uninstall mingw before attempting upgrade #9288

Merged
merged 1 commit into from Jan 9, 2024

Conversation

kiashok
Copy link
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

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.

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
Copy link
Contributor Author

kiashok commented Nov 1, 2023

cc @helsaawy @dmcgowan

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@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
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
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
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

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

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
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
Contributor

LGTM

Copy link
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

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
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
46 checks passed
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.

None yet

9 participants