-
Notifications
You must be signed in to change notification settings - Fork 18
Cache lld
binaries to avoid redundant rebuilds
#35
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
Changes from all commits
4db7eaf
3ea95f4
73c1f4e
a5e5254
c525ec9
09201ea
80a4173
b5304e8
c96bda6
2d677b5
5ccecd4
aef9fcb
996fd27
e477ac5
6ca73f1
acd4973
5ca817c
a72e72a
45b30b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,36 @@ | ||||||||||
//===----------------------------------------------------------------------===// | ||||||||||
// | ||||||||||
// This source file is part of the Swift open source project | ||||||||||
// | ||||||||||
// Copyright (c) 2022-2023 Apple Inc. and the Swift project authors | ||||||||||
// Licensed under Apache License v2.0 with Runtime Library Exception | ||||||||||
// | ||||||||||
// See https://swift.org/LICENSE.txt for license information | ||||||||||
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||||||||||
// | ||||||||||
//===----------------------------------------------------------------------===// | ||||||||||
|
||||||||||
import GeneratorEngine | ||||||||||
import struct SystemPackage.FilePath | ||||||||||
|
||||||||||
@Query | ||||||||||
struct CMakeBuildQuery { | ||||||||||
let sourcesDirectory: FilePath | ||||||||||
/// Path to the output binary relative to the CMake build directory. | ||||||||||
let outputBinarySubpath: [FilePath.Component] | ||||||||||
euanh marked this conversation as resolved.
Show resolved
Hide resolved
MaxDesiatov marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
let options: String | ||||||||||
|
||||||||||
func run(engine: Engine) async throws -> FilePath { | ||||||||||
try await Shell.run( | ||||||||||
""" | ||||||||||
cmake -B build -G Ninja -S llvm -DCMAKE_BUILD_TYPE=Release \(self.options) | ||||||||||
""", | ||||||||||
currentDirectory: self.sourcesDirectory | ||||||||||
) | ||||||||||
|
||||||||||
let buildDirectory = self.sourcesDirectory.appending("build") | ||||||||||
try await Shell.run("ninja", currentDirectory: buildDirectory) | ||||||||||
|
||||||||||
return self.outputBinarySubpath.reduce(into: buildDirectory) { $0.append($1) } | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to me to read more easily and produces the same result, but maybe I'm missing something subtle
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was mostly about making it explicit that the query takes relative paths as arguments and not absolute ones. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I was reading the docs for FilePath just now and I wondered if that was what you were intending to do, to make sure components such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added a doc comment on that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thing that originally drew my attention here was that I found the The following still seems clearer to me, and it seems to be be able to append However it's just a style suggestion, and I think it's fine to merge this now so the generator branch doesn't accumulate too many PRs.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great, thanks, I'll update this to make it more readable! I'd be happy to merge after that, but prefer to merge PRs separately, so I need a review on the engine PR at the start of this PR chain: #31 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. I wondered if you were going to merge them all back down into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I prefer merging PRs from a stack separately, especially as I stick to "Squash and merge" that allows to preserve history of each PR in separate commits. |
||||||||||
} | ||||||||||
} |
Uh oh!
There was an error while loading. Please reload this page.