Skip to content

Fix building on Windows#51

Merged
milseman merged 2 commits into
apple:mainfrom
stevapple:windows-build
Aug 5, 2021
Merged

Fix building on Windows#51
milseman merged 2 commits into
apple:mainfrom
stevapple:windows-build

Conversation

@stevapple

@stevapple stevapple commented Jun 8, 2021

Copy link
Copy Markdown
Contributor

Nothing but as shown in the title.

The base for swiftlang/swift-tools-support-core#219, swiftlang/swift-tools-support-core#221

@milseman

milseman commented Jun 17, 2021

Copy link
Copy Markdown
Contributor

@compnerd

@milseman milseman requested a review from compnerd June 17, 2021 12:52
milseman
milseman reviewed Jun 17, 2021
View reviewed changes

@milseman milseman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. @compnerd, what do you think?

Comment thread Package.swift Outdated
var windowsPlatform: [Platform] = []
#if os(Windows)
windowsPlatform.append(.windows)
#endif

@milseman milseman Jun 17, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not that familiar with manifests. Why is conditional compilation needed here?

@stevapple stevapple Jun 17, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🤔 AFAIK this is used to comfort SwiftPM 5.3 because it doesn't recognize Windows. This workaround is introduced by @compnerd for other projects and I just use it here.

@compnerd compnerd Jun 17, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think that this would be needed given the comment below.

compnerd
compnerd reviewed Jun 17, 2021
View reviewed changes
Comment thread Package.swift Outdated
var windowsPlatform: [Platform] = []
#if os(Windows)
windowsPlatform.append(.windows)
#endif

@compnerd compnerd Jun 17, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think that this would be needed given the comment below.

Comment thread Package.swift Outdated
dependencies: ["CSystem"],
path: "Sources/System",
cSettings: [
.define("_CRT_SECURE_NO_WARNINGS", .when(platforms: windowsPlatform)),

@compnerd compnerd Jun 17, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We can do this unconditionally, it won't impact other targets. Alternatively, we could clean up the cases to use the Microsoft "extensions" (technically I believe that they are part of Annex G, so they should be portable).

@stevapple stevapple Jun 18, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable. Fixed

milseman
milseman approved these changes Jun 18, 2021
View reviewed changes

@milseman milseman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@milseman

milseman commented Jun 18, 2021

Copy link
Copy Markdown
Contributor

@swift-ci please test

@milseman

milseman commented Jun 22, 2021

Copy link
Copy Markdown
Contributor

@swift-ci please test

@karwa

karwa commented Aug 3, 2021

Copy link
Copy Markdown

Can confirm that the Windows build is broken, and that this patch fixes it. This is a blocking issue for me; is there anything you need before this can be merged @milseman? Also, once merged, it would be great if you could tag a new release so libraries which depend on swift-system can pick up the fix.

There are still some deprecation warnings (included below), which should probably be addressed at some point, but at least this patch gets things building again.

C:\Users\User\Documents\Code\swift-url\.build\checkouts\swift-system\Sources\System\Internals\Syscalls.swift:82:10: warning: 'lseek' is deprecated: The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _lseek. See online help for details.
  return lseek(fd, off, whence)
         ^
C:\Users\User\Documents\Code\swift-url\.build\checkouts\swift-system\Sources\System\Internals\Syscalls.swift:109:10: warning: 'dup' is deprecated: The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _dup. See online help for details.
  return dup(fd)
         ^
C:\Users\User\Documents\Code\swift-url\.build\checkouts\swift-system\Sources\System\Internals\Syscalls.swift:116:10: warning: 'dup2' is deprecated: The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _dup2. See online help for details.
  return dup2(fd, fd2)
         ^

@milseman

milseman commented Aug 5, 2021

Copy link
Copy Markdown
Contributor

My bad!

@milseman milseman merged commit 39774ef into apple:main Aug 5, 2021
@milseman

milseman commented Aug 5, 2021

Copy link
Copy Markdown
Contributor

@karwa I made a draft 0.0.3 release here: https://github.com/apple/swift-system/releases/tag/untagged-cb0bff1f58c393ba2621

Are you able to try it out before I publish it? The release process is still a little awkward as we're preparing for a proper 1.0 soon.

@stevapple

stevapple commented Aug 5, 2021

Copy link
Copy Markdown
Contributor Author

@milseman Although this is off-topic here, could I expect using both POSIX and Windows path on either platform as for 1.0 release? This useful for cross-platform things or simply using POSIX path style on Windows (eg. in-memory file system in SwiftPM).

@milseman

milseman commented Aug 5, 2021

Copy link
Copy Markdown
Contributor

Unfortunately not. While we can support that technology internally, we don't have a good way to surface that as API yet. I'm hoping that task-local values gives us a way of establishing a contextual platform path type. We can also explore a FilePathProtocol and have a family of various concrete conformers for explicitly picking one over the other, but there are many many design axis there and that would take more consideration than we can do before 1.0.

I'd be interested in your thoughts on how you'd want to use such an ability though.

@karwa

karwa commented Aug 30, 2021

Copy link
Copy Markdown

@milseman Apologies for the delay. I'm not sure how to get the link you posted to work, but I switched the dependency to the main branch, and it does work.

The README currently suggests using upToNextMinor for source-stability reasons, and anybody who is following that advice will not automatically get 1.0, so I think it may still make sense to tag a 0.0.3 release regardless.

That being said, probably all packages which use swift-system as a dependency should release new versions which bump their requirements to 1.0 anyway. So I don't know ⚖️

@milseman

milseman commented Aug 30, 2021

Copy link
Copy Markdown
Contributor

Published 0.0.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@compnerd compnerd compnerd approved these changes
@milseman milseman milseman approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@stevapple @milseman @karwa @compnerd