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

Implementation for the `swift run` command #1187

Merged
merged 1 commit into from Jun 17, 2017

Conversation

Projects
None yet
4 participants
@hartbit
Copy link
Collaborator

hartbit commented May 15, 2017

No description provided.


/// Executes and returns execution status. Prints output on standard streams.
private func run(path: AbsolutePath) throws {
let process = Process(arguments: [path.asString] + options.arguments, redirectOutput: false)

This comment has been minimized.

@ddunbar

ddunbar May 15, 2017

Member

On UNIX, we should exec the process rather than run it in a subprocess. These will ensure that we don't influence the programs behavior w.r.t. signal handling, buffering, etc.

/// Builds the "executable" target if enabled in options.
///
/// - Returns: The path to the executable binary.
private func buildIfNeeded() throws -> AbsolutePath {

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r May 16, 2017

Member

It feels weird that this returns path to the executable. We can just inline this for now.

This comment has been minimized.

@hartbit

hartbit May 16, 2017

Author Collaborator

Agreed. I was just reproducing the structure of swift test in case that's what you preferred. Will inline it.


return buildPath
.appending(RelativePath(options.config.dirname))
.appending(component: executable.name)

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r May 16, 2017

Member

We should be using buildplan to get the path to executable, can you land the refactor first?

@hartbit hartbit force-pushed the hartbit:swift-run branch from 689f82f to 5d2fc94 May 17, 2017

@aciidb0mb3r aciidb0mb3r added the wip label May 21, 2017

@ddunbar

This comment has been minimized.

Copy link
Member

ddunbar commented May 31, 2017

We accepted SE-179 with revisions:
https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20170529/036909.html
which we should incorporate here.

@hartbit

This comment has been minimized.

Copy link
Collaborator Author

hartbit commented May 31, 2017

I have an implementation ready for the accepted (revised) version. Just need to wait for #1215 to be merged first.

@hartbit hartbit force-pushed the hartbit:swift-run branch 2 times, most recently from 66f7067 to 32e47e2 May 31, 2017

@hartbit hartbit assigned ddunbar and aciidb0mb3r and unassigned hartbit May 31, 2017

@hartbit hartbit added awaiting review and removed wip labels May 31, 2017

@hartbit hartbit changed the title [WIP] Implementation for the `swift run` command Implementation for the `swift run` command May 31, 2017

case .executableNotFound(let executable):
return "could not find executable '\(executable)'"
case .multipleExecutables:
return "multiple products defined"

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Jun 1, 2017

Member

We should make this more descriptive, maybe something like: "There are multiple executable products in the package. Use 'swift run ' to run one of the executable".

This comment has been minimized.

@ddunbar

ddunbar Jun 1, 2017

Member

It might be nice to also list the executables, so users don't then have to look up the names.

private enum RunError: Swift.Error {
case noExecutableFound
case executableNotFound(String)
case multipleExecutables

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Jun 1, 2017

Member

doc comments

}
}

/// Executes and returns execution status. Prints output on standard streams.

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Jun 1, 2017

Member

Stale comment


let product = try findExecutable(in: plan)
let path = plan.buildParameters.buildPath.appending(component: product.name)
try run(at: path)

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Jun 1, 2017

Member

This sounds wrong. Maybe findExectuable can return the path of the exe and then we can do this:

...
let executable = try findExecutable(in: plan)
try run(executable)
...
}

return executableProduct
} else {

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Jun 1, 2017

Member

Else is not required

This comment has been minimized.

@hartbit

hartbit Jun 1, 2017

Author Collaborator

What do you mean by that?

This comment has been minimized.

@ddunbar

ddunbar Jun 1, 2017

Member

That you can just put the else branch after the if, since it early returns.

This comment has been minimized.

@hartbit

hartbit Jun 1, 2017

Author Collaborator

Ok, got it!

throw RunError.multipleExecutables
}

return executableProducts.first!

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Jun 1, 2017

Member

nit: return executableProducts[0] feels nicer for some reason.

This comment has been minimized.

@ddunbar

ddunbar Jun 1, 2017

Member

Maybe we should have an extension .only which returns the only item iff array has one element, this is a not uncommon pattern, and that utility works nicely in guard.

This comment has been minimized.

@hartbit

hartbit Jun 1, 2017

Author Collaborator

Sounds like a nice utility! Separate PR I guess?

This comment has been minimized.

@ddunbar

ddunbar Jun 1, 2017

Member

Ya, I wouldn't block this one on it, just update it to use it if we decide it is a good idea.


/// Executes and returns execution status. Prints output on standard streams.
private func run(at path: AbsolutePath) throws {
if let workingDirectory = options.workingDirectory {

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Jun 1, 2017

Member

Where is this set?

This comment has been minimized.

@ddunbar

ddunbar Jun 1, 2017

Member

I think this became dead code, and the option can be removed.

try POSIX.chdir(workingDirectory.asString)
}

let relativePath = path.relative(to: currentWorkingDirectory)

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Jun 1, 2017

Member

currentWorkingDirectory -> originalWorkingDirectory

This comment has been minimized.

@ddunbar

ddunbar Jun 1, 2017

Member

We should have a test case that fails w/o this fix.

This comment has been minimized.

@hartbit

hartbit Jun 1, 2017

Author Collaborator

I'm confused. Looking at the code, it seems originalWorkingDirectory is currently only set once to currentWorkingDirectory. Not quite sure what its use case is.

@@ -929,6 +929,18 @@ public final class ArgumentBinder<Options> {
}
}

/// Bind an array positional argument.
public func bindArray<T>(

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Jun 1, 2017

Member

Please add this in a separate commit/PR


func testVersion() throws {
XCTAssert(try execute(["--version"]).contains("Swift Package Manager"))
}

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Jun 1, 2017

Member

We should add a basic functional test. We can extend one of the executable related functional test.

@aciidb0mb3r aciidb0mb3r added wip and removed awaiting review labels Jun 1, 2017

@aciidb0mb3r aciidb0mb3r removed their assignment Jun 1, 2017

case .executableNotFound(let executable):
return "could not find executable '\(executable)'"
case .multipleExecutables:
return "multiple products defined"

This comment has been minimized.

@ddunbar

ddunbar Jun 1, 2017

Member

It might be nice to also list the executables, so users don't then have to look up the names.


/// Executes and returns execution status. Prints output on standard streams.
private func run(at path: AbsolutePath) throws {
if let workingDirectory = options.workingDirectory {

This comment has been minimized.

@ddunbar

ddunbar Jun 1, 2017

Member

I think this became dead code, and the option can be removed.

public convenience init(args: [String]) {
self.init(
toolName: "run",
usage: "[options] [executable <arguments>]",

This comment has been minimized.

@ddunbar

ddunbar Jun 1, 2017

Member

This is somewhat unconventional syntax for usage, I would spell it as:
[executable [arguments ...]]

throw RunError.multipleExecutables
}

return executableProducts.first!

This comment has been minimized.

@ddunbar

ddunbar Jun 1, 2017

Member

Maybe we should have an extension .only which returns the only item iff array has one element, this is a not uncommon pattern, and that utility works nicely in guard.

try POSIX.chdir(workingDirectory.asString)
}

let relativePath = path.relative(to: currentWorkingDirectory)

This comment has been minimized.

@ddunbar

ddunbar Jun 1, 2017

Member

We should have a test case that fails w/o this fix.

@ddunbar ddunbar assigned hartbit and unassigned ddunbar Jun 1, 2017

@hartbit hartbit force-pushed the hartbit:swift-run branch 3 times, most recently from 815cde3 to ffc6e67 Jun 1, 2017

@hartbit hartbit assigned ddunbar and aciidb0mb3r and unassigned hartbit Jun 4, 2017

product = executableProducts[0]
}

return plan.buildParameters.buildPath.appending(component: product.name)

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Jun 13, 2017

Member

nit: Consider adding some high level comments on whats going on in this method.


/// Executes the executable at the specified path.
private func run(_ excutablePath: AbsolutePath) throws {
let relativePath = excutablePath.relative(to: originalWorkingDirectory)

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Jun 13, 2017

Member

The name relativePath is too vague.

See http://swift.org/CONTRIBUTORS.txt for Swift project authors
*/

import class Foundation.ProcessInfo

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Jun 13, 2017

Member

Is this needed?

@aciidb0mb3r aciidb0mb3r assigned hartbit and unassigned aciidb0mb3r Jun 13, 2017

@aciidb0mb3r aciidb0mb3r added wip and removed awaiting review labels Jun 13, 2017

@hartbit hartbit force-pushed the hartbit:swift-run branch from 25c3e6c to ca4af45 Jun 14, 2017

@hartbit hartbit added awaiting review and removed wip labels Jun 16, 2017

@hartbit hartbit assigned aciidb0mb3r and unassigned hartbit Jun 16, 2017

@hartbit hartbit force-pushed the hartbit:swift-run branch from ca4af45 to 088b6eb Jun 16, 2017

@hartbit

This comment has been minimized.

Copy link
Collaborator Author

hartbit commented Jun 16, 2017

@swift-ci please smoke test

1 similar comment
@hartbit

This comment has been minimized.

Copy link
Collaborator Author

hartbit commented Jun 16, 2017

@swift-ci please smoke test

@hartbit hartbit force-pushed the hartbit:swift-run branch from 088b6eb to 733b9b1 Jun 16, 2017

@hartbit

This comment has been minimized.

Copy link
Collaborator Author

hartbit commented Jun 16, 2017

@swift-ci please smoke test

2 similar comments
@hartbit

This comment has been minimized.

Copy link
Collaborator Author

hartbit commented Jun 16, 2017

@swift-ci please smoke test

@hartbit

This comment has been minimized.

Copy link
Collaborator Author

hartbit commented Jun 16, 2017

@swift-ci please smoke test

@hartbit hartbit force-pushed the hartbit:swift-run branch from 733b9b1 to 1fe9ea2 Jun 17, 2017

@hartbit

This comment has been minimized.

Copy link
Collaborator Author

hartbit commented Jun 17, 2017

@swift-ci please smoke test

1 similar comment
@hartbit

This comment has been minimized.

Copy link
Collaborator Author

hartbit commented Jun 17, 2017

@swift-ci please smoke test

@hartbit hartbit merged commit b4d728c into apple:master Jun 17, 2017

2 checks passed

Swift Test Linux Platform (smoke test) Build finished.
Details
Swift Test OS X Platform (smoke test) Build finished.
Details

@hartbit hartbit deleted the hartbit:swift-run branch Jun 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment