fermyon
Build profiles
Incomplete draft for discussion. This diverges from Till's draft SIP in some ways, mostly cosmetic and open to change; but also somewhat in preferring to constrain variation between profiles.
Example:
[component.build-profile-test]
source = "target/wasm32-wasip1/release/build_profile_test.wasm"
profile.debug.source = "target/wasm32-wasip1/debug/build_profile_test.wasm"
allowed_outbound_hosts = []
[component.build-profile-test.build]
command = "cargo build --target wasm32-wasip1 --release"
profile.debug.command = "cargo build --target wasm32-wasip1"
watch = ["src/**/*.rs", "Cargo.toml"]
This proposal uses a partial approach to the possible "am I running the same profile I built" problem. When you do a build, it records the built profile in a "last profile built" file. When you do a run, it checks if the profile being run matches the last built one, and prints a warning if not. This is certainly far from reliable: if you build both debug and release profiles, and then run both of them, you'll get a spurious warning.
If we're okay with this as a basis for further development, we need to look at what other fields should be allowed to vary by profile, e.g.:
allowed_outbound_hosts- you should be able to specify additional hosts. I'd be inclined not to say you can specify an entirely separate collection, because the 90% case is going to "I need to do sockets to a debugger." (Things like "I want to talk to the dev database not the prod database" should be handled by variables.)dependencies- should be able to replace a dependency, e.g. to use its debug build or a stub/mock. If I squint I can imagine use cases for extra dependencies, in case a component has debug-only imports, not sureenvironment- should be able to replace an EV or set additional EVs- KV/SQLite/LLM - my inclination that varying these by profile is probably an anti-pattern but I am open to being talked round if someone has a concrete use case - otherwise propose deferring to later
variables- not clear why you would, I guess there could be a knob to control profile-specific behaviour? Propose deferfiles- not clear why you would, but I guess I could imagine a special build needing some additional asset? Again my inclination would be to defer until someone needs it
Possible further features:
- whole components being conditional on the profile - e.g. include KV explorer only in certain build configs
- this implies conditional triggers and I am not sure how much I love where this is going
Anyway here it is for discussion, please be gentle with me
This diverges from Till's draft SIP in some ways, mostly cosmetic and open to change; but also somewhat in preferring to constrain variation between profiles.
I think I prefer the SIP's TOML structure with everything for component X / profile Y going under component.X.profile.Y. I agree that we should constrain variation between profiles regardless of the structure.
I need to do sockets to a debugger.
I think we should do this as a separate feature: https://github.com/spinframework/spin/issues/3153
Updated with reworked manifest:
[component.build-profile-test]
source = "target/wasm32-wasip1/release/build_profile_test.wasm"
allowed_outbound_hosts = []
[component.build-profile-test.build]
command = "cargo build --target wasm32-wasip1 --release"
watch = ["src/**/*.rs", "Cargo.toml"]
[component.build-profile-test.profile.debug]
source = "target/wasm32-wasip1/debug/build_profile_test.wasm"
build.command = "cargo build --target wasm32-wasip1"
I dislike the verbosity and repetition of the way TOML does this. It takes a lot to make me yearn for JSON but man this comes close. But the clarify benefit of the grouping overrides the verbosity issue.
(again, will do squashes once we have agreement on the approach)
Done env vars and deps, holding off on allowed hosts: if we can do the special-case thing for debug connections, then I reckon we can defer allowed hosts in general until someone comes up with a use case.
This comment helped clarify the design a bit and brings up a brace of questions:
- Should last profile be tracked per-component?
spin build --component-id <component-id>could cause this to be different for different components. - Should build failures be recorded as a separate "last build" value? In general if a build fails you can't know if the build state represents the old profile, the new profile, or something else entirely.
If we don't want to address these problems directly we could maybe mitigate them in common cases by 1) removing last-build.txt immediately before builds and 2) not writing last-build.txt unless all components were built.
I haven't done a real code review, and certainly shouldn't be the one to sign off on all implementation aspects. With that said: this looks great, and I'm very excited we'll soon have profiles support! 🥳
Could you perhaps go through the SIP and comment on any differences between what's proposed there and implemented here? I think that'd make it clearer where we are, and move design discussions, if any, to the right venue.
@lann The "last build" is a heuristic effort to mitigate the "I built Profile A then ran Profile B and none of my changes are showing up" problem discussed on the SIP (without either 1. adopting the "always build" nuclear option or 2. building a whole series of file scans and timestamp checks). It's certainly not perfect (e.g. I built Profile A, then built Profile B, ran Profile A - I get the warning even though Profile A is still up to date) but the intention is just to give folks a heads up in the commonest mistake case.
My inclination is that if folks are doing selective builds then they're likely thinking about their build-run cycle enough that they'll be conscious of "oh these things might be out of sync," so I'm not greatly inclined to make things more complicated in order to support that. I'd propose that if users find it to be a problem then we can address it in a follow-up.
The point about failed builds is valid - we could have built 9/10 components in Profile B, then failed on the last 1/10, leaving the file saying Profile A. Again, though, my inclination is that in a failed build situation you have a mix of what is current anyway, so a gentle warning is likely to be the least of your surprises. But maybe I'm taking an overly simplistic view here.
I just re-read my previous message and noticed "The point about failed builds is valid" which I realised could come across as "not like your other point," which was not my intention. Both of your points were good. Sorry about that.
@tschneidereit Comparison to the draft SIP (https://github.com/spinframework/spin/pull/3075):
- No support for
SPIN_PROFILEenvironment variable - No support for selecting the
debugprofile with--debug - No aliasing of
releaseto the anonymous profile - No support for "profile per component" (
--component-profile) - No automatic release build before registry push
- Allows pushing non-default profile
- Allows override of
environmentanddependencies - Profiles are not 'atomic' - all overrides are optional
- Heuristic check for "is this profile's build current"
- No support for
SPIN_PROFILEenvironment variable- No support for selecting the
debugprofile with--debug- No aliasing of
releaseto the anonymous profile- No support for "profile per component" (
--component-profile)
These all seem okay to omit, yeah.
- No automatic release build before registry push
This I feel quite strongly is important to do: I'm almost entirely certain that I myself would get this wrong and push debug builds at some point in the future.
- Allows pushing non-default profile
By "allows" do you mean "will push whatever was built last", or "adds a --profile option to the CLI subcommand"? If the latter, I think that's actively good, in particular for workflows involving test deployments. If the former, see my comment above.
- Allows override of
environmentanddependencies
Excellent!
- Profiles are not 'atomic' - all overrides are optional
I keep going back and forth on this one, which probably means that either version is okay. Non-atomic profiles are more ergonomic for sure, whereas atomic ones feel a bit less foot-gunny to me. But I can't really articulate all that well why, so let's ignore the FUD.
- Heuristic check for "is this profile's build current"
We should make sure that that heuristic is well documented somewhere, lest our future selves have no idea what is going on and make incompatible changes.
All in all, I think only the build-before-push thing needs (more than documentation) changes, or more discussion. I can update the SIP for the other things.
- Allows pushing non-default profile
By "allows" do you mean "will push whatever was built last", or "adds a --profile option to the CLI subcommand"? If the latter, I think that's actively good, in particular for workflows involving test deployments. If the former, see my comment above.
It adds a --profile option to spin registry push. The default remains to push the anonymous (presumably release) profile.
- Profiles are not 'atomic' - all overrides are optional
I keep going back and forth on this one, which probably means that either version is okay. Non-atomic profiles are more ergonomic for sure, whereas atomic ones feel a bit less foot-gunny to me. But I can't really articulate all that well why, so let's ignore the FUD.
Adding environment and dependency overrides forced non-atomicity. And then it sorta kinda made sense to me that a profile might involve only overriding a dependency or EV, and not change the binary at all. I agree it's a matter of taste - if we see it causing problems we can be more forceful about it in the next major version.
And then it sorta kinda made sense to me that a profile might involve only overriding a dependency or EV, and not change the binary at all.
Yeah, I agree that that makes sense. One thing we could consider is to introduce an explicit inherits property like cargo has, so one is forced to choose which profile to inherit stuff from.
That does seem like a nice thing to have, honestly. The major issue I'd see is that it only really fully works if it's a required field, as in cargo. Which raises the question of how to refer to the anonymous profile.
And that, in turn, brings me to another point: I think instead of anonymous calling that profile default might be better? That is what it effectively is, since it's used if not profile is mentioned. And inherits = "default" sure looks better to me than inherits = "anonymous"