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.

Sign up for GitHub

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

Jump to bottom

Fix building on Windows #51

Merged
merged 2 commits into from
Aug 5, 2021
Merged

Fix building on Windows #51

merged 2 commits into from
Aug 5, 2021

Conversation

stevapple
Copy link
Contributor

Nothing but as shown in the title.

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

@milseman
Copy link
Contributor

@compnerd

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

@milseman milseman left a comment

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?

Package.swift Outdated
var windowsPlatform: [Platform] = []
#if os(Windows)
windowsPlatform.append(.windows)
#endif
Copy link
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?

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

Copy link
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
Package.swift Outdated
var windowsPlatform: [Platform] = []
#if os(Windows)
windowsPlatform.append(.windows)
#endif
Copy link
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.

Package.swift Outdated
.target(
name: "SystemPackage",
dependencies: ["CSystem"],
path: "Sources/System",
cSettings: [
.define("_CRT_SECURE_NO_WARNINGS", .when(platforms: windowsPlatform)),
Copy link
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).

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

@milseman milseman left a comment

Choose a reason for hiding this comment

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

LGTM

@milseman
Copy link
Contributor

@swift-ci please test

@milseman
Copy link
Contributor

@swift-ci please test

@karwa
Copy link

karwa commented Aug 3, 2021

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

milseman commented Aug 5, 2021

My bad!

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

milseman commented Aug 5, 2021

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

milseman commented Aug 5, 2021

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

karwa commented Aug 30, 2021

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

Published 0.0.3

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

@milseman milseman milseman approved these changes

@compnerd compnerd compnerd 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