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

SDK: add SDK overlay for Windows #20906

Merged
merged 1 commit into from
Dec 1, 2018
Merged

Conversation

compnerd
Copy link
Collaborator

Introduce a WinSDK overlay for Windows. This allows us to define some
shared constants that are not correctly imported right now. This cleans
up the logic in the swift side of things and aids in the bring up.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

@compnerd
Copy link
Collaborator Author

CC: @jrose-apple @gottesmm

I figure you may have some words on the location of the file. Thanks to @gottesmm for reminding me to do the right thing and make things feel more natural. This is going to enable a bunch of additional tests.

@compnerd
Copy link
Collaborator Author

@swift-ci please test

@swift-ci
Copy link
Collaborator

Build failed
Swift Test Linux Platform
Git Sha - cfb64a38b3bf53ed6ace4691f48fe36d47c2c1e3

@compnerd
Copy link
Collaborator Author

@swift-ci please test Linux platform

public let FALSE: BOOL = 0

// minwindef.h
public let TRUE: BOOL = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh boy, Windows has a BOOL too? Do we want to pull an ObjCBool on it eventually? (WindowsBoolean?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The world of the 90s was a terrible place. Yes, unfortunately, I think that we will want to pull an ObjCBool on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

At least we've done it twice already (ObjCBool and DarwinBoolean), which means we're past the ZOM threshold.

Copy link
Contributor

Choose a reason for hiding this comment

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

WindowsBool? WinBool?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

WindowsBool sounds nice to me.

stdlib/public/SDK/Windows/WinSDK.swift Outdated Show resolved Hide resolved
@@ -72,3 +72,8 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
add_subdirectory(SDK)
endif()
endif()

if(WINDOWS IN_LIST SWIFT_SDKS)
add_subdirectory(SDK/Windows)
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 gonna be obstinate and say "SDK/" really means "Apple/" at this point. Maybe "WinSDK/" or "Windows/" instead of "SDK/Windows/"? cc @moiseev @lancep

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lol, thats why I wanted you to review this :) This is getting into the territory of difficult things ... naming. How about changing the structure to SDK/Darwin and SDK/Windows? Or Apple/ and Microsoft/?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think "OS family" is the right grouping here, so "SDK/Darwin/" and "SDK/Windows/" is closer to the right thing, but then I don't think the SDK/ subfolder is buying us anything. I personally don't like using "Darwin" to mean "Apple platforms" these days because it's Yet Another Term and also doesn't line up with the LLVM "x86_64-apple-darwin" triple that only means macOS (Mac OS X) and not iOS or anything else. So my names would be "Apple/" and "Windows/", but "Darwin/" and "Windows/" would be okay too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW – The only other name for "Darwin" that covers all of Apple's operating systems is "xnu" (the kernel). It is near impossible to build an xnu-based platform without dragging along a ton of "Darwin" frameworks and daemons due to various reentrant dependancies.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@davezarzycki - haha, perhaps we should call it "XNU/Darwin" (mirroring "GNU/Linux") :-p I'm going to go with Darwin for now I think.

@compnerd
Copy link
Collaborator Author

CC: @davezarzycki, not sure if you care, but this is probably going to be fun!

@davezarzycki
Copy link
Collaborator

Hi @compnerd – I'm not concerned about the SDK overlays at the moment, but thanks for the heads up.

@compnerd
Copy link
Collaborator Author

@swift-ci please test

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - cfb64a38b3bf53ed6ace4691f48fe36d47c2c1e3

@compnerd
Copy link
Collaborator Author

@swift-ci please test

@swift-ci
Copy link
Collaborator

Build failed
Swift Test Linux Platform
Git Sha - aec17be3b3963d0d7c613eab2c26cd3f192746f0

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - aec17be3b3963d0d7c613eab2c26cd3f192746f0

@jrose-apple
Copy link
Contributor

cc also @Rostepher, since we build things directly from stdlib/public/SDK/. He may want you to hold off on renaming that to Apple/ just now.

Introduce a WinSDK overlay for Windows.  This allows us to define some
shared constants that are not correctly imported right now.  This cleans
up the logic in the swift side of things and aids in the bring up.

Now that we have a SDK overlay for Windows, we should structure the tree
according to the OS family.
@compnerd
Copy link
Collaborator Author

compnerd commented Dec 1, 2018

Split out the SDK rename, I'll upload a separate change for that.

@compnerd
Copy link
Collaborator Author

compnerd commented Dec 1, 2018

@swift-ci please test and merge

@gottesmm
Copy link
Member

gottesmm commented Dec 1, 2018

(test and merge hit the force push issue... just making it run).

@gottesmm
Copy link
Member

gottesmm commented Dec 1, 2018

@swift-ci please test and merge

1 similar comment
@gottesmm
Copy link
Member

gottesmm commented Dec 1, 2018

@swift-ci please test and merge

@swift-ci swift-ci merged commit 19eed12 into apple:master Dec 1, 2018
@compnerd compnerd deleted the sdk-overlay branch December 1, 2018 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants