Skip to content

DIP17: describe native build extensions #57

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.

Sign up for GitHub

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

Open
philpax wants to merge 2 commits into
base: main
Choose a base branch
from
+26 −1
Open

DIP17: describe native build extensions #57

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a6419e
DIP17: describe native build extensions
philpax Nov 10, 2022
2f0e758
DIP17: update native extensions with comments
philpax Nov 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion text/17-automated-build-and-submit-pipeline.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,19 @@ changelog = "added Herobrine"
version = "1.42.0"
```

The following are optional extensions to allow for more complicated build scenarios, including native code:

```toml
[build]
image = "extended"

[[build.needs]]
type = "file"
url = "some_file_url"
dest = "filename"
sha512 = "sha512_hash_of_file"
```

For this to work, the repository must be structured to be amenable to automatic builds and packaging, and it must be accessible to the CI pipeline (which in turn means it must be accessible to the public). Details of this will be covered in [reference-level-explanation].

The `owners` array controls which GitHub accounts are allowed to publish updates for this manifest:
Expand All @@ -70,6 +83,12 @@ owners = [
]
```

The `build.image` property refers to the Docker image used for building. By default, this is a minimalistic image that can build C# code; the `extended` image includes MSVC build tools and Wine, allowing it to build native C++ code. Future images may be made available to build other code. The extended image is defined [here](https://github.com/goatcorp/plogon-builder). There are currently two supported images, hardcoded in [Plogon](https://github.com/goatcorp/Plogon); a formal mechanism for this may be added at some point. This image is unversioned, but this will be revisited when required.
Copy link
Copy Markdown
Member

@KazWolfe KazWolfe Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So long as the image is unversioned, we need to be careful to not introduce any breaking changes. Until such time as we have a system to version build images, may it make sense to guarantee that updates to the base image coincide with Dalamud API bumps?

Copy link
Copy Markdown
Contributor Author

@philpax philpax Nov 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goaaats Do you have any problems with that? My concern is that we might need to update the image at a time that doesn't coincide with an API bump, and we're left in the unfortunate situation of either holding back a plugin or forcing an API bump on everyone else.

My feeling is that if we run into issues with breaking changes, we'll just revert the relevant change and either a) make a new image or b) add some kind of versioning scheme (which could be as simple as creating extended-v2 or something while we get the rest of our ducks in line)


The `build.needs` array property describes files that should be downloaded by the build agent and made available to the build environment, ala NixOS or similar build environments. An arbitrary number of files can be downloaded, but this will be checked by members of the review team. This is intended for direct dependencies required for your build script. These files are downloaded before the rest of the build occurs.
Copy link
Copy Markdown
Member

@KazWolfe KazWolfe Nov 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review team specifically should also encourage the use of clearly-defined URLs that point to a specific version (e.g. don't use https://get.myapp.io/download/sdk).

Files or URLs that seem to have a high tendency to break should be rejected by the review team. This does pose a bit of a problem though, as certain providers don't necessarily like "playing nice" and providing versioned files - we'd likely need to recommend users upload these SDK versions to their own file store (or something else?).


If the plugin requires additional non-C# code to be built (i.e. for the purposes of building native or web code), a `plogonbuild.sh` file can be created in `project_path`. This is specified further in the [reference-level explanation][reference-level-explanation].

This DIP also proposes moving all properties associated with `plugin.json` (formerly `$plugin_name.json`) into the project's `csproj` (or equivalent project file, including `fsproj`), and having the `plugin.json` be generated by DalamudPackager using the information within the `csproj`. Developers would still be able to maintain their own `json` if they really want to, but the new process should be easier (only one place to update all project-related metadata).

This change would result in a new section being added to all `csproj`s that choose to participate. An example is provided of what this change would look like for GatherBuddy:
Expand Down Expand Up @@ -142,9 +161,13 @@ Developers should add a DalamudPackager step to their `csproj`. It will generate

Developers are also encouraged to ensure that the `csproj` is only responsible for building the plugin, and does not build anything unrelated (normal dependencies are fine). This may require developers to partition their project, or use a conditional build step. Additionally, if the plugin has a build-time dependency on a non-.NET build (e.g. JavaScript assets), it is best if that dependency is built ahead of time and the result committed to the repository, as the CI will not be able to build it.
Copy link
Copy Markdown
Member

@KazWolfe KazWolfe Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clarify this to better suit the below additions - would we rather assets be built in plogonbuild.sh or still pre-baked?

Copy link
Copy Markdown
Contributor Author

@philpax philpax Nov 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, pre-baked, depending on the type of asset. Rule of thumb, I guess: if it can be built with the tools available in the image, it's probably fine, but if it can't, then it should be prebaked. If uncertain, ask goat or another build system tech.


As new plugins have been introduced that include native components (i.e. C++ or other such languages), the build agent will also execute a `./plogonbuild.sh` script located at `project_path` to produce any native artifacts. As these native components are likely to require the MSVC toolchain, it is recommended that you use the `extended` build image, as described above. An example of a build script can be seen in the [xivr repository](https://github.com/ProjectMimer/xivr/blob/main/dip17build.sh).
Copy link
Copy Markdown
Member

@KazWolfe KazWolfe Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we would allow users to specify build scripts as part of the manifest to allow for more flexibility in the process, potentially allowing caching of steps, and similar. This would also give developers more flexibility in deciding where to place files (e.g. those that wish to have .plogon/build.sh) as well as generally limiting the amount of hardcoded things for us to deal with.

This, however, is a very low priority need and would be more of a wishlist item than anything. The defined build script may already call other scripts as necessary, rendering a viable workaround.

Copy link
Copy Markdown
Contributor Author

@philpax philpax Nov 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not going to say "no", but I do think that's potentially overcomplicating things. To some extent, if you need multiple script files, there might be other issues that need to be resolved?

Re caching of steps: that's a decent point. For now, leaning towards "call make in your build script" and revisiting this at a later stage once we have a better idea of how it's being used.

Copy link
Copy Markdown
Member

@KazWolfe KazWolfe Nov 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason for "overcomplication" and allowing multiple scripts is to give developers flexibility and freedom in structuring their repositories in a way that best suits their purposes, as well as allow certain components to be more isolated (useful for testing). Granted, this can be done by having the script we call also call subscripts, but then we get back into the entire "hardcoded paths thing." This doesn't have to be solved now, however.


This script runs before MSBuild builds the rest of your project. It has no network access, so you must use `build.needs` to fetch any relevant dependencies. The script is intended to build any direct dependencies of your C# code (i.e. native code).

To encourage CI compatibility, as well as making it easier to test, a goatcorp-blessed GitHub Action will be made available that plugin developers can include as part of their repository. This action will build the package, similarly to how the CI would do it, so that developers can test locally with [act](https://github.com/nektos/act) or similar. This action will be made part of the existing plugin samples/templates, so that a user creating a new plugin will be automatically ready to go with CI. This action should be the same as the action run by the CI, or as close as possible.

Finally, build successes and/or failures will be published to the #github Discord channel, or its equivalent, to keep plugin developers in the loop. This is not a strict requirement, and may be adjusted to ensure that the signal-to-noise ratio stays high.
Finally, build successes and/or failures will be published to the #plugin-stream channel to keep plugin developers in the loop. This is not a strict requirement, and may be adjusted to ensure that the signal-to-noise ratio stays high.

# Drawbacks

Expand Down Expand Up @@ -265,3 +288,5 @@ This would form the backbone of any future CI we do around plugin submission. Id
- This is not an immediate problem as the plugin security model is currently too open to allow for it, anyway. It's something we can revisit in a future DIP.
- This RFC does not currently describe a way to disable a plugin. The metadata structure could be extended with an `enabled: bool` that will govern whether or not the plugin is blacklisted, thus allowing both goatcorp and plugin developers to blacklist plugins with ease.
- This RFC does not address automated building and deployment for plugins with hidden source or secrets, which will continue to submit to DalamudPlugins. This should be addressed at some point to allow plugins under these categories to submit. The current solution in mind for secrets is for the secret to be provided to goatcorp, who will then supply it to the GitHub runner, but this requires more thought. For hidden source plugins, we may ask developers to provide access to their repository for the buildbot, but we will need to ensure that this is done securely.
- The existence of a build script, as well as the ability to download network dependencies, suggests a potential future extension of secrets encrypted with a goatcorp public key and decrypted by a private key provided by the build agent.
Comment on lines 290 to +291
Copy link
Copy Markdown
Member

@KazWolfe KazWolfe Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultimately, closed-source plugins need to go away. At present, the two (known) closed-source plugins have security concerns related to API keys, which is reasonable but also problematic.

I like the idea of our images having a known GPG key loaded into its keychain (stored as a secret in our runner configs), and then allowing users to specify whatever decryption process as part of their pre-build steps. This will, of course, not solve the issue of the final build artifact leaking whatever was created, but that's the nature of (trusted) client-side code. Plugins that require secure connections to backend servers are, as always, advised to do account or session management themselves.

Copy link
Copy Markdown
Contributor Author

@philpax philpax Nov 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree on all of this. Feel free to propose a follow-up extension in another PR - I don't know enough about how to securely handle secrets in a scenario like this

- The build images are currently hardcoded. It would be nice to define these more systematically so that anyone can contribute images. (PRs welcome!)