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

Spin Factors #2519

Closed
wants to merge 22 commits into from
Closed

Spin Factors #2519

wants to merge 22 commits into from

Conversation

lann
Copy link
Collaborator

@lann lann commented May 20, 2024

ref #2518

This has been turned into the factors branch

crates/core/src/lib.rs Outdated Show resolved Hide resolved
crates/factor-outbound-networking/src/lib.rs Outdated Show resolved Hide resolved
crates/factor-outbound-networking/src/lib.rs Outdated Show resolved Hide resolved
crates/factor-outbound-networking/src/lib.rs Outdated Show resolved Hide resolved
.component_allowed_hosts
.get(ctx.app_component().id())
.cloned()
.context("missing component allowed hosts")?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This error message is a bit weak. Perhaps:

Suggested change
.context("missing component allowed hosts")?;
.with_context(|| format!("missing allowed outbound hosts configuration for component {}", ctx.app_component().id()))?;

crates/factor-outbound-networking/src/lib.rs Outdated Show resolved Hide resolved
crates/factor-outbound-networking/src/lib.rs Outdated Show resolved Hide resolved
crates/factor-wasi/src/lib.rs Show resolved Hide resolved
crates/factors/src/lib.rs Outdated Show resolved Hide resolved
crates/factors/src/factor.rs Outdated Show resolved Hide resolved
crates/factor-wasi/src/lib.rs Outdated Show resolved Hide resolved
crates/factors/src/spin_factors.rs Outdated Show resolved Hide resolved
crates/factors/src/runtime_config.rs Outdated Show resolved Hide resolved
crates/factors/src/runtime_config.rs Outdated Show resolved Hide resolved
crates/factors/src/factor.rs Outdated Show resolved Hide resolved
crates/factor-outbound-networking/src/lib.rs Outdated Show resolved Hide resolved
crates/factors/src/factor.rs Outdated Show resolved Hide resolved
crates/factors/src/factor.rs Outdated Show resolved Hide resolved
crates/factors/src/factor.rs Outdated Show resolved Hide resolved
crates/factors/src/factor.rs Outdated Show resolved Hide resolved
crates/factor-variables/src/provider_type.rs Outdated Show resolved Hide resolved
crates/factor-variables/src/provider_type.rs Outdated Show resolved Hide resolved
crates/factor-variables/src/provider_type.rs Outdated Show resolved Hide resolved
crates/factor-variables/src/provider_type.rs Outdated Show resolved Hide resolved
}
}

pub trait SelfInstanceBuilder: 'static {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This and DefaultInstanceBuilder are nice, but I'm not convinced they're worth it. It seems like they might make things harder to understand in order to save a few lines of boilerplate...

crates/factors/src/runtime_factors.rs Outdated Show resolved Hide resolved
/// The default implementation returns an empty schema, which accepts any
/// configuration.
fn runtime_config_json_schema(&self) -> impl serde::Serialize {
HashMap::<(), ()>::new()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is somewhat of a placeholder for now, thinking about how to support #2529 (comment)

Comment on lines +99 to +101
// TODO: This seems fine, but then again I don't understand why `FieldOffset`'s
// own `Sync`ness depends on `U`...
unsafe impl<T: RuntimeFactors, F1: Factor, F2: Factor> Sync for StateGetter2<T, F1, F2> {}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A little spooky, but less spooky than what I had before...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to stay here? If so, can you try rewriting the comment as to why this is actually safe to do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have a fork of field-offset that fixes it, but I'm not sure that anyone is actually maintaining upstream 😬.

If that PR doesn't get merged I'm not sure I even want to keep the dependency...

@lann lann force-pushed the factors branch 2 times, most recently from eeeb140 to 6fbd5b7 Compare June 20, 2024 21:03
lann added 13 commits June 25, 2024 14:25
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
And shorten some generic params.

Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Also address a bunch of feedback.

Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
lann added 9 commits June 25, 2024 14:25
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Also sneak in Factor::runtime_config_json_schema

Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
@lann
Copy link
Collaborator Author

lann commented Jun 25, 2024

Closing this in favor of working on the factors branch.

@lann lann closed this Jun 25, 2024
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.

2 participants