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

Support custom cell executions #613

Merged
merged 10 commits into from
Jul 20, 2023
Merged

Support custom cell executions #613

merged 10 commits into from
Jul 20, 2023

Conversation

mxsdev
Copy link
Contributor

@mxsdev mxsdev commented Jun 14, 2023

Allows running cells with custom languages. Provides new program cell annotation, with which the cell will run.

Requires runme with stateful/runme#302

Depends on #590

2023-06-13_16-53-02

Copy link
Contributor

@duvanmonsa duvanmonsa left a comment

Choose a reason for hiding this comment

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

LGTM

tests/extension/messages/cellOutput.test.ts Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
Copy link
Member

@sourishkrout sourishkrout left a comment

Choose a reason for hiding this comment

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

Let's talk this thru today. I'd like us to provide more clarity that this, in fact, works like a #! shebang. We can decide to do that in a follow-up PR (likely more UX than logic).

Outside of that something like /usr/bin/env python3 does not work and errors will be "swallowed". Can we improve that please. Using env as a way to allow multi argument shebangs is commonplace.

@mxsdev
Copy link
Contributor Author

mxsdev commented Jun 15, 2023

Let's talk this thru today. I'd like us to provide more clarity that this, in fact, works like a #! shebang. We can decide to do that in a follow-up PR (likely more UX than logic).

Agreed. I used "program" as a placeholder (it fits with the internal naming structure), but I presumed we would be working it out more.

Outside of that something like /usr/bin/env python3 does not work and errors will be "swallowed". Can we improve that please. Using env as a way to allow multi argument shebangs is commonplace.

This is a bit complicated since we need to distinguish the program path from the arguments (this is probably a limitation of the syscall responsible for spawning a new process). Some paths can contain spaces, like "C:\Program Files...". For now I can use a simple regexp on whitespace.

But eventually we would need to (in descending order of difficulty):

  1. Do some basic shell parsing (maybe a use case for shlex)
  2. Wrap program execution in a bash shell (makes determining if the program exists [to create error window indicating unsupported language] difficult)
  3. Regexp/split on whitespace (breaks filepaths with spaces)
  4. Provide a separate input field for arguments (if i recall correctly, arguments are parsed by the system/shell program, so i don't think we need to worry here)

I think I can get this working with (1), maybe in a follow-up PR.

EDIT: looks like shebang line doesn't support spaces in path, so this would be extra functionality. Definitely interested in your thoughts @sourishkrout

@mxsdev
Copy link
Contributor Author

mxsdev commented Jun 15, 2023

@sourishkrout Just pushed changes that supports arguments in the program. One pretty massive issue, though: annotations don't allow spaces :/ not sure what to do about that.

I also fixed error handling. You should now get the following error popup if you don't provide a program, and the language is not supported:

Unable to automatically execute cell. To execute this cell, set the "program" field in the configuration foldout!

And the following error popup if the user-provided program does not exist:

Unable to locate program "<program>"

Happy to work out UX here 👍

@mxsdev mxsdev self-assigned this Jun 16, 2023
@mxsdev mxsdev force-pushed the mxsdev/shebang branch 2 times, most recently from f934ede to d23c9d5 Compare June 16, 2023 18:42
fix tests

serializer tests

support languageid on grpc runner

support runmeTask

configurable program

catch invalid program error

fix tests

test `getCellProgram`

basic e2e test

typo

Co-authored-by: Sebastian Tiedtke <sebastiantiedtke@gmail.com>

fix error messages

format
Copy link
Member

@sourishkrout sourishkrout left a comment

Choose a reason for hiding this comment

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

👍 LGTM, just some minor things. Feel free to change & merge.

@@ -145,6 +145,11 @@ export class Annotations extends LitElement {
description: "Cell's canonical name for easy referencing in the CLI.",
docs: 'https://docs.runme.dev/configuration#cell-options',
},
interpreter: {
description: 'Script shebang line',
// FIXME: update docs link
Copy link
Member

Choose a reason for hiding this comment

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

I'll do that before releasing 👍

src/extension/runner.ts Outdated Show resolved Hide resolved
@mxsdev mxsdev merged commit 93cb16a into main Jul 20, 2023
1 of 3 checks passed
@joseeantonior joseeantonior deleted the mxsdev/shebang branch November 20, 2023 19:13
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.

3 participants