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

Prefer "#if os(Linux)" to "#if !os(macOS) #1826

Merged
merged 1 commit into from Oct 23, 2018

Conversation

stephencelis
Copy link
Contributor

Currently, projects that use --generate-linuxmain will generate a file that breaks iOS and other platforms.

There may be more things that need to be fixed here, but this is a start. I noticed that throughout this project things are cased on os(macOS) a lot. Perhaps an audit is a good idea?

@SDGGiesbrecht
Copy link
Contributor

This has been annoying me too for some time, but I haven’t gotten around to submitting a fix myself.

In this specific case (the generated test manifests), I think #if !canImport(ObjectiveC) is what we really want, since it won’t break Windows, Android, etc. like #if os(Linux) would.

@stephencelis
Copy link
Contributor Author

@SDGGiesbrecht Good idea, thanks!

@SDGGiesbrecht
Copy link
Contributor

@aciidb0mb3r

@ankitspd
Copy link
Member

Thanks! Can you also run .build/x86_64-apple-macosx10.10/debug/spm test --generate-linuxmain to update SwiftPM's generated files?

@stephencelis
Copy link
Contributor Author

@aciidb0mb3r What are the steps to getting there? I've built spm per the docs and there's no .build folder. I then ran swift build and the .build/x86_64-apple-macosx10.10/debug directory exists, but no spm executable. When I run .build/x86_64-apple-macosx10.10/debug/swift-test --generate-linuxmain, I get:

error: toolchain is invalid: could not find the swiftc at expected path /Users/stephencelis/Developer/apple/swiftpm/.build/x86_64-apple-macosx10.10/debug/swiftc

@ankitspd
Copy link
Member

How did you build swiftpm? Don't run swift build directly. If you have the latest Xcode, Utilities/bootstrap should be enough to build it.

@stephencelis
Copy link
Contributor Author

I followed the directions pointed to here:

https://github.com/apple/swift-package-manager/blob/master/Documentation/Development.md

Basically, ran ../swift/utils/build-script -R --llbuild --swiftpm.

@ankitspd
Copy link
Member

Ah. That puts the build directory next to your checkout of SwiftPM. However, I would recommend following the second method unless you really require a compiler built from scratch: https://github.com/apple/swift-package-manager/blob/master/Documentation/Development.md#using-trunk-snapshot

@stephencelis
Copy link
Contributor Author

Done! Probably would have been faster to manually update :) Took awhile to attempt to build with swift as a whole.

@millenomi
Copy link
Contributor

To avoid making things harder for non-Linux porters, I’ve been slowly transitioning Foundation to #if canImport(Darwin) rather than using os with Apple OS names.

@stephencelis
Copy link
Contributor Author

Should I change to #if !canImport(Darwin) instead? The ObjectiveC check seemed appropriate since the runtime is what's used to find all the test cases.

@ankitspd
Copy link
Member

Done! Probably would have been faster to manually update :) Took awhile to attempt to build with swift as a whole.

I should update the dev docs. Given the source compatibility, compiling from scratch is rarely required.

@ankitspd
Copy link
Member

I agree, ObjectiveC check seems more appropriate but Darwin should also work. Let me see if I can quickly check with someone from the Swift team.

@ankitspd
Copy link
Member

I haven't heard back yet. Lets get this in, I'll update the code if I hear back about a preferred way.

@ankitspd
Copy link
Member

@swift-ci smoke test

@ankitspd
Copy link
Member

Failure seems related, I'll update the patch.

Currently, projects that use `--generate-linuxmain` will generate a file that breaks iOS and other platforms.
@ankitspd
Copy link
Member

@swift-ci smoke test

@ankitspd ankitspd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Oct 23, 2018
@ankitspd ankitspd merged commit 14e5e60 into apple:master Oct 23, 2018
CodaFi added a commit to CodaFi/swift that referenced this pull request Jul 1, 2020
Clean up a few general patterns that are now obviated by canImport

This aligns more generally with the cleanup that the Swift Package
Manager has already done in their automated XCTest-plumbing tool in
apple/swift-package-manager#1826.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Author believes the PR is ready to be merged & any feedback has been addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants