Skip to content

Allow build & run to reference unreachable targets #1284

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 26, 2017
Merged

Allow build & run to reference unreachable targets #1284

merged 1 commit into from
Jul 26, 2017

Conversation

@hartbit
Copy link
Contributor

@hartbit hartbit commented Jul 24, 2017

No description provided.

@hartbit hartbit requested a review from aciidgh July 24, 2017 21:07
@hartbit hartbit force-pushed the build-unreachable-products-and-targets branch 2 times, most recently from 6c97ff5 to e3583b5 Compare July 24, 2017 21:09
aciidgh
aciidgh requested changes Jul 25, 2017
View reviewed changes
Sources/Build/BuildPlan.swift
// Create build target description for each target which we need to plan.
var targetMap = [ResolvedTarget: TargetDescription]()
for target in graph.targets {
for target in graph.allTargets {
Copy link
Contributor

@aciidgh aciidgh Jul 25, 2017

Choose a reason for hiding this comment

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

Won't this start building all the targets when swift build is invoked?

Copy link
Contributor Author

@hartbit hartbit Jul 25, 2017

Choose a reason for hiding this comment

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

Fixed now.

Sources/Commands/SwiftRunTool.swift
}) else {
throw RunError.executableNotFound(executable)
}
Copy link
Contributor

@aciidgh aciidgh Jul 25, 2017

Choose a reason for hiding this comment

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

nit: avoid whitespace diffs

Sources/Commands/SwiftRunTool.swift
guard rootExecutables.count == 1 else {
throw RunError.multipleExecutables(rootExecutables.map({ $0.name }))
}
Copy link
Contributor

@aciidgh aciidgh Jul 25, 2017

Choose a reason for hiding this comment

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

nit: avoid whitespace diffs

Tests/CommandsTests/BuildToolTests.swift Outdated
XCTAssert(!output.contains("BLibrary"))
XCTAssert(!output.contains("bexec"))
XCTAssert(!output.contains("BTarget1"))
XCTAssert(!output.contains("BTarget2"))
Copy link
Contributor

@aciidgh aciidgh Jul 25, 2017

Choose a reason for hiding this comment

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

I'll land some APIs for string pattern matching. That should help a lot in this type of asserts.

@hartbit hartbit force-pushed the build-unreachable-products-and-targets branch from e3583b5 to a5edec5 Compare July 25, 2017 05:30
aciidgh
aciidgh requested changes Jul 25, 2017
View reviewed changes
Sources/Build/llbuild.swift Outdated
switch description {
case .swift(let description):
targets.append(createSwiftCompileTarget(description),
isReachable: plan.graph.targets.contains(target),
Copy link
Contributor

@aciidgh aciidgh Jul 25, 2017

Choose a reason for hiding this comment

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

This will be slow. We can add a method in package graph which internally uses a set for fast lookup.

Sources/Build/llbuild.swift Outdated
switch description {
case .swift(let description):
targets.append(createSwiftCompileTarget(description),
isReachable: plan.graph.targets.contains(target),
Copy link
Contributor

@aciidgh aciidgh Jul 25, 2017

Choose a reason for hiding this comment

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

We should also add some comments here about the isReachable and why it is needed.

Sources/Build/llbuild.swift Outdated

/// Append a command.
mutating func append(_ target: Target, isTest: Bool) {
mutating func append(_ target: Target, isReachable: Bool, isTest: Bool) {
Copy link
Contributor

@aciidgh aciidgh Jul 25, 2017

Choose a reason for hiding this comment

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

How about buildByDefault?

Sources/Commands/SwiftRunTool.swift
return executableProduct
} else {
// If the executably is implicit, search through root products.
let rootExecutables = plan.graph.rootPackages.flatMap({ $0.products }).filter({ $0.type == .executable })
Copy link
Contributor

@aciidgh aciidgh Jul 25, 2017

Choose a reason for hiding this comment

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

nit: matchingRootExecutables

Copy link
Contributor Author

@hartbit hartbit Jul 25, 2017

Choose a reason for hiding this comment

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

I don't mind, but is it really more explicit? The predicate we are matching against is already in the name: it's the list of "root executables".

@hartbit hartbit force-pushed the build-unreachable-products-and-targets branch 2 times, most recently from 24c09eb to 7a87824 Compare July 25, 2017 20:09
aciidgh
aciidgh requested changes Jul 26, 2017
View reviewed changes
Tests/CommandsTests/BuildToolTests.swift Outdated
let plan = try BuildPlan(buildParameters: mockBuildParameters(), graph: graph)

XCTAssertEqual(Set(plan.buildProducts.map({ $0.product.name })), ["aexec", "ATarget", "BLibrary", "bexec", "cexec"])
XCTAssertEqual(Set(plan.targets.map({
Copy link
Contributor

@aciidgh aciidgh Jul 26, 2017

Choose a reason for hiding this comment

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

We have BuildPlanResult to assert the build plan in an easier way.

Tests/CommandsTests/BuildToolTests.swift Outdated
}
}

func testNonReachableProductsAndTargets() throws {
Copy link
Contributor

@aciidgh aciidgh Jul 26, 2017

Choose a reason for hiding this comment

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

This test should be in `BuildPlanTests``

@aciidgh aciidgh added WIP Work in progress and removed pending evolution proposal labels Jul 26, 2017
@aciidgh aciidgh assigned hartbit and unassigned aciidgh Jul 26, 2017
@hartbit hartbit force-pushed the build-unreachable-products-and-targets branch from 7a87824 to d11e5a7 Compare July 26, 2017 11:13
@hartbit hartbit added pending evolution proposal and removed WIP Work in progress labels Jul 26, 2017
@hartbit hartbit assigned aciidgh and unassigned hartbit Jul 26, 2017
aciidgh
aciidgh reviewed Jul 26, 2017
View reviewed changes
Tests/CommandsTests/BuildToolTests.swift Outdated
import Basic
import Build
import Commands
import PackageDescription4
Copy link
Contributor

@aciidgh aciidgh Jul 26, 2017

Choose a reason for hiding this comment

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

Are these imports required?

Copy link
Contributor Author

@hartbit hartbit Jul 26, 2017

Choose a reason for hiding this comment

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

Nope, nice catch!

aciidgh
aciidgh reviewed Jul 26, 2017
View reviewed changes
Tests/BuildTests/BuildPlanTests.swift
import PackageDescription
import PackageDescription4

private struct MockToolchain: Toolchain {
Copy link
Contributor

@aciidgh aciidgh Jul 26, 2017

Choose a reason for hiding this comment

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

This doesn't need to be moved now, right?

Copy link
Contributor Author

@hartbit hartbit Jul 26, 2017

Choose a reason for hiding this comment

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

Correct!

@hartbit hartbit force-pushed the build-unreachable-products-and-targets branch from d11e5a7 to cb71bd7 Compare July 26, 2017 11:25
@hartbit
Copy link
Contributor Author

hartbit commented Jul 26, 2017

@swift-ci please smoke test

@hartbit hartbit force-pushed the build-unreachable-products-and-targets branch from cb71bd7 to 0e6f30d Compare July 26, 2017 12:28
@hartbit
Copy link
Contributor Author

hartbit commented Jul 26, 2017

@swift-ci please smoke test

@hartbit hartbit merged commit fb4ef9d into swiftlang:master Jul 26, 2017
@hartbit hartbit deleted the build-unreachable-products-and-targets branch July 26, 2017 22:20
@aciidgh aciidgh mentioned this pull request Jul 31, 2017
Merged
@hartbit hartbit mentioned this pull request Oct 26, 2017
Merged
@aciidgh aciidgh mentioned this pull request Jul 31, 2017
Closed
@hartbit hartbit mentioned this pull request May 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aciidgh aciidgh aciidgh approved these changes

Assignees

@aciidgh aciidgh

Labels

pending evolution proposal

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hartbit @aciidgh