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

Include cli implementations for adapter components #723

Merged
merged 2 commits into from
Nov 22, 2024

Conversation

jsturtevant
Copy link
Contributor

@jsturtevant jsturtevant commented Nov 8, 2024

Some languages (c# and go) always include a few imports like environment and exit. Others components that are composed with the p2 adapter will have all the imports. This makes it so those components can run by default.

dotnet/runtime#107405

todo:

  • add a test

Mossaka
Mossaka previously approved these changes Nov 8, 2024
@Mossaka Mossaka closed this Nov 8, 2024
@Mossaka Mossaka reopened this Nov 8, 2024
Copy link
Collaborator

@jprendes jprendes left a comment

Choose a reason for hiding this comment

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

Thanks @jsturtevant !

Makefile Outdated Show resolved Hide resolved
devigned
devigned approved these changes Nov 16, 2024
Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

I haven't dug into the broken test runs, but besides that, lgtm.

@Mossaka
Copy link
Member

Mossaka commented Nov 16, 2024

Idea: We should allow users to apply environment variables or containerd configs to mutate WASI host capabilities, similar to how the Wasmtime CLI allows flags like '-Shttp' and '-Scli'. We might say, for WASI-HTTP components, the default host APIs are WASI-HTTP, no CLI, but you can configure Wasmtime shim to add CLI host APIs.

Not a blocker to this PR though.

jprendes
jprendes previously approved these changes Nov 19, 2024
Copy link
Collaborator

@jprendes jprendes left a comment

Choose a reason for hiding this comment

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

Thanks @jsturtevant !
LGTM

@jsturtevant
Copy link
Contributor Author

I had to remove the changes I made to the makefile completely to pass CI. I still need to clean the dist/ folder locally or it won't update the binary after changes but due to the way CI is set up it needs the dist/ folder in place. It would be nice to have the tooling do the right thing locally and in CI but for now will remove those changes.

@jprendes
Copy link
Collaborator

I feel our makefile is overly complex.
I wonder what would be a good solution.
How do you feel about a rust program that drives the build / test / ci workflows?

@jprendes jprendes dismissed devigned’s stale review November 19, 2024 22:22

Doesn't actually request a change

@jprendes
Copy link
Collaborator

@jsturtevant , do you want to add tests on this PR, or on a different one?

Some languages(c# and go) always include a few imports like environment and exit.  Others components that are composed with the p2 adapter will have all the imports.  This makes it so those components can run by default

Signed-off-by: James Sturtevant <[email protected]>
Mossaka
Mossaka previously approved these changes Nov 21, 2024
Signed-off-by: James Sturtevant <[email protected]>
Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

lgtm!

@Mossaka Mossaka merged commit 251575a into containerd:main Nov 22, 2024
71 checks passed
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.

4 participants