Build profiles

[itowlson]

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 sure
• environment - 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 defer
• files - 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

[lann]

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.

[lann]

I need to do sockets to a debugger.

I think we should do this as a separate feature: #3153

[itowlson]

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)

[itowlson]

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.

[lann]

This brings to mind a famous quote, by Abraham Lincoln I believe...

#deep-inside-joke

[itowlson]

Yes: the trouble is that we've already rather violated President Lincoln's decree. We use serialisation types throughout Spin's application logic, and now the choice is major rework of multiple subsystems for no functional benefit, or papering over where we've coupled to a serialisation format that no longer gives us the correctness guarantees we seek. Making dangerous fields as private as possible and providing safer accessors seems like the lowest risk...

I did explore keeping Component as POD and doing something at the application level so that the components collection returned pre-profiled components, which would avoid a lot of this nonsense in favour of a single piece of once-and-for-all nonsense. It didn't really seem to work out, but I can revisit it if you feel that's a better strategy.

There, that will teach you to make light inside jokes.

[lann]

It didn't really seem to work out, but I can revisit it if you feel that's a better strategy.

Speak of the devil: https://github.com/spinframework/spin/pull/3246/files/ea595fcfd5df993eb55155409fb56eaf37f4d715#r2320229292

[lann]

Another "I wonder if it would be better" here:

Would it be reasonable to handle profiles in a new normalize_manifest pass (i.e. normalize_manifest(manifest, profile))? It would have the benefit and/or fatal flaw of hiding most of the profile logic from the rest of the downstream code.

[itowlson]

I explored this but had trouble getting it to work safely. This may point to a deeper problem of normalisation not being sufficiently embedded in the loader logic, but again this is a risk of world+dog being able to go "eh I'll just deserialise from TOML." The loader pipeline goes from TOML to LockedApp but there are things in the dev tooling that care about AppManifests and they tend to bypass the correct load sequence because it's not readily accessible. But like I say this may be an opportunity to improve the load sequence and make better guarantees, it will just likely be more work.

[lann]

Could you say more about the troubles here? I can see a few potential problems along those lines but it isn't clear to me where things would be worse by normalizing in spin-manifest rather than spin-loader.

[itowlson]

If I remember rightly, the trouble was that some places were deserialising manifests rather than asking a loader to do it for them (because the loader produced a LockedApp and they wanted a Manifest). And this is hard to stop because deserialisability is a public feature. You need to make the DTO private and force consumers to go through a loader which can then enforce normalisation. (But some things do have a legit interest in the raw manifest. So, choices, choices.)

Let me look again. I didn't spend much time on it and my memory of what I was trying is not great.

[lann]

I'm reasonably confident this is equivalent (and less-not-inarguably better):

Suggested change

	let is_match = match (profile, latest_build) {
	(None, None) => true,
	(Some(_), None) | (None, Some(_)) => false,
	(Some(p), Some(latest)) => p == latest,
	};
	if !is_match {
	if profile != latest_build {

[itowlson]

FACEPALM

Thank you!

[lann]

Not inarguably better, but another option:

Suggested change

	if last_build_str == LAST_BUILD_ANON_VALUE {
	None
	} else {
	Some(last_build_str)
	}
	(last_build_str != LAST_BUILD_ANON_VALUE).then_some(last_build_str)

[itowlson]

Maybe it's too much Go programming but I find this a bit obscure and would prefer to keep spelling it out. Appreciate the education though - just give me a couple of years to internalise it!

[lann]

🚲🏠

Suggested change

	const LAST_BUILD_ANON_VALUE: &str = "<anonymous>";
	const LAST_BUILD_NONE_VALUE: &str = "<none>";

[itowlson]

It's not necessarily "none" (which to me implies no last build). It's usually "the anonymous profile."

[lann]

Ah ok that makes sense.

[lann]

The only comment here rising to the level of "maybe-blocking" is the one about logging errors. The rest are minor nits/suggestions.

[lann]

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.

[tschneidereit]

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.

[itowlson]

@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.

[itowlson]

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.

[itowlson]

@tschneidereit Comparison to the draft SIP (#3075):

• No support for SPIN_PROFILE environment variable
• No support for selecting the debug profile with --debug
• No aliasing of release to 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 environment and dependencies
• Profiles are not 'atomic' - all overrides are optional
• Heuristic check for "is this profile's build current"

[tschneidereit]

• No support for SPIN_PROFILE environment variable
• No support for selecting the debug profile with --debug
• No aliasing of release to 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 environment and dependencies

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.

[itowlson]

• 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.

[tschneidereit]

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"