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

Use internal import instead of implementationOnly import #5109

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

parkera
Copy link
Contributor

@parkera parkera commented Oct 14, 2024

Use internal import CoreFoundation instead of @_implementationOnly import CoreFoundation. Resolves #5108

@parkera
Copy link
Contributor Author

parkera commented Oct 14, 2024

@swift-ci test

@parkera
Copy link
Contributor Author

parkera commented Oct 18, 2024

/home/build-user/swift-corelibs-foundation/Sources/plutil/main.swift:13:8: error: missing required module 'CoreFoundation'
 11 | import SwiftFoundation
 12 | #elseif canImport(Glibc)
 13 | import Foundation
    |        `- error: missing required module 'CoreFoundation'
 14 | import Glibc
 15 | #elseif canImport(Musl)

Looks like we have something else to sort out here...

@clackary
Copy link

Can I also make the request to get this cherrypicked to 6.0 (and even 6.0.2) if possible?

@parkera
Copy link
Contributor Author

parkera commented Oct 18, 2024

cc @xymus

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

I think that this will break Foundation on Windows. the @_implementationOnly import does not require the client to see the CoreFoundation module, while internal import does. We do not ship the CoreFoundation interface (headers) on Windows as per the request from the initial port. If we need to start shipping them, we need to ensure that the driver, installer, and SPM are updated to correctly handle this.

@jmschonfeld
Copy link
Contributor

I had the same concern as @compnerd - unfortunately @_implementationOnly and internal import are not equivalent for non-resilient modules like Foundation. It sounds like @_implementationOnly fully hid the module which caused the NSLock subclassing bug, but changing it to internal import will cause the module to become visible meaning that we need to install it into the toolchain (which we don't on Windows). It sounds like the right decision is to make this an internal import to fix the subclassing bug, but that likely means we'll need to start installing the CoreFoundation module on Windows - is there a way we can do this while still preventing clients from writing import CoreFoundation in their sources? I'm thinking something like the -allowable-client flag for swift modules but for this clang module

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants