SG start bazel enhancements - #59718
Conversation
| [ | ||
| { | ||
| "regex": "^Check that imports in Go sources match importpath attributes in deps.$", | ||
| "command": "./dev/bazel_configure_accept_changes.sh", | ||
| "args": [] | ||
| }, | ||
| { | ||
| "regex": "missing input file", | ||
| "command": "./dev/bazel_configure_accept_changes.sh", | ||
| "args": [] | ||
| }, | ||
| { | ||
| "regex": ": undefined:", | ||
| "command": "./dev/bazel_configure_accept_changes.sh", | ||
| "args": [] | ||
| } | ||
| ] |
There was a problem hiding this comment.
Contains patterns of bazel output and the command to run to try to auto fix it. iBazel will use this to to try to auto-resolve build problems.
| #! /bin/bash | ||
|
|
||
| # Run bazel configure and if the error code is 110, exit with error code 0 | ||
| # This is because 110 means that configuration files were successfully |
There was a problem hiding this comment.
looks like the sentence is cut over here :D
| } | ||
|
|
||
| secretsEnv, err := getSecrets(ctx, bc.Name, bc.ExternalSecrets) | ||
| func (bc BazelCommand) watchPaths() ([]string, error) { |
There was a problem hiding this comment.
nit: shouldn't this be watchedPaths? I intuitively read watchPaths as "I will watch those paths" not as "get me the paths that I should watch"
| if bc.IgnoreStderr { | ||
| std.Out.WriteLine(output.Styledf(output.StyleSuggestion, "Ignoring stderr of %s", bc.Name)) | ||
| stderrWriter = sc.stderrBuf | ||
| func (bc BazelCommand) StartWatch(ctx context.Context) (<-chan struct{}, error) { |
There was a problem hiding this comment.
nit: Watch(ctx context.Context) ... ?
There was a problem hiding this comment.
Would love to but but both Command and BazelCommand use Watch for a local field.
| } | ||
|
|
||
| func (cmd Command) watchPaths() ([]string, error) { | ||
| root, err := root.RepositoryRoot() |
There was a problem hiding this comment.
nit: we're doing this over and over for every command, it's most likely harmless to run it each time, but we could do this at construction time, when we're building commands.
| sc.cancel = cancel | ||
| ctx, cancel := context.WithCancel(ctx) | ||
| sc.cancel = func() { | ||
| sc.Cmd.Process.Signal(os.Interrupt) |
There was a problem hiding this comment.
As per the godoc in exec.Cmd
// If Cancel is non-nil, the command must have been created with
// CommandContext and Cancel will be called when the command's
// Context is done. By default, CommandContext sets Cancel to
// call the Kill method on the command's Process.
// If the child process has failed to exit — perhaps because it ignored or
// failed to receive a shutdown signal from a Cancel function, or because no
// Cancel function was set — then it will be terminated using os.Process.Kill.
Therefore, I think we don't need to handle this explicitly, did this result of observing a different behaviour when you tested the implementation?
There was a problem hiding this comment.
Yes I saw a ton of orphan child processes but there were lots of other bugs that have been resolved since then. Maybe this can be removed.
There was a problem hiding this comment.
Yeah I am definitely still seeing this problem. I think the issue is exactly that it's using SIGKILL which doesn't do any cleanup, which specifically orphans any child processes. So for example docsite and web both spin up servers in child processes and then you get EADDRINUSE all over the place.
Adding a proper interrupt fixed it for me.
| stderrWriter = io.MultiWriter(logger, sc.stderrBuf) | ||
| eg, err := process.PipeOutputUnbuffered(ctx, sc.Cmd, stdoutWriter, stderrWriter) | ||
| func profileEventsFilePath(eventsDir string) string { | ||
| return path.Join(eventsDir, "profile.json") |
There was a problem hiding this comment.
IIRC this can be overriden by a command line flag, shouldn't we perhaps always explicitly set this when calling iBazel so we're 100% sure it'll be there?
| "Might as well check Slack. %s is taking its time...", | ||
| "In German there's a saying: ein guter Käse braucht seine Zeit - a good cheese needs its time. Maybe %s is cheese?", | ||
| "If %ss turns out to be cheese I'm gonna lose it. Hey, hurry up, will ya", | ||
| "Still waiting for %s to finish installing...", |
There was a problem hiding this comment.
We should make a tradition for anyone editing this file to add a new one :D
…ph into jsm/sg-start-bazel
jhchabran
left a comment
jhchabran
left a comment
There was a problem hiding this comment.
@jamesmcnamara I don't want to block you further and as long as we don't replace the default target, this isn't going to break anything, so I'm inclined to LGTM :)
2 participants
Just getting this up in a draft status so it can get some eyes on it. There are a lot of changes and still some known bugs that I need to resolve but its close.
High Level Changes
SGConfigCommandrunAndWatchinto many different piecesInstallerinterface to capture events that can be installedstartedCmdby using acommandOptionsstructI'll call out more inline
Known Issues
Test plan
Manually tested locally