The review of SE-0303, begins now and runs through March 12, 2021.
Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager or direct message in the Swift forums).
What goes into a review of a proposal?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.
When reviewing a proposal, here are some questions to consider:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Thank you for helping improve the Swift programing language and ecosystem.
Yes, the intent here was to express that, because the list of input source files can be large, the API will provide a custom iterator for them rather than a plain array. The proposal is incorrect about the syntax here, but that is the intent.
At the same time, I would like to as a question. As a contributor to the SwiftGtk I would like to use this new feature to wrap code generator gir2swift and generate the Gtk swift API before build.
Background:
The Gtk library (and it's dependencies) are C libraries. Part of the GLib/Gtk project is a tool called GObject Introspection which reads header files of the library, collects metadata of the API and produces a XML file .gir. The .gir file is distributed with each GLib/GObject library (development package) and is stored in the filesystem. The gir2swift searches for the file and generates a Swift API using metadata stored in the .gir file.
The .gir file location varies, depending on the platform and/or version of the platform. For example /usr/share/gir-1.0 on ubuntu. But on macOS 11, .gir file for each library is in it's separate folder.
The problem:
I have read in the proposal, that the plugin will gain access only to the folder of the swift package. Am I correct in assuming, that this will prevent potential gir2sdwift plugin from obtaining the .gir file?
If so, would you accept a suggestion that will make me able to search the file system in any way? Shipping the .gir file with the package itself is not an option, since it would defeat the prupose of gir2swift in the first place.
This looks great! I just have a couple of questions:
The APIs being introduced include this note
/// In addition, the API of `FilePath` from SwiftSystem will be available.
Does this mean SwiftPM will be vendoring a copy of System on macOS, or reimplementing the API itself? The version of System.FilePath bundled with macOS 11 is very limited without the newer syntactic operations.
Also, relying on System's FilePath seems like it could cause problems if and when platform specific APIs get added. I think it would be pretty easy for somebody to accidentally break their plugin on platforms they're not testing regularly.
An errors emitted by the build command will be handled by the build system as for other build commands. SwiftPM will should output on console, and IDEs that use libSwiftPM will show it as it does for the other build commands.
It might be helpful to specify what the expected format is here for errors/warnings/etc. In the long run it might also be good to provide a CommandConstructor API like func addDiagnosticsRecordFile(path: FilePath) using either .dia files or some other format to let tools like linters provide source locations, highlighted ranges, and fix-its.
Is the problem being addressed significant enough to warrant a change to Swift?
I think so.
I personally intend to use this for linter. It might also lead me to use Sourcery and/or Protobuf.
Does this proposal fit well with the feel and direction of Swift?
Yes.
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
I use Xcode script build phases.
This proposal seems better integrated, especially in the build dependency graph.
On the other hand, it seems a bit tedious to require a target for the plugin. It seems to me it would be convenient sometimes to also be able to put that code in the manifest itself.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Apologizes if I missed these in the proposal/previous discussions, I've only had the time to do a quick skim of things.
Looking at the examples and apis, I'm not completely clear on how much support is intended for dependencies between generated things? The SwiftProtobuf example is one where this could easily come up - a package exposes some .proto files to generate from as part of the interface. Another package could then expose their own .proto files that use Message/Enums from the option package as fields in their Messages. How would something like this get captured/declared so the proper dependencies are in place for both the generation action as well as the compilation actions? You could also imagine something similar with .proto files in one package and a Service definition in new .proto file (using grpc-swift) in another package.
The package graph is somewhat exposed, but would a build tool have to expect things expressed in a custom file for generation params and then it uses that data to go back and fine all the other files/things it needs? The DependencyTargetInfo seems to include the source directory information, but not the output directory, so a new build tool couldn't read/consume output from another generation, it would have to reimplement the logic based on the inputs to derive what should have happened and what would be in the outputs. For this last part, I'm specifically thinking of thinks like the SwiftProtobuf proto file to module mappings. Since the tool will likely have to generate this file, if there was a second target that depended on it, it would be nice to reuse that generate file instead of each action having to generate the data for the entire graph.
What about support for files that need to be generated for some tool to be run but aren't actually part of the final SwiftPM build? Something like the previously mentioned proto file to module mappings file? Is that expected to be done in some way with CommandConstructor.addCommand? The documentation on TargetBuildContext.outputDir says it can be written to to add anything needed, but when is the plugin called? i.e. - what if it needs something from a dependency, is the dependency already built?
CommandConstructor.addCommand tasks a lot of FilePath arguments, and since may of them are likely to include generated directories, they could get very long. How will the process limits on command line length be handled? Compilers and linkers already have support for params files for this, are the tools expected to auto detect when a params file has to be used and write one out, or should addCommand have some way to indicate that a params file should be used if things get too long? (Bazel goes the route of having a command builder with support to indicate how a params file can be used (and the format of the file) - Args - Bazel 2.1.0)
TargetBuildContext.inputFiles calls out a custom Sequence for efficiency, but addCommand only takes Arrays for the inputPaths and arguments, so if there are extra args to the tool or a configuration file needed as inputs, that efficient Sequence will always have to be fully expanded to add additional things. Bazel solves some of this space with it's depset for building up input file lists and the Args object to allow transforms into the command lines.
How do the different target platforms come into play for all of this? Say a Package's library is used in an Xcode project for both an iOS app and a watchOS app that is bundled with it. Should the build tool get to know it is generating for watchOS vs. iOS incase it needs to change how it generates? That would also imply it generates once per platform, or does it have to always generate code with conditions to compile for any of the platforms?
Related to the target platforms questions - with an eye to apple/swift-llbuild2 & apple/swift-llbuild2, should there be anything about indicating if the tool should run on a given platform? Likewise, the current api exposes a lot of directory structure when things run does seem to allow writing into the output directory independent of addCommand, etc.; that could be an issue in trying to eventually use remote execution and/or require work that happen within the client build process that slows down sending things for remote execution.
Thanks for pointing out this use case. The intent behind only allowing the plugin to see the package and its dependencies was avoid unexpressed dependencies on the host system, to avoid having to rely on having things like source generator tools be manually installed before the package can be built. The idea is to extend binary targets to allow tools and auxiliary files to be fetched on-demand.
There will probably need to be some exceptions for cases where the support files really do need to be external. The experience with SwiftPM's systemModule() targets, which kind of like this, hasn't been great, since the host has to be set up carefully for things to work.
I'm guessing that the GLib/GObject libraries don't build using SwiftPM but instead have to already be installed as system modules (in the SwiftPM sense). For those cases it would certainly make sense to allow the plugin to access files related to those libraries. It does still mean that packages that rely on this won't be able to build without preparing the host machine, but it sounds as if that's already the case with these libraries.
Am I understanding the problem correctly here? If so, perhaps a dependency on a systemModule() target that represents the installed libraries could also be exposed in the TargetBuildContext so that the plugin can find them.
Yes, the buildTool capability being proposed here are a bit more similar to Xcode's build rules feature, while the prebuild and postbuild are a bit more similar to the scheme prebuild and postbuild actions in Xcode.
I definitely see that point, but the amount of changes that would be required in SwiftPM to support having code in the manifest would mean that the feature wouldn't be available for some time (just in practical terms). The balance that this proposal tries to strike is to provide some benefits with some limitations in the short term, but to do so in a way that could be gradually extended over time. Being able to implement small plugins in the manifest would be nice, as well as letting a plugin provide type-safe option structures for clients to use. The hope is to be able to grow the support over time to allow that.
As you note, the source files of dependencies are available to the plugin, but currently the outputs defined by other plugins aren't. But this is a great point. The proposal could define that plugins are invoked in dependency order (with independent ones being allowed to be invoked in parallel, of course), and provide for a way of passing the results of those invocations on to downstream plugins.
The idea with the current proposal was that the TargetBuildContext would contain enough information to allow each plugin to construct a module mappings file for the closure of dependency targets, and the plugin would be able to write this out directly to the output directory (and not have to use a separate external command) since the information is known before any commands are run. But as you point out it might be better to allow access to the outputs of the other plugins, so that the component module mappings could be reused.
Would it be important for the plugin to be able to distinguish between original source files and those produced by another plugin? In the case of protoc this might not matter, but for different kinds of plugins (e.g. linters) there might be a need to distinguish original sources from generated sources.
Having a custom type for a file set would make sense. There should also perhaps be a way for the plugin to provide a filter for the set of input files it sees. Having access to all the files is flexible but it might make sense to specify a filename filter up-front (in the declaration of the plugin, for example).
In this proposal the plugins are invoked before the build (while the build plan is being created), so if the proposal is amended to define plugins as being invoked in dependency DAG order (and the TargetBuildContext is extended with the appropriate fields), the plugins for a target would have access to any intermediates written by plugins for its dependencies. Even as written, in this proposal the commands produced by a plugin would have access to the outputs of other commands run for the dependency targets.
So this would not work if a plugin needs to see the outputs of running build-time commands from other plugins, but it does work if, for example, the plugins themselves write out the module map files (which was the intent in the example in the proposal).
This is one of the simplifications that this proposal makes, with an eye toward being able to loosen that restriction in the future. Ideally a plugin would be able to be invoked as needed by a build system capable of doing that, and as long as the inputs and outputs of the plugin are well defined.
This initial proposal is focused on source generation, and initially any platform differences would need to be written into the generated source files using conditionals. To support tools that do actual compilation, or to allow more flexible source generation, I would expect this proposal to be extended in the future to let the plugin have access to target platform information (and to declare whether it wants to be invoked once for all the platforms, or once per platform, etc).
The current approach does let the plugins write "up-front" information that can be derived just from the configuration and structure of the target to which it's being applied and it's dependency closure (such as the module mappings). But it's a good point that it might be better to require it to use custom APIs to provide the contents of any such files, if that makes the invocation more portable. The idea would be that a plugin would always generate the same outputs (in terms of command lines defined using addCommand as well as intermediate inputs to those commands written up-front), but it might be better to extend the API to support that rather than allow the output directory to be mutable.
I don't know what systemModule is. Is it something like systemLibrary target? When I read the proposal, I made this assumption.
About GLib/GObject/GTK...
The usual way of obtaining the libraries on macOS is using brew. Brew downloads the library and installs it into /usr/local/Cellar/gtk+3/3.24.25/lib on my mac. The .gir file is than located in /usr/local/Cellar/gtk+3/3.24.25/share/gir-1.0 and is part of the downloaded package.
On previous versions of macOS, all required gir files were automatically stored at /usr/local/share/gir-1.0 , on distributions like Ubuntu, gir files are also stored in one directory. We would need SPM extensible build tools to allow us read this concrete directory.
HOWEVER!
Several users complained, that on the lates macOS 11 and computers with M1 chips, the /usr/local/share/gir-1.0 directory DOES NOT contain all required gir files.
Therefore, we need to get the gir file from the directory where library is installed by homebrew. I have came up with solution, that involves using pkg-config like this pkg-config --variable=libdir gtk+-3.0 appending path /../share/gir-1.0/. Therefore, on the new macOS 11, we would not only need access to any directory on the filesystem, we would also need the ability to initiate process and call shell/pkg-config.
I understand that this post might be a bit confusing, but I wanted to keep the post short. Let me know if you would like me to clarify something :)
Hopefully not, generally, not exposing "everything" and making them declare what they need access too keeps down what you'd have to copy if you ever did want to run these tasks remotely.
Slight tangent - if one did declare needed inputs/output, would prebuild/postbuild be needed? Or would that become implicitly based on taking no inputs or on having a wildcard for all outputs as the input?
And a second tangent - on the postbuild - is the tool allowed to modify the things in the output directory or just add things? i.e. - could it say remove unneeded localizations? In anycase, when does it run in relation to something like codesigning? I don't think I saw that mentioned.
The source generation is a good point, what about resource generation? Say a 3d library might want to have a plugin for generating textures out of images? Or maybe something to extract images/icons directly from photoshop files? This also sorta brings up something else I didn't realized until re-reading, currently all the sources are exposed to every plugin and every plugin then has to filter them. Since the target also implies a module, does this force folks to put everything for each plugin into it's own module, or should there be a way in the Package file to say which files should be sent to each plugin?
What is your evaluation of the proposal?
-0.5. It does bring value to SPM but the "scripting part" seem quite complex IMHO.
Is the problem being addressed significant enough to warrant a change to Swift?
Yes
Does this proposal fit well with the feel and direction of Swift?
IMO no.
I like the simplicity of added attributes in Package.swift but plugin scripting is about building a builder tool and not actually scripting.
I think it’s important that we keep simplicity and that we ask people to actually write a script and not define any kind of dsl/rules for the swift compiler which is the way it is going.
Also I’m not sure with current implementation plugins can be runned manually?
For instance I have a project where I generate some translations. I want to be able to run it while building and manually when I want. I think it’s important to not be tied to the compilation phase for these things.
To me we already have a library which could help us in doing what we want: Swift Argument Parser. Was there any thoughts about using (and enhancing) it to make the plugin itself? For instance taking SwiftGen example:
struct SwiftGenPlugin: PluginCommand {
let buildContext: BuildContext
var generatedSourceFiles: [FilePath] = []
lazy var prebuildGeneratedSourceDirectory: [FilePath] = [
targetBuildContext.outputDir.appending("SwiftGenOutputs")
]
func run() {
buildContext
.lookupTool(named: "swiftgen")
.execute(
arguments: "config", "run", "...", // don't really see the point of a Dictionary here, what about using Variadic?
environment: [...]
)
}
}
This would allow:
to run scripts outside of the build phase (though this would still require some logic from SPM to set the buildContext)
to have better SoC between running the script and any mandatory data (like generated source files or directories)
There could be an easy migration from “scripts” using Swift Argument Parser to this
We could give arguments both manually and through Package.swift (adding a arguments: [String:String] property)
IMHO, it's easier to read as we have a run() method
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
Used Cocoapods and NPM and it seemed simpler with these tools.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
"Lookup" is the noun or adjective, "look up" the verb. It sounds like the intention is to have a method named lookUpTool (rather than lookupTool, which implies that you're returning a "lookup tool," i.e., a tool for looking things up). Another consideration is whether this API should have a "noun" name or a "verb" name, but in either case, it wouldn't be lookupTool.
I don't know of other uses of "Dir" as an abbreviation in Swift APIs; would prefer "Directory" here.
I think this is an excellent proposal that will greatly expand the capabilities of the package manager in really important ways. Enthusiastic +1, although I do have some ideas for improvements.
Is the problem being addressed significant enough to warrant a change to Swift?
Absolutely. The proposal does a good job of illustrating some of the ways developers use tools such as source generation today. I'm generally quite averse to adding dependencies just for developer QoL features, but after trying one of these out (I think it was R.swift) a few years ago, I became convinced that they really are quite valuable. I really like the way they're able to bridge your code with your resources, and make them feel like more of a cohesive whole.
Does this proposal fit well with the feel and direction of Swift?
I think it does - Swift is all about reducing runtime errors by being smarter at build time (typically compile-time, but no reason not to extend that to the package build process).
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
I think this is easier for beginners to set up than Xcode's build phase scripts, and much more convenient for linux users, who otherwise might have to build via a bash script.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
I read the proposal and did a bit of research.
As for improvements - I think the exposed API for build tools could be improved.
Target system information (not "build target" - "target" as in host/build/target machines). When cross-compiling, a plugin may very well need to know the architecture, OS (and possibly minimum deployment... uh... target), and other information in order to generate the correct files or pass the correct flags to external tools. This API doesn't provide that, meaning developers will have to rely on build-machine information using conditionals such as #if os(macOS). Sysroot is also very useful.
File information. It's a bit unfortunate how the API globs sources, resources, and other files in the same list, with nothing more than a FilePath given to the tool. It might be nice to make this an enum, or to wrap it in a struct with some type of enum Kind property.
/// Absolute paths of the files in the target being built, including the
/// sources, resources, and other files. This also includes any source
/// files generated by other plugins that are listed earlier than this
/// plugin in the `plugins` parameter of the target being built.
var inputFiles: FileList { get }
/// Provides information about the input files in the target being built,
/// including sources, resources, and other files. The order of the files
/// is not defined but is guaranteed to be stable.
/// This allows the implementation to be more efficient than a static
/// file list.
protocol FileList: Sequence {
func makeIterator() -> FileListIterator
}
struct FileListIterator: IteratorProtocol {
mutating func next() -> FilePath?
}
Convenience APIs. I understand that this is V1 of this feature, but the API is pretty verbose - for instance, having to separately look up a tool and add the command which uses it. Lots of plugins will probably end up looking like the protobuf example - grabbing all files which match a simple pattern and passing them to a tool with a couple of flags.
It may seem like a bit of a luxury feature, but it also means plugin scripts would be easier to read and audit. Take a look at the protobuf example again, and it's hard not to notice just how much ceremony there is to construct the invocation.
That sounds quite interesting as a future direction.
One thing I've also been wondering about is if we could add support for configure scripts. Essentially recycling my idea for edit-scripts from the manifest editing commands proposal. Unfortunately, scripts are not a good fit for that particular proposal, since they would lead to a loss of non-code information such as comments - but they'd be great for making tweaks to a package manifest at build time, or even generating them entirely at build-time.
The output of the configure script would still be a Package object, which could be serialised and available for inspection by the user, but it would be a better solution for a lot of the automation use-cases that the new CLI commands attempt to help with.
Not sure if this was just an oversight, but is there a reason the public PackageDescription API does not provide a way to specify a path: sources directory?
Question about the swiftgen example in the proposal:
// swift-tools-version: 999.0
import PackageDescription
let package = Package(
name: "SwiftGen",
targets: [
/// Package plugin that tells SwiftPM how to run `swiftgen` based on
/// the configuration file. Client targets use this plugin by listing
/// it in their `plugins` parameter. This example uses the `prebuild`
/// capability, to make `swiftgen` run before each build.
/// A different example might use the `builtTool` capability, if the
/// names of inputs and outputs could be known ahead of time.
.plugin(
name: "SwiftGenPlugin",
capability: .prebuild(),
dependencies: ["SwiftGen"]
),
/// Binary target that provides the built SwiftGen executables.
.binaryTarget(
name: "SwiftGen",
url: "https://url/to/the/built/swiftgen-executables.zip",
checksum: "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
),
]
)
Is it possible for a prebuild plugin to build and run a swift target, as opposed to a binary target? or would it be necessary for the plugin to clone its associated tool from a git repository into its output directory, and then invoke swift build / swift run on its own?