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: remove mingw job #18580

Merged
merged 3 commits into from May 15, 2022
Merged

ci: remove mingw job #18580

merged 3 commits into from May 15, 2022

Conversation

dundargoc
Copy link
Member

The mingw CI job takes a long time to finish and the benefit it provides
is relatively low. A more thorough rationale and discussion can be found
in the provided link.

Closes #18551

The mingw CI job takes a long time to finish and the benefit it provides
is relatively low. A more thorough rationale and discussion can be found
in the provided link.

Closes neovim#18551
@github-actions github-actions bot added the ci automation for build, test, and release label May 15, 2022
@github-actions github-actions bot requested a review from jamessan May 15, 2022 15:52
Copy link
Member

@justinmk justinmk left a comment

Choose a reason for hiding this comment

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

is there build.ps1 or other script code that can be simplified ? We probably can't continue to support the msys stuff in build.ps1 if we aren't checking it in CI.

@dundargoc dundargoc marked this pull request as draft May 15, 2022 16:06
@github-actions github-actions bot removed the request for review from jamessan May 15, 2022 16:07
@dundargoc dundargoc marked this pull request as ready for review May 15, 2022 16:29
@github-actions github-actions bot requested a review from jamessan May 15, 2022 16:29
@dundargoc dundargoc requested a review from justinmk May 15, 2022 16:29
ci/build.ps1 Outdated Show resolved Hide resolved
@@ -76,57 +76,6 @@ if(UNIX)
CC=${DEPS_C_COMPILER} PREFIX=${DEPS_INSTALL_DIR}
${DEPLOYMENT_TARGET})

elseif(MINGW AND CMAKE_CROSSCOMPILING)
Copy link
Member

Choose a reason for hiding this comment

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

This we can definitely get rid of. Not sure if we need crosscompiling stuff at all.

Copy link
Member

@justinmk justinmk left a comment

Choose a reason for hiding this comment

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

Well, let's go ahead with this as-is and see if anyone cares about the mingw support. We can surgically restore some stuff later if needed.

@justinmk justinmk merged commit f8af814 into neovim:master May 15, 2022
@github-actions github-actions bot removed the request for review from jamessan May 15, 2022 23:00
@justinmk
Copy link
Member

Does CI still use msys for running oldtests?

neovim/ci/build.ps1

Lines 121 to 125 in e501e4e

# Add MSYS to path, required for e.g. `find` used in test scripts.
# But would break functionaltests, where its `more` would be used then.
$OldPath = $env:PATH
$env:PATH = "C:\msys64\usr\bin;$env:PATH"
& "C:\msys64\mingw$bits\bin\mingw32-make.exe" -C $(Convert-Path ..\src\nvim\testdir) VERBOSE=1 ; exitIfFailed

@dundargoc
Copy link
Member Author

Honestly, I wasn't sure so I just left is as I didn't want to break something acidentally.

Shougo pushed a commit to Shougo/neovim that referenced this pull request May 17, 2022
Unnecessary CI builds increase the change of spurious failures, which are costly
noise. Of course, we should fix all legitimate bugs, but we also cannot
micro-manage every platform, so there needs to be a clear motivation for the CI
builds that we maintain.

Reasons against maintaining a mingw CI job:
1. The windows mingw build is slow.
2. Failures:
    - neovim#18494
    - neovim#18495
3. The mingw artifact is 10x bigger than the windows MSVC artifact:
   neovim#10560
4. Our releases publish the MSVC (not mingw) artifact for Windows users:
   https://github.com/neovim/neovim/releases
5. Non-MSVCRT has limitations documented by libuv: http://docs.libuv.org/en/v1.x/process.html
   > On Windows file descriptors greater than 2 are available to the child process only if the child processes uses the MSVCRT runtime.

Closes neovim#18551
@dundargoc dundargoc deleted the ci/remove-mingw branch May 17, 2022 08:14
dundargoc added a commit to dundargoc/neovim that referenced this pull request May 17, 2022
This partially reverts commit f8af814.

The mingw parts of cmake was removed to see if it was still used
(ref: neovim#18580). It turns out it is,
so this will fix that.

Closes: neovim#18597
dundargoc added a commit to dundargoc/neovim that referenced this pull request May 17, 2022
This partially reverts commit f8af814.

The mingw parts of cmake was removed to see if it was still used
(ref: neovim#18580). It turns out it is,
so this will fix that.

Closes: neovim#18597
dmitmel pushed a commit to dmitmel/neovim that referenced this pull request May 18, 2022
Unnecessary CI builds increase the change of spurious failures, which are costly
noise. Of course, we should fix all legitimate bugs, but we also cannot
micro-manage every platform, so there needs to be a clear motivation for the CI
builds that we maintain.

Reasons against maintaining a mingw CI job:
1. The windows mingw build is slow.
2. Failures:
    - neovim#18494
    - neovim#18495
3. The mingw artifact is 10x bigger than the windows MSVC artifact:
   neovim#10560
4. Our releases publish the MSVC (not mingw) artifact for Windows users:
   https://github.com/neovim/neovim/releases
5. Non-MSVCRT has limitations documented by libuv: http://docs.libuv.org/en/v1.x/process.html
   > On Windows file descriptors greater than 2 are available to the child process only if the child processes uses the MSVCRT runtime.

Closes neovim#18551
kraftwerk28 pushed a commit to kraftwerk28/neovim that referenced this pull request Jun 1, 2022
Unnecessary CI builds increase the change of spurious failures, which are costly
noise. Of course, we should fix all legitimate bugs, but we also cannot
micro-manage every platform, so there needs to be a clear motivation for the CI
builds that we maintain.

Reasons against maintaining a mingw CI job:
1. The windows mingw build is slow.
2. Failures:
    - neovim#18494
    - neovim#18495
3. The mingw artifact is 10x bigger than the windows MSVC artifact:
   neovim#10560
4. Our releases publish the MSVC (not mingw) artifact for Windows users:
   https://github.com/neovim/neovim/releases
5. Non-MSVCRT has limitations documented by libuv: http://docs.libuv.org/en/v1.x/process.html
   > On Windows file descriptors greater than 2 are available to the child process only if the child processes uses the MSVCRT runtime.

Closes neovim#18551
kraftwerk28 pushed a commit to kraftwerk28/neovim that referenced this pull request Jun 1, 2022
This partially reverts commit f8af814.

The mingw parts of cmake was removed to see if it was still used
(ref: neovim#18580). It turns out it is,
so this will fix that.

Closes: neovim#18597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci automation for build, test, and release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows CI: do we need msys2 / mingw ?
2 participants