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

fix deprecation warning for Plugin's Path #363

Merged
merged 11 commits into from
Sep 6, 2024

Conversation

sebsto
Copy link
Contributor

@sebsto sebsto commented Sep 6, 2024

Replace the usage of Plugin's Path with URL

Motivation:

Path is deprecated in Swift 6

Modifications:

  • Replace the use of Path with URL and adjust path components management accordingly

Result:

no more compilation warnings when compiling the archive plugin on Swift 6

Command to test the plugin with Swift 6

	swift package archive \
		--base-docker-image swiftlang/swift:nightly-6.0-amazonlinux2 \
		--disable-sandbox

@sebsto
Copy link
Contributor Author

sebsto commented Sep 6, 2024

@swift-server-bot test this please

Comment on lines 18 to 25
#if canImport(FoundationEssentials)
import FoundationEssentials
#else
import struct Foundation.URL
import class Foundation.ProcessInfo
import class Foundation.FileManager
import struct Foundation.ObjCBool
#endif
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid we need full Foundation here. This is a built time dependency. So therefore we don't really care.

Suggested change
#if canImport(FoundationEssentials)
import FoundationEssentials
#else
import struct Foundation.URL
import class Foundation.ProcessInfo
import class Foundation.FileManager
import struct Foundation.ObjCBool
#endif
import Foundation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct - just realized that when testing the CI compilation (that failed) - I just pushed a change

Comment on lines 40 to 41
// this shared global variable is safe because we're mutating it in a dispatch group
// https://developer.apple.com/documentation/foundation/process/1408746-terminationhandler
Copy link
Member

Choose a reason for hiding this comment

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

I think the comment here is misleading. The important thing here is the following. We write to the variable from a single SerialDispatchQueue here. We wait until the process is run below process.waitUntilExit(). This means no further writes to output will happen. This makes it save for us to read the output.

Copy link
Member

Choose a reason for hiding this comment

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

We should switch to the new Foundation Process API as soon as it is available:
https://github.com/apple/swift-foundation/blob/main/Proposals/0007-swift-subprocess.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to

        // We need to use an unsafe transfer here to get the fd into our Sendable closure.
        // This transfer is fine, because we write to the variable from a single SerialDispatchQueue here.
        // We wait until the process is run below process.waitUntilExit().
        // This means no further writes to output will happen.
        // This makes it save for us to read the output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #364 to keep track of SubProcess

@sebsto sebsto mentioned this pull request Sep 6, 2024
Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Thanks so much! This is so great!

@fabianfett fabianfett merged commit 0af509b into swift-server:main Sep 6, 2024
10 of 13 checks passed
@sebsto sebsto deleted the sebsto/archive_plugin branch September 6, 2024 14:53
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.

2 participants