The review of SE-0303: Package Manager Extensible Build Tools has concluded and the core team has returned it for revision.
The feedback to idea of extending SwiftPM with plugins, the concrete design of the plugin system, and the tradeoffs the design make was very positive. Both authors and users of potential future plugins have provided input that the proposed design would work in real-world use cases, and that today require solutions outside of SwiftPM. However, there were a number of ideas that came up during the review that the core team feels would be good to adopt.
-
The proposed package manifest syntax does not offer a structured way to configure the plugins added to the package. The underlying technical challenge is that in order to support type-safe configuration, the package manager will need to compile & load the plugins prior to be being able to parse the manifest (since its Swift code), causing a chicken-and-egg issue.
A more complete solution would be to restructure the package manifest to multiple top level "stanzas" (or even multiple files): the plugins "build" and the application "build", which can then drive a two pass build solving the chicken-and-egg problem. Such solution will be significantly more complicated to implement, and requires changes to the build system.
Some reviewers suggested using a non type-safe configuration (e.g. a dictionary) to work around the issue, but this is likely the worst of both world: compromises on user experience and type-safety and paints the API into a corner given SwiftPM’s backwards compatibility requirement.
The solution the authors picked was to make this a plugin concern - in practice a plugin has access to the file system and as such can ask users to define configuration via a configuration file following some naming convention, etc.
Given that the more complete two-pass build solution would not change the core plugin API, the core team felt the proposal makes the right trade-off, but would like to see more details on this topic in the "future directions" section.
-
Technically related to the above, several reviewers pointed out that this having a single dependencies stanza implies the plugins and the package share the same version of the dependencies which could lead to undesirable revlock side-effects. Here too, the more complete solution is to restructure the package manifest to multiple top level "stanzas" (or even multiple files), as described above.
Similarly, given that the more complete two-pass build solution would not change the core plugin API, the core team felt the proposal makes the right trade-off, but would like to see more details on this topic in the "future directions" section.
-
The proposal describes that plugins gain access to the file system using simplistic file system abstraction, namely array of
FilePath(from SwiftSystem). Bazel's depset was mentioned as an alternative for building up input file lists and the arguments for constructing shell commands, which seems like an interesting alternative. Several reviewers pointed our that the plugins API should provide something more useful, for example the type/category of the files, and some filtering/globbing capabilities to make the plugin authors job easier.The core team felt this is one area where the proposal can be improved and encourages the authors to explore the ideas suggested. The proposal should also explicitly call out any future ideas for this API in the "future directions" section.
-
The proposed API does not provide contextual information about the target platform, which limits the API usefulness for building platform-specific plugins.
The proposal authors deliberately left this feature for future iterations to control the scope of initial design and implementation. They felt it can be added later, and should not block progress in supporting useful use cases that do no required this capability.
The core team agreed this can be a good future addition and as such would like to see more details on this topic in the "future directions" section.
-
The proposal describes 3 initial plugin types:
prebuild,postbuild, andbuildTool, defined as the following:prebuild: runs before the build (like a script you run before the build), designed for plugins that do not know the exact inputs/outputs, but may be inefficient because of that (e.g. incremental builds)postbuild: runs after the build was complete (like a script you run after the build), designed for plugins that do not know the exact inputs/outputs, but may be inefficient because of that (e.g. incremental builds)buildTool: runs as part of the build, designed for plugins that can enumerate the inputs and outputs, runs as part of the build graph efficiently.Out of the three, postbuild received the least attention and seems to be the least useful. Many reviewers in both the pitch and proposal has a hard time grokking the difference between
prebuildandbuildTooland the trade-offs of picking one over the other.The core team felt the proposed design is not clear enough in that respect, leading to such confusion. It also made the observation that the separation between
prebuildandbuildToolreflects current limitation of the underlying build tools, not the desired API for when such constraints are lifted. As such, the core team would like to see this area of the design further refined, including:a. Consider removing postbuild given that its utility is unclear.
b. Consider merging prebuild and buildTool into a single, intuitive API, or provide better guidance on how to choose one over the other.