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

Fix Windows build break in Terminal. #205

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

jeffdav
Copy link
Contributor

@jeffdav jeffdav commented Oct 20, 2024

These changes are now available in 4.15.1

There are three things that cause Terminal to not build on Windows:

  1. isprint() returns an Int32 which requires an explicit comparison. Easy fix: just compare against 0.
  2. _getch() returns an Int32, but Unicode.Scalar() takes a UInt32. So we must cast.
  3. The entire implementation of size was Posix-specific. Add a Windows implementation.

@jeffdav jeffdav requested review from 0xTim and gwynne as code owners October 20, 2024 22:58
@jeffdav
Copy link
Contributor Author

jeffdav commented Oct 20, 2024

Note: To build on Windows, first one must update swift-nio to this revision or later:
apple/swift-nio@8b66b22

@0xTim
Copy link
Member

0xTim commented Oct 20, 2024

@jeffdav Could you enable Windows CI so we can track this properly? See https://github.com/vapor/jwt-kit/blob/main/.github/workflows/test.yml#L35

@jeffdav
Copy link
Contributor Author

jeffdav commented Oct 21, 2024

Sure. But we may have to wait for swift-nio to ship a new minor revision.

@0xTim
Copy link
Member

0xTim commented Oct 21, 2024

Yeah I'm not expecting it to pass but we can at least track the errors across the modules and make sure we don't add regressions in new code. It will be a while until NIO ships a windows compatible version

@jeffdav
Copy link
Contributor Author

jeffdav commented Oct 21, 2024

@0xTim #206
I'm not super familiar with your workflows but it seems straightforward enough. LMK if this isn't what you meant.

BTW when I run swift test with these fixes + the swift-nio fixes everything does pass. I.e. with this locally:

    ],
    dependencies: [
        .package(url: "https://github.com/apple/swift-log.git", from: "1.5.3"),
        .package(url: "https://github.com/apple/swift-nio.git", revision: "8b66b22895115954dca2f1a6c5ab67172b6a6a1c"),
    ],

It looks like swift-nio does releases pretty frequently, so hopefully won't have to wait too long for it!

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 54.11%. Comparing base (a44d671) to head (bc3eee0).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
Sources/ConsoleKitTerminal/Terminal/Terminal.swift 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #205      +/-   ##
==========================================
- Coverage   54.18%   54.11%   -0.08%     
==========================================
  Files         103      103              
  Lines        4603     4609       +6     
==========================================
  Hits         2494     2494              
- Misses       2109     2115       +6     
Files with missing lines Coverage Δ
Sources/ConsoleKitTerminal/Terminal/Terminal.swift 21.87% <0.00%> (-1.46%) ⬇️

... and 1 file with indirect coverage changes

@jeffdav jeffdav force-pushed the jeff/fix-terminal-windows-build branch from e328439 to bc3eee0 Compare October 22, 2024 02:10
@0xTim
Copy link
Member

0xTim commented Oct 22, 2024

Ah yeah, we're only using NIOConcurrency helpers, not the rest of NIO. Pity we had to pull that in but those fixes should land soon!

@jeffdav
Copy link
Contributor Author

jeffdav commented Oct 23, 2024

SwiftNIO released 2.76.0 with the fixes in it. I believe a rerun of the windows-unit workflow will pass now.

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Thanks!

@0xTim 0xTim added the semver-patch When merged, a new patch version release will be generated label Oct 23, 2024
@0xTim 0xTim merged commit 966d89a into vapor:main Oct 23, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch When merged, a new patch version release will be generated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants