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

Add experimental-destination configuration subcommand #6252

Merged
merged 4 commits into from
Mar 10, 2023

Conversation

MaxDesiatov
Copy link
Contributor

Motivation:

For configuring destination bundles that can't be self-contained and have to reference absolute paths we need new to provide a CLI. This comes as a new swift experimental-destination configuration subcommand per the proposed changes in swiftlang/swift-evolution#1942.

Modifications:

DestinationCommand protocol is refined to wait for the observability handler to flush all output. Existing destination-related subcommands are modified to conform to it. Additionally, it now passes a newly initialized observability scope and configuration values.

Two new subcommands are added: configuration show and configuration reset. For actually updating configuration values a third subcommand configuration set will be added in a separate PR.

Result:

Users can now view and reset destination-related paths configuration.

@MaxDesiatov MaxDesiatov self-assigned this Mar 9, 2023
@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

@@ -198,7 +198,8 @@ extension ObservabilitySystem {
public static func swiftTool(
outputStream: OutputByteStream = stdoutStream,
logLevel: Basics.Diagnostic.Severity = .warning
) -> ObservabilitySystem {
.init(SwiftToolObservabilityHandler(outputStream: stdoutStream, logLevel: logLevel))
) -> (ObservabilitySystem, SwiftToolObservabilityHandler) {
Copy link
Contributor Author

@MaxDesiatov MaxDesiatov Mar 9, 2023

Choose a reason for hiding this comment

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

This change is needed to allow getting a reference to SwiftToolObservabilityHandler and calling wait(timeout:) 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.

I think this is fine, but at some point we should consider if these new command should become aligned with the other commands so they dont need this kind of external access

Copy link
Contributor Author

@MaxDesiatov MaxDesiatov Mar 9, 2023

Choose a reason for hiding this comment

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

I found a better way to address that within this PR. Turns out static func swiftTool was used just once in DestinationCommand, it made more sense to move its implementation over there, so there's no need for this tuple-returning function anymore.

@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci smoke test

@MaxDesiatov MaxDesiatov enabled auto-merge (squash) March 10, 2023 13:02
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov MaxDesiatov merged commit 52d9590 into main Mar 10, 2023
@MaxDesiatov MaxDesiatov deleted the maxd/destination-configuration-subcommand branch March 10, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants