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

SwiftTool: introduce --experimental-destination-selector option #5922

Merged
merged 16 commits into from Dec 16, 2022

Conversation

MaxDesiatov
Copy link
Member

@MaxDesiatov MaxDesiatov commented Nov 22, 2022

Depends on #5911.

Motivation:

With support for listing cross-compilation destinations with new swift experimental-destinations list command, we also need a way to actually use those. New --experimental-destination-selector option is added, through which users can either select a destination via an ID or a target triple.

Modifications:

Added new crossCompilationDestinationSelector to BuildOptions. Modified SwiftTool to select a destination from this selector string. Implemented destination selection based on a selector in DestinationsBundle.swift. Refactored Workspace.Location to create shared locations in a unified way. Added a simple test for destination selection.

Result:

Cross-compilation destinations can be selected when building a package.

@MaxDesiatov MaxDesiatov self-assigned this Nov 22, 2022
@MaxDesiatov
Copy link
Member Author

@swift-ci please smoke test

1 similar comment
@MaxDesiatov
Copy link
Member Author

@swift-ci please smoke test

@MaxDesiatov
Copy link
Member Author

I think @swift-ci doesn't support stacked PRs yet? I don't have any other explanation for current CI failures.

@MaxDesiatov MaxDesiatov marked this pull request as ready for review November 28, 2022 14:12
@@ -103,7 +103,7 @@ public struct Destination: Encodable, Equatable {
}

/// Additional flags to be passed to the build tools.
public let extraFlags: BuildFlags
public var extraFlags: BuildFlags
Copy link
Member

Choose a reason for hiding this comment

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

does it need to be mutated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it happens on https://github.com/apple/swift-package-manager/pull/5922/files#diff-ba866b666cdc2795c59e6713e0817015f0076f5447541e8c477db717cbb6baf3R114

Since it's a struct, I think making this var is safe, as any mutations are local and don't have side effects?

Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -180,14 +180,14 @@ public final class Manifest {
}

var requiredDependencies: Set<PackageIdentity> = []
for targetTriple in self.targetsRequired(for: products) {
for targetDependency in targetTriple.dependencies {
for linuxGNUTargetTriple in self.targetsRequired(for: products) {
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

automated Xcode renaming is bad at this, sorry...

@@ -48,7 +48,7 @@ public struct TestingObservability {
self.collector.hasWarnings
}

struct Collector: ObservabilityHandlerProvider, DiagnosticsHandler, CustomStringConvertible {
final class Collector: ObservabilityHandlerProvider, DiagnosticsHandler, CustomStringConvertible {
Copy link
Member Author

Choose a reason for hiding this comment

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

As I'm using ObservabilityScope in this PR, I'm taking an opportunity to make this a class, because diagnostics: ThreadSafeArrayStore is a class instance, so there's no actual value semantics here even with a struct.

try location.validate(
keyPath: \.sharedCrossCompilationDestinationsDirectory,
fileSystem: fileSystem,
getOrCreateHandler: {
Copy link
Member

Choose a reason for hiding this comment

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

use the function pointer?

Base automatically changed from maxd/list-destinations to main December 15, 2022 22:36
…xd/build-with-destinations

# Conflicts:
#	Sources/SPMBuildCore/DestinationsBundle.swift
@MaxDesiatov
Copy link
Member Author

@swift-ci please smoke test

@MaxDesiatov
Copy link
Member Author

@swift-ci please smoke test

@MaxDesiatov MaxDesiatov enabled auto-merge (squash) December 15, 2022 23:17
@MaxDesiatov
Copy link
Member Author

@swift-ci smoke test

@MaxDesiatov MaxDesiatov merged commit 9d287dd into main Dec 16, 2022
@MaxDesiatov MaxDesiatov deleted the maxd/build-with-destinations branch December 16, 2022 12:01
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.

None yet

3 participants