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

Unlock shebang++ #560

Merged
merged 8 commits into from
Apr 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions internal/api/runme/runner/v1/runner.proto
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,18 @@ message ExecuteRequest {

// file extension associated with script
string file_extension = 26;

// optional well known id for cell/block. "know" meaning that
// CLI/notebook UX have id/name for cells/blocks that contain commands.
// While the runner doesn't require the name to work, it is useful for
// auxiliary concerns (e.g. tracing, logging, etc).
string known_id = 27;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that it is not used. We can keep it for the completeness reason.

The most important for me is to provide more detailed description. What does "known" mean? What guarantees does it give? Maybe given an example how client would use this.

Finally, if you take a look at config/v1alpha1/config.proto, we have a way to validate field values in the proto definition. This is a bit tricky and not used anywhere else, so it might be a better idea for a separate PR.

Copy link
Member Author

@sourishkrout sourishkrout Apr 22, 2024

Choose a reason for hiding this comment

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

Will add an example. The reason why I added id (unused) was because I have near-term projects in mind where I need it. I'll be sure to make the description better. Going to skip validation in this PR for now.


// optional well known name for cell/block. "know" meaning that
// CLI/notebook UX have id/name for cells/blocks that contain commands.
// While the runner doesn't require the name to work, it is useful for
// auxiliary concerns (e.g. tracing, logging, etc).
string known_name = 28;
}

message ProcessPID {
Expand Down
6 changes: 6 additions & 0 deletions internal/api/runme/runner/v2alpha1/config.proto
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ message ProgramConfig {
// should tell whether to execute it inline or as a file.
CommandMode mode = 8;

// optional well known id for cell/block
string known_id = 9;

// optional well known name for cell/block
string known_name = 10;

message CommandList {
// commands are commands to be executed by the program.
// The commands are joined and executed as a script.
Expand Down
6 changes: 3 additions & 3 deletions internal/api/runme/runner/v2alpha1/runner.proto
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,9 @@ message ExecuteRequest {
// project used to load environment variables from .env files.
optional Project project = 22;

// store_last_stdout, if true, will store the stdout of
// the last ran block in the environment variable `__`.
bool store_last_stdout = 23;
// store_stdout_in_env, if true, will store the stdout under well known name
// and the last ran block in the environment variable `__`.
bool store_stdout_in_env = 23;
}

message ExecuteResponse {
Expand Down
7 changes: 7 additions & 0 deletions internal/command/config_path_normalizer.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package command

import (
"os/exec"
"strings"

"github.com/pkg/errors"
Expand Down Expand Up @@ -53,6 +54,12 @@ func (n *pathNormalizer) findProgramInInterpreters(programName string) (programP
}
}

// Default to "cat"
cat, err := exec.LookPath("cat")
Copy link
Member Author

Choose a reason for hiding this comment

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

@adambabik should this be using the new sys abstraction?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can leave it as is. I need to port the system pkg usage to beta commands and runnerv2.

if err == nil {
return cat, nil, nil
}

return "", nil, errors.Errorf("unable to look up any of interpreters %s", interpreters)
}

Expand Down
2 changes: 1 addition & 1 deletion internal/command/env_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func (s *envStore) Set(k, v string) (*envStore, error) {
// environSize += envPairSize(key, value)
// }

// if environSize > MaxEnvironSizInBytes {
// if environSize > MaxEnvironSizeInBytes {
// return s, errors.New("could not set environment variable, environment size limit exceeded")
// }

Expand Down
4 changes: 2 additions & 2 deletions internal/command/env_store_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

package command

// MaxEnvironSizInBytes is the maximum size of an environment variable
// MaxEnvironSizeInBytes is the maximum size of an environment variable
// including equal sign and NUL separators.
//
// This size is an artificial limit as Linux and macOS do not have a real limit.
const MaxEnvironSizInBytes = 128 * 1000 * 1000 // 128 MB
const MaxEnvironSizeInBytes = 128 * 1000 * 1000 // 128 MB
4 changes: 2 additions & 2 deletions internal/command/env_store_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@

package command

// MaxEnvironSizInBytes is the maximum size of an environment variable
// MaxEnvironSizeInBytes is the maximum size of an environment variable
// including equal sign and NUL separators.
const MaxEnvironSizInBytes = 32767
const MaxEnvironSizeInBytes = 32767
448 changes: 235 additions & 213 deletions internal/gen/proto/go/runme/runner/v1/runner.pb.go

Large diffs are not rendered by default.

55 changes: 38 additions & 17 deletions internal/gen/proto/go/runme/runner/v2alpha1/config.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading