llbuild: Support is-mutated/is-command-timestamp nodes#7020
Conversation
Dependency of #7010 Added per the llbuild documentation at https://github.com/apple/swift-llbuild/blob/a3b8d34be319f1b3ba8de53c434386419dec1f4f/docs/buildsystem.rst#L387 and a corresponding test at https://github.com/apple/swift-llbuild/blob/a3b8d34be319f1b3ba8de53c434386419dec1f4f/tests/BuildSystem/Build/mutable-outputs.llbuild#L66. I also took the opportunity to get rid of deprecated TSC dependencies in `ManifestWriter.swift`, replacing it with string interpolation instead.
MaxDesiatov
added
build system
|
@swift-ci test |
|
|
||
| private mutating func render(nodes: [Node]) { | ||
| // We need to explicitly configure certain kinds of nodes. | ||
| let directoryStructureNodes = Set(nodes.filter { $0.kind == .directoryStructure }) |
There was a problem hiding this comment.
It's probably fine to separate all these for now, but it is valid for a node to be, for example, both a directory structure and mutating node
|
@swift-ci test |
| "\"\(self.jsonEscaped)\"" | ||
| } | ||
|
|
||
| private var jsonEscaped: String { |
There was a problem hiding this comment.
Directly lifted from TSC, suitable just for this use case. When we fully deprecate the JSON type, we can deprecate this in TSC too.
There was a problem hiding this comment.
is there nothing in Foundation that we can use instead of inventing our own? further why not use Codable?
There was a problem hiding this comment.
Nothing that they publicly expose. The new Swift Foundation has an internal serializedForJSON method on String, I'm not sure it's worth vendoring that instead of the TSC version? Additionally, we'd have to write our own encoder, since this is strictly neither JSON, nor YAML, it only uses JSON-escaping for strings. If we use JSONEncoder for some bits, without refactoring this much more we'd have to instantiate JSONEncoder on every property call. That would be quite expensive and probably a significant slowdown for large projects.
There was a problem hiding this comment.
I believe the llbuild manifest is supposed to be YAML?
There was a problem hiding this comment.
Hm, my bad, my impression was that it was somewhat custom, but I see it does mention YAML. Would you be opposed to refactoring this to render the build manifest with Codable and JSONEncoder in a future PR?
There was a problem hiding this comment.
Since we don't need to parse these manifests, I think rendering to JSON would mean we don't have to pull in additional dependencies for YAML support, and JSON is a subset of YAML.
There was a problem hiding this comment.
Added a FIXME comment to come back to this later. IMO the refactoring done in this PR is enough to unblock the other PR for now, but switching to JSONEncoder for this in the future makes the most sense to me.
| Node(name: name.pathString, kind: .file) | ||
| } | ||
|
|
||
| public static func file(_ name: AbsolutePath, isMutated: Bool) -> Node { |
There was a problem hiding this comment.
Couldn't create a default parameter for this one without breaking .map(Node.file) constructs that we have elsewhere in our code.
|
|
||
| /// Write a description of the tool to the given output `stream`. | ||
| func write(to stream: ManifestToolStream) | ||
| func write(to stream: inout ManifestToolStream) |
There was a problem hiding this comment.
Required after converting ManifestToolStream to a struct with a String buffer instead of using TSC streams.
|
@swift-ci test windows |
| targets:\n | ||
| """ | ||
| ) | ||
| public static func write(_ manifest: BuildManifest, at path: AbsolutePath, _ fileSystem: FileSystem) throws { |
There was a problem hiding this comment.
nit: use labeled parameter for fileSystem
|
@swift-ci test windows |
|
|
||
| let fs = InMemoryFileSystem() | ||
| try ManifestWriter(fileSystem: fs).write(manifest, at: "/manifest.yaml") | ||
| try ManifestWriter.write(manifest, at: "/manifest.yaml", fs) |
There was a problem hiding this comment.
while we are here, should we call this something different than "Manifest"? the current name is pretty confusing with the package manifest
There was a problem hiding this comment.
renamed to LLBuildManifest and LLBuildManifestWriter
|
@swift-ci test |
|
@swift-ci test windows |
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test windows |
4 participants
Dependency of #7010.
Added per the llbuild documentation at https://github.com/apple/swift-llbuild/blob/a3b8d34be319f1b3ba8de53c434386419dec1f4f/docs/buildsystem.rst#L387 and a corresponding test at https://github.com/apple/swift-llbuild/blob/a3b8d34be319f1b3ba8de53c434386419dec1f4f/tests/BuildSystem/Build/mutable-outputs.llbuild#L66.
Newly introduced attributes are not used anywhere yet in this PR, so technically it is NFC.
I also took the opportunity to get rid of deprecated TSC dependencies in
ManifestWriter.swift, replacing it with string interpolation instead.