Skip to content
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.

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

Implementation of Component Dependencies #2673

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

fibonacci1729
Copy link
Contributor

@fibonacci1729 fibonacci1729 commented Jul 22, 2024

This PR implements the Component Dependencies SIP.

See here for a simple example.

There are a few remaining items to address, namely better error messages and some lingering documentation TODOs. Additionally i'd like to put a few examples together.

@lann lann marked this pull request as draft July 22, 2024 21:27
@fibonacci1729 fibonacci1729 force-pushed the component-dependencies branch 6 times, most recently from d01a047 to 92efd6d Compare July 22, 2024 22:35
@itowlson

This comment was marked as resolved.

@fibonacci1729

This comment was marked as resolved.

@fibonacci1729 fibonacci1729 force-pushed the component-dependencies branch 2 times, most recently from 9cba0fc to f64cdd6 Compare July 23, 2024 02:05
Copy link
Collaborator

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Looking like a really great start! 🎉

crates/common/src/dependencies.rs Outdated Show resolved Hide resolved
crates/common/src/dependencies.rs Outdated Show resolved Hide resolved
crates/compose/src/lib.rs Outdated Show resolved Hide resolved
crates/compose/src/lib.rs Outdated Show resolved Hide resolved
crates/compose/src/lib.rs Outdated Show resolved Hide resolved
crates/compose/src/lib.rs Outdated Show resolved Hide resolved
@fibonacci1729 fibonacci1729 force-pushed the component-dependencies branch 11 times, most recently from 3ee065c to eaa0280 Compare July 23, 2024 23:53
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

A few random and not very organised comments - I hope it's not too early for them, feel free to ignore if it is! I'm very much enjoying playing with the new feature. Pretty slick!

crates/locked-app/src/locked.rs Outdated Show resolved Hide resolved
crates/common/src/dependencies.rs Outdated Show resolved Hide resolved
crates/trigger/src/loader.rs Outdated Show resolved Hide resolved
crates/manifest/src/schema/v2.rs Outdated Show resolved Hide resolved
crates/manifest/src/schema/v2.rs Show resolved Hide resolved
crates/manifest/src/schema/v2.rs Outdated Show resolved Hide resolved
crates/manifest/src/schema/v2.rs Show resolved Hide resolved
crates/trigger/src/loader.rs Outdated Show resolved Hide resolved
@fibonacci1729 fibonacci1729 force-pushed the component-dependencies branch 6 times, most recently from 1b27140 to bb1f0af Compare July 26, 2024 18:04
@fibonacci1729 fibonacci1729 force-pushed the component-dependencies branch from fb7ed10 to 617803b Compare July 30, 2024 18:51
@fibonacci1729 fibonacci1729 requested review from lann, rylev and itowlson July 30, 2024 19:05
crates/compose/Cargo.toml Outdated Show resolved Hide resolved
crates/compose/deny-all/src/fermyon.rs Outdated Show resolved Hide resolved
crates/compose/src/lib.rs Outdated Show resolved Hide resolved
crates/compose/src/lib.rs Outdated Show resolved Hide resolved
crates/compose/src/lib.rs Show resolved Hide resolved
crates/manifest/src/schema/v2.rs Outdated Show resolved Hide resolved
crates/compose/deny-all/src/wasi.rs Outdated Show resolved Hide resolved
@fibonacci1729 fibonacci1729 force-pushed the component-dependencies branch from 617803b to 22afb6a Compare July 31, 2024 18:44
Copy link
Collaborator

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Looking good! I'm fine with merging as is.

I do have one piece of feedback about the user experience, that I'd like to discuss. In the spin.toml manifest, the component dependencies keys are somewhat treated like patterns where a "org:package" = { path = "/path/to/wasm.wasm" } means that all imports that have "org:package" as their org/package namespace will be satisfied by that dependency. I wonder if we would be better of with a more explicit pattern matching syntax like "org:package/*" = { path = "/path/to/wasm.wasm" }. Such a syntax seems more extensible and less likely to paint us into design corners in the future.

crates/compose/src/lib.rs Outdated Show resolved Hide resolved
crates/compose/src/lib.rs Show resolved Hide resolved
crates/compose/src/lib.rs Show resolved Hide resolved
crates/trigger/src/loader.rs Outdated Show resolved Hide resolved
@fibonacci1729
Copy link
Contributor Author

I've been thinking about this over the last week and i'm not really sold that .../* is really all that more intuitive over what we have. Personally having multiple syntaxes to imply variabilitiy (e.g. .../* and no version to imply any-version) is a bit confusing. As a dev, if i saw foo:bar/* to mean "any interface" i would intuitively think that i would need to say foo:bar/*@* to imply any interface and version. @rylev i know this isn't a super strong argument against but i think i'd like to land this with the syntax as-is unless you really feel strongly.

crates/compose/deny-all/src/fermyon.rs Show resolved Hide resolved
crates/compose/deny-all/src/wasi.rs Show resolved Hide resolved
crates/compose/src/lib.rs Outdated Show resolved Hide resolved
crates/compose/src/lib.rs Outdated Show resolved Hide resolved
crates/compose/src/lib.rs Show resolved Hide resolved
crates/manifest/src/schema/v2.rs Outdated Show resolved Hide resolved
crates/manifest/src/schema/v2.rs Outdated Show resolved Hide resolved
crates/manifest/src/schema/v2.rs Outdated Show resolved Hide resolved
crates/serde/src/dependencies.rs Show resolved Hide resolved
crates/serde/src/dependencies.rs Show resolved Hide resolved
@lann lann mentioned this pull request Aug 20, 2024
48 tasks
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

Thanks @fibonacci1729 for making this happen!

@itowlson
Copy link
Contributor

I did have one further (extremely non-blocking) question on the deny-all adapter. The adapter Wasm file is checked in. But I am nut sure how it gets built. Is this a "build locally and check in the binary" thing? If so it would be good to investigate (as a follow-up) how to move it to a controlled, trusted build environment, but I am not sure how that would play nicely with needing to have it checked in and being included as a blob.

@fibonacci1729 fibonacci1729 force-pushed the component-dependencies branch from 7d8ae2a to 07aa519 Compare August 22, 2024 18:32
@rylev
Copy link
Collaborator

rylev commented Aug 23, 2024

I've been thinking about this over the last week and i'm not really sold that .../* is really all that more intuitive over what we have.

@fibonacci1729 I'm not tied to my proposed syntax, but I do maintain that the current syntax is not ideal. The fact that "org:package/interface" means a specific interface while"org:package"` means any interface seems inconsistent, and if you had asked me to guess what the syntax would be, I would have guessed that. What's more, the current syntax doesn't seem to be very extensible. For example, how would we imagine a syntax for specifying a list of interfaces?

I'm not tied to any particular solution, but I feel somewhat strongly that the existing syntax is the wrong way go.

In the interest of making some sort of alternative for discussion, how about:

# A specific interface
"org:package" = { interface = "my-interface", path = "path/to/component.wasm" }
# All interfaces is the default
"org:package" = {  path = "path/to/component.wasm" }

In the above syntax, the key is always the package.

@lann
Copy link
Collaborator

lann commented Aug 23, 2024

how would we imagine a syntax for specifying a list of interfaces?

# equivalent to "org:package/my-interface"
"org:package" = { exports = ["my-interface"], ... }

@fibonacci1729
Copy link
Contributor Author

fibonacci1729 commented Aug 23, 2024

@rylev could you say more about why org:package/interface meaning a specific interface while org:package meaning a set of interfaces seems inconsistent? If you're concerned about enabling a more granular control over which interfaces can be matched, what @lann suggested is what we've been planning for in a follow-up iteration. Currently the code enforces a constraint to prevent this where it checks that if a dependency name is in the package pattern form the dependency object (right-hand-side) cannot specify an export. In the future it would seem to make sense to allow saying "org:package" = { ..., exports = ["a", "b"] } to mean that for all compatible imports part of the org:package package use a and b to satisfy them.

@fibonacci1729 fibonacci1729 force-pushed the component-dependencies branch 2 times, most recently from 0a344e3 to 19b2e6f Compare August 23, 2024 20:31
@itowlson
Copy link
Contributor

Regarding the UI, my mental model was that the left hand side was "the thing that is being satisfied" and the right hand side was "the thing that satisfies it". I appreciate that Ryan's "org:package" = { interface = "..." } idea was intended to spark new thinking, but for me the specific detail of putting "the interface that is being satisfied" on the right hand side feels a bit awkward. But maybe my mental model is out of sync with the realities of components? It wouldn't be the first time!

(And if Lann's comment is how we plan to do multiple interfaces then Ryan's idea seems consistent with it, although perhaps I am misunderstanding.)

@fibonacci1729 fibonacci1729 force-pushed the component-dependencies branch 6 times, most recently from e09cdd5 to 9df6994 Compare August 27, 2024 21:50
Signed-off-by: Brian H <brian.hardock@fermyon.com>
@fibonacci1729 fibonacci1729 force-pushed the component-dependencies branch from 9df6994 to b121199 Compare August 27, 2024 21:55
@fibonacci1729 fibonacci1729 merged commit 73044d8 into fermyon:main Aug 27, 2024
17 checks passed
@fibonacci1729 fibonacci1729 deleted the component-dependencies branch August 27, 2024 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants