Spin Improvement Proposal: Supporting multiple build profiles#3075
Spin Improvement Proposal: Supporting multiple build profiles#3075itowlson merged 4 commits intospinframework:mainfrom
Conversation
This PR adds a SIP describing changes that'd allow developers to define and use multiple build profiles for components in a `spin.toml` file Signed-off-by: Till Schneidereit <till@tillschneidereit.net>
fibonacci1729
left a comment
fibonacci1729
left a comment
There was a problem hiding this comment.
LGTM; proposal is quite simple and seems very straightforward to implement.
| The application defined in this manifest can be run in various configurations: | ||
|
|
||
| - `spin up`: uses the release/default builds for everything | ||
| - `spin up --profile debug`: uses builds of the profile named `debug` of all components that have them, default builds for the rest |
There was a problem hiding this comment.
My worry here is the disconnection between the build and run steps. I know that in three months I will be tweaking an app and wondering why my changes aren't showing and it will be because I am building release and running debug or something. This was one of the concerns that stopped us doing this before (maddeningly, I can no longer find the discussion, sorry). Do you believe this is a false consideration, or do you have ways to mitigate it?
There was a problem hiding this comment.
yeah, that's a very good point. One, and perhaps the only real one, option is to make spin up imply spin build.
This is the behavior cargo run has, and it makes sense to me. Cargo of course has the benefit of having much deeper understanding of what a build implies, but maybe that's not actually key here?
There was a problem hiding this comment.
spin up isn't like cargo run though. It's more like wasmtime run. It cares only about the presence of the binary; its job is to get the binary up and running as quickly as possible.
(At least that's how it is today. We can, of course, change that; but making it default to building would be a breaking change for certain scenarios. Which is not a blocker, but we'd want to socialise it well in advance of the change.)
I'd be concerned about a philosophy of "build everywhere." Okay, I built it to make sure that it typechecked and was warning free, now let me run it, huh, looks like it built again. Push it to a registry, built again; deploy it, built again. For sure Rust components will fairly quickly (and fairly quietly) figure out that the build is a no-op. But rebuilding a single minimal JS component (with no changes) takes more than half a second on my computer, and produces 2/3 of a screen of npm spew Maybe that's an extreme case but it does give me collywobbles about defaulting to rebuilding with every single operation. I do appreciate the consistency guarantees it provides though - after all, even today we have a risk of "oops I forgot to build before pushing"!
Signed-off-by: Till Schneidereit <till@tillschneidereit.net>
tschneidereit
left a comment
tschneidereit
left a comment
There was a problem hiding this comment.
Thank you very much for the detailed feedback! I think I addressed all of it, but some of the ways I did probably warrant further discussion
|
|
||
| 1. Adding an optional `[component.name.profile.profile-name]` table to use for defining additional build profiles of a component | ||
| 2. Adding a `-profile [profile-name]` CLI flag and `SPIN_PROFILE` env var to `spin {build, watch, up}` to use specific build profiles of all components (where available, with fallback to the default otherwise) | ||
| 1. By popular demand, the `debug` profile can be selected using the `--debug` CLI flag |
There was a problem hiding this comment.
While I added the --debug flag for now, I think there might be a reason for not doing so after all: we might want to later add support for actual debugging to Spin, and would in that case probably regret having used this flag for profile selection.
There was a problem hiding this comment.
@tschneidereit Ah, yep. Reserving --debug for "I want to actually attach a debugger" is a good point. So spin up --build --debug would build the debug profile and run under the debugger; but spin up --build --profile=debug would build the debug profile but run it without actually debugging. The first case does feel it should (eventually) be the "common case" and get the fancy flag.
There was a problem hiding this comment.
Just throwing in my perspective that attaching the debugger sounds to me like the special case, and I would expect --debug to only refer to the debug profile, but that's likely due to my perspective as a Rust programmer.
|
|
||
| 1. Adding an optional `[component.name.profile.profile-name]` table to use for defining additional build profiles of a component | ||
| 2. Adding a `-profile [profile-name]` CLI flag and `SPIN_PROFILE` env var to `spin {build, watch, up}` to use specific build profiles of all components (where available, with fallback to the default otherwise) | ||
| 1. By popular demand, the `debug` profile can be selected using the `--debug` CLI flag |
There was a problem hiding this comment.
Just throwing in my perspective that attaching the debugger sounds to me like the special case, and I would expect --debug to only refer to the debug profile, but that's likely due to my perspective as a Rust programmer.
| 1. Adding an optional `[component.name.profile.profile-name]` table to use for defining additional build profiles of a component | ||
| 2. Adding a `-profile [profile-name]` CLI flag and `SPIN_PROFILE` env var to `spin {build, watch, up}` to use specific build profiles of all components (where available, with fallback to the default otherwise) | ||
| 1. By popular demand, the `debug` profile can be selected using the `--debug` CLI flag | ||
| 2. `release` is the default profile, but can also be explicitly named for regularity, including with the `--release` alias |
There was a problem hiding this comment.
Is the reason for making release the default, for backwards compatibility reasons? The Cargo model of debug being the default makes most sense to me, but I'm certainly not without bias in this regard 😄 .
There was a problem hiding this comment.
I like release being the default in part because spin build then spin registry push is a promoted flow and i like orienting around defaulting to pushing release builds
|
|
||
| A profile can be selected for the entire application in three ways: | ||
| 1. Via the `--profile=[profile-name]` CLI flag to `spin {build, up, watch}` | ||
| 2. Via the `--debug` CLI flag to `spin {build, up, watch}` as an alias for `--profile=debug` |
There was a problem hiding this comment.
You mention a --release CLI flag but not here. I'm guessing that's just an oversight?
|
I believe we have merged the feature in #3246 but the SIP has not landed yet. It also seems the implementation diverged from the SIP — would it be possible to update the SIP and merge it so we have a record of what we shipped? Thanks! |
|
Cc/ @karthik2804 are you working on bringing the SIP up-to-date? |
|
@fibonacci1729 yes I am. |
Signed-off-by: Karthik Ganeshram <kganeshr@akamai.com>
|
|
||
| # Proposal | ||
|
|
||
| This very simple proposal consists of four parts: |
There was a problem hiding this comment.
for small values of 'four' grin
|
|
||
| --- | ||
|
|
||
| Summary: Support defining multiple build profiles for components, and running a Spin application in with those profiles. |
There was a problem hiding this comment.
| Summary: Support defining multiple build profiles for components, and running a Spin application in with those profiles. | |
| Summary: Support defining multiple build profiles for components, and running a Spin application with those profiles. |
| The application defined in this manifest can be run in various configurations: | ||
|
|
||
| - `spin up`: uses the release/default builds for everything | ||
| - `spin up --profile debug` or `spin up --debug`: uses builds of the profile named `debug` of all components that have them, default builds for the rest |
There was a problem hiding this comment.
| - `spin up --profile debug` or `spin up --debug`: uses builds of the profile named `debug` of all components that have them, default builds for the rest | |
| - `spin up --profile debug`: uses builds of the profile named `debug` of all components that have them, default builds for the rest |
(I think we deferred the spin up --debug thingy to allow for more thinking)
|
|
||
| - `spin up`: uses the release/default builds for everything | ||
| - `spin up --profile debug` or `spin up --debug`: uses builds of the profile named `debug` of all components that have them, default builds for the rest | ||
| - `spin up --component-profile sentiment-analysis=debug`: uses the debug build of just the `sentiment-analysis` component, default builds for everything else |
There was a problem hiding this comment.
I didn't do this. It can be a future enhancement if needed.
|
|
||
| ## Details of profile selection | ||
|
|
||
| A profile can be selected `--profile=[profile-name]` CLI flag to `spin {build, up, watch, deploy}` |
| This proposal is focused on build configurations. As such, the fields that are supported in `[component.component-name.profile.profile-name]` tables are limited to: | ||
|
|
||
| - `source` | ||
| - `command` |
|
|
||
| - `source` | ||
| - `command` | ||
| - `watch` |
There was a problem hiding this comment.
Pretty sure you can't do this
|
|
||
| [component.sentiment-analysis.profile.debug.build] | ||
| command = "npm run build:debug" | ||
| watch = ["src/**/*", "package.json", "package-lock.json"] |
There was a problem hiding this comment.
I don't think this is allowed in a profile
There was a problem hiding this comment.
This is specifically in reference to the watch right?
There was a problem hiding this comment.
Correct. Looking at the code, the only override in build is the command. You can't override the working directory or watch list (because there seems no reason for those to vary across profiles - you're building from the same things, just in different configurations).
| This very simple proposal consists of four parts: | ||
|
|
||
| 1. Adding an optional `[component.name.profile.profile-name]` table to use for defining additional build profiles of a component | ||
| 2. Adding a `-profile [profile-name]` CLI flag to `spin {build, watch, up, deploy}` to use specific build profiles of all components (where available, with fallback to the default otherwise) |
There was a problem hiding this comment.
SUPER MEGA NIT
| 2. Adding a `-profile [profile-name]` CLI flag to `spin {build, watch, up, deploy}` to use specific build profiles of all components (where available, with fallback to the default otherwise) | |
| 2. Adding a `--profile [profile-name]` CLI flag to `spin {build, watch, up, deploy}` to use specific build profiles of all components (where available, with fallback to the default otherwise) |
| This very simple proposal consists of four parts: | ||
|
|
||
| 1. Adding an optional `[component.name.profile.profile-name]` table to use for defining additional build profiles of a component | ||
| 2. Adding a `-profile [profile-name]` CLI flag to `spin {build, watch, up, deploy}` to use specific build profiles of all components (where available, with fallback to the default otherwise) |
There was a problem hiding this comment.
also registry push (but maybe this intro section doesn't need to get specific about individual commands)
|
|
||
| # Proposal | ||
|
|
||
| This very simple proposal consists of four parts: |
There was a problem hiding this comment.
also ULTRA NIT but I hate it when writers tell me that the thing is simple. If it's simple, I'll figure it out for myself. If it's not, you've made me feel stupid for not getting it. I suppose it's meant to be reassuring but fie!
anyway now back to your regularly scheduled punctuation complaints
Signed-off-by: Karthik Ganeshram <kganeshr@akamai.com>
7 participants
This PR adds a SIP describing changes that'd allow developers to define and use multiple build profiles for components in a
spin.tomlfile