[GSoC] SwiftPM PR #1
Conversation
Package.swift
Outdated
| @@ -356,12 +378,14 @@ if ProcessInfo.processInfo.environment["SWIFTCI_USE_LOCAL_DEPS"] == nil { | |||
| .package(url: "https://github.com/apple/swift-argument-parser.git", .upToNextMinor(from: "0.4.3")), | |||
| .package(url: "https://github.com/apple/swift-driver.git", .branch(relatedDependenciesBranch)), | |||
| .package(url: "https://github.com/apple/swift-crypto.git", .upToNextMinor(from: "1.1.4")), | |||
| .package(url: "https://github.com/apple/swift-syntax.git", .branch(relatedDependenciesBranch)), | |||
There was a problem hiding this comment.
In principle this is correct, but SwiftSyntax is particularly tricky, because of its dependency on the corresponding linker library. In fact I wasn't able to get the main branch to work with Xcode 13 Betas at all. But as it happens we have two other intended uses of SwiftSyntax, and so I will be working with someone who knows it better than I do to add it as a dependency in a separate PR. So you can keep this as it is right now, and within not too long we should be able to have this dependency already added through a separate PR.
There was a problem hiding this comment.
That's weird because I'm building it with Xcode 13 Beta 4/5, on Big Sur.
I think the dependency could be dropped here some day for parsing, but it would help doing things like generating a package.
| } | ||
|
|
||
| extension FileSystem { | ||
| public func getOrCreateSwiftScriptCacheDirectory() throws -> AbsolutePath { |
There was a problem hiding this comment.
Could this instead just call getOrCreateSwiftPMCacheDirectory() and then append the scripts subdirectory to it? Otherwise this function seems to duplicate some of the existing code.
There was a problem hiding this comment.
Ummm... In fact the scripts dir is supposed to be beside cache, which I'm afraid may "escape" if we simply append ../scripts to the cache dir (also, we need to create it if there's none, which needs a function to wrap up).
I thought it may be better to hardcode the path with ~/.swiftpm/scripts to make it more accessible.
There was a problem hiding this comment.
That makes sense — I'm not 100% familiar with how the layout is supposed to look here, it just seemed as if there was some code overlap here.
| extension ScriptCommand { | ||
| public func run() throws { | ||
| guard let file = options.file else { | ||
| throw ScriptError.fileNotFound("") |
There was a problem hiding this comment.
Should this perhaps be a different error, indicating that no file was provided (as opposed to that the provided file couldn't be found)?
| var output: AbsolutePath? | ||
|
|
||
| func run(_ swiftTool: SwiftTool, as productName: String, at cacheDirPath: AbsolutePath) throws { | ||
| swiftTool.redirectStdoutToStderr() |
There was a problem hiding this comment.
Is this to separate out any output from the build from the output emitted by the script, once it runs?
There was a problem hiding this comment.
Yes. This piece of code is copied from swift-run.
| swiftTool.redirectStdoutToStderr() | ||
| } | ||
| // FIXME: More elegant solution? | ||
| print(diagnostic: .init(message: .note("Using cache: \(cacheDirPath.basename)")), |
There was a problem hiding this comment.
If quiet is set, this will have already been redirected to a buffer that won't be displayed — am I understanding that correctly? In other words, it won't be shown when quiet?
I think that quiet should perhaps be the default, or to turn it around, that there should be a verbose flag that can provide extra information (such as whether cache is used).
There was a problem hiding this comment.
Also: is it the case that this only says whether a cache could be used? It doesn't seem to indicate whether a cache result was actually found, or am I reading that incorrectly?
There was a problem hiding this comment.
If quiet is set, this will have already been redirected to a buffer that won't be displayed
Yes. Except that the build fails.
Also: is it the case that this only says whether a cache could be used? It doesn't seem to indicate whether a cache result was actually found, or am I reading that incorrectly?
The cache name displayed here acts as a shortcut to the file.eg., you can call swift script run test-67a8a2 instead of swift script run /Users/yr.chen/Developer/GSoC/test.swift once the cache is set up.
Currently it cannot detect whether there was an up-to-date prebuild. We can add this feature and emit building step (and its outputs) when a cache is exactly hit, making clean as a default if the script was prebuilt.
| fileprivate extension AbsolutePath { | ||
| var sha256Hash: String { | ||
| String(ByteString(stringLiteral: pathString) | ||
| .sha256Checksum.prefix(6)) |
There was a problem hiding this comment.
Are the first six digits going to be unique enough?
There was a problem hiding this comment.
These digits will be used as a prefix to the file name, I think the combination is fairly unique. 🤔
Sources/ScriptingCore/utils.swift
Outdated
| case fileNotFound(String) | ||
| /// The target path is directory | ||
| case isDirectory(String) |
There was a problem hiding this comment.
Should these two maybe store AbsolutePaths?
There was a problem hiding this comment.
These may receive user inputs that is even not a valid path. String is better as they had no meanings but for display use.
| } | ||
|
|
||
| /// Resolves a path string to `AbsolutePath`. | ||
| public func resolveFilePath(_ path: String) -> AbsolutePath? { |
There was a problem hiding this comment.
I think there's a convenience initializer in AbsolutePath to do this.
There was a problem hiding this comment.
This is the current implementation:
public init(_ absPath: AbsolutePath, _ relStr: String) {
self.init(absPath, RelativePath(relStr))
}The second parameter is used to init a RelativePath, which cannot meet our need.
I'm working on a more robust implementation at swiftlang/swift-tools-support-core#219, but there's more to be done before it can get merged.
| // swift-tools-version:5.4 | ||
| import PackageDescription | ||
| let package = Package( | ||
| name: "\(productName)", |
There was a problem hiding this comment.
All these strings should probably be escaped in case they contain a quote character (unlikely, but good practice in general).
There was a problem hiding this comment.
let productName = scriptPath.basename.spm_dropSuffix(".swift").spm_mangledToBundleIdentifier()productName is already escaped by spm_mangledToBundleIdentifier().
| guard localFileSystem.isDirectory(cacheDirPath) else { | ||
| throw ScriptError.fileNotFound(file) | ||
| } | ||
| return (String(file.dropLast(7)), cacheDirPath) |
There was a problem hiding this comment.
This hardcoded 7 seems a little shady — what is the suffix that is being dropped here? Is it "Sources"? There is probably a clearer / safer way to do this. I think there's a helper function to drop a suffix if it exists on the path.
There was a problem hiding this comment.
It's not a hardcoded one. These should be hyphen and the six hash digits.
Better to split the name with - and emit the last part? I was not doing it because this shortcut is only supposed to work with a standardized cache name.
There was a problem hiding this comment.
Yes, in general using a separator and split on it is ore robust than assuming a certain number of characters.
| let resolved = try scripts.compactMap { script -> (String, String)? in | ||
| let sourceDir = cacheDir.appending(components: script, "Sources") | ||
| guard localFileSystem.isDirectory(sourceDir), | ||
| let name = try localFileSystem.getDirectoryContents(sourceDir).first, |
There was a problem hiding this comment.
Does this logic depend on the order of the file system entries? FileSystem.getDirectoryEntries() doesn't make any guarantees about the order.
There was a problem hiding this comment.
If it is a valid package layout, there should be exactly one directory under Sources, because the package should have only one target.
[One line description of your change]
Motivation:
[Explain here the context, and why you're making that change. What is the problem you're trying to solve.]
Modifications:
[Describe the modifications you've done.]
Result:
[After your change, what will change.]