Skip to content

Extend LLBuildManifestWriter to let shell commands specify environment and initial working directory #3332

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion Sources/LLBuildManifest/BuildManifest.swift
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
This source file is part of the Swift.org open source project

Copyright (c) 2014 - 2019 Apple Inc. and the Swift project authors
Copyright (c) 2014 - 2021 Apple Inc. and the Swift project authors
Licensed under Apache License v2.0 with Runtime Library Exception

See http://swift.org/LICENSE.txt for license information
Expand Down Expand Up @@ -96,13 +96,17 @@ public struct BuildManifest {
inputs: [Node],
outputs: [Node],
args: [String],
environ: [String: String] = [:],
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: environ -> environment (more swifty)

Copy link
Contributor Author

@abertelrud abertelrud Mar 8, 2021

Choose a reason for hiding this comment

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

Agreed. I was trying to follow the style of "args" above. I could change this but would probably also want to change 'args etc. Let's do that in a follow-up where we can clean them all up.

workingDir: String? = nil,
allowMissingInputs: Bool = false
) {
let tool = ShellTool(
description: description,
inputs: inputs,
outputs: outputs,
args: args,
environ: environ,
workingDir: workingDir,
allowMissingInputs: allowMissingInputs
)
commands[name] = Command(name: name, tool: tool)
Expand Down
10 changes: 10 additions & 0 deletions Sources/LLBuildManifest/ManifestWriter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,16 @@ public final class ManifestToolStream {
stream <<< " \(key): " <<< Format.asJSON(newValue) <<< "\n"
}
}

public subscript(key: String) -> [String: String] {
get { fatalError() }
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be better to use a func instead of subscript so we don't have this potential runtime issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree in general but figured it was better to stay with the existing approach for this PR, i.e. the cognitive cost of something completely different for this one case would be more than the benefit. Seems to me that we should redo all of these at some point.

set {
stream <<< " \(key):\n"
for (key, value) in newValue.sorted(by: { $0.key < $1.key }) {
stream <<< " " <<< Format.asJSON(key) <<< ": " <<< Format.asJSON(value) <<< "\n"
}
}
}
}

extension Format {
Expand Down
14 changes: 13 additions & 1 deletion Sources/LLBuildManifest/Tools.swift
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
This source file is part of the Swift.org open source project

Copyright (c) 2014 - 2019 Apple Inc. and the Swift project authors
Copyright (c) 2014 - 2021 Apple Inc. and the Swift project authors
Licensed under Apache License v2.0 with Runtime Library Exception

See http://swift.org/LICENSE.txt for license information
Expand Down Expand Up @@ -97,25 +97,37 @@ public struct ShellTool: ToolProtocol {
public var inputs: [Node]
public var outputs: [Node]
public var args: [String]
public var environ: [String: String]
public var workingDir: String?
public var allowMissingInputs: Bool

init(
description: String,
inputs: [Node],
outputs: [Node],
args: [String],
environ: [String: String] = [:],
workingDir: String? = nil,
allowMissingInputs: Bool = false
) {
self.description = description
self.inputs = inputs
self.outputs = outputs
self.args = args
self.environ = environ
self.workingDir = workingDir
self.allowMissingInputs = allowMissingInputs
}

public func write(to stream: ManifestToolStream) {
stream["description"] = description
stream["args"] = args
if !environ.isEmpty {
stream["env"] = environ
}
if let workingDir = workingDir {
stream["working-directory"] = workingDir
}
if allowMissingInputs {
stream["allow-missing-inputs"] = true
}
Expand Down
57 changes: 56 additions & 1 deletion Tests/BuildTests/LLBuildManifestTests.swift
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
This source file is part of the Swift.org open source project

Copyright (c) 2014 - 2019 Apple Inc. and the Swift project authors
Copyright (c) 2014 - 2021 Apple Inc. and the Swift project authors
Licensed under Apache License v2.0 with Runtime Library Exception

See http://swift.org/LICENSE.txt for license information
Expand Down Expand Up @@ -53,6 +53,61 @@ final class LLBuildManifestTests: XCTestCase {
outputs: ["<Foo>"]


""")
}

func testShellCommands() throws {
var manifest = BuildManifest()

manifest.defaultTarget = "main"
manifest.addShellCmd(
name: "shelley",
description: "Shelley, Keats, and Byron",
inputs: [
.file(AbsolutePath("/file.in]"))
],
outputs: [
.file(AbsolutePath("/file.out"))
],
args: [
"foo", "bar", "baz"
],
environ: [
"ABC": "DEF",
"G H": "I J K",
],
workingDir: "/wdir",
allowMissingInputs: true
)

manifest.addNode(.file(AbsolutePath("/file.out")), toTarget: "main")

let fs = InMemoryFileSystem()
try ManifestWriter(fs).write(manifest, at: AbsolutePath("/manifest.yaml"))

let contents = try fs.readFileContents(AbsolutePath("/manifest.yaml"))

XCTAssertEqual(contents, """
client:
name: basic
tools: {}
targets:
"main": ["/file.out"]
default: "main"
commands:
"shelley":
tool: shell
inputs: ["/file.in]"]
outputs: ["/file.out"]
description: "Shelley, Keats, and Byron"
args: ["foo","bar","baz"]
env:
"ABC": "DEF"
"G H": "I J K"
working-directory: "/wdir"
allow-missing-inputs: true


""")
}
}