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

Output Runner and Multiple Runs #133

Closed
wants to merge 2 commits into from

Conversation

borkaehw
Copy link
Contributor

Output Runner

Issue

When we build a target with Bazel, if we got redundant or missing deps, there is a prompt like

You can use the following buildozer command:
buildozer 'add deps @deps_name' //target:target

or

You can use the following buildozer command:
buildozer 'remove deps @deps_name' //target:target

The build fails and hangs.

Fix

We monitor the Bazel logs for these kinds of information. We generate an interactive prompt

Do you want to execute this command?
buildozer 'add deps @deps_name' //target:target
[y/N]

Users can reply y to fix the deps problem and rebuild.

The command matching is defined by regex and it is configurable by JSON. Users are able to decide what to monitor.

To enable output runner, use flag -run_output=config_file_name.json. The config file format is provided in ibazel/output_runner/output_runner_test.json.

Multiple Runs

Issue

iBazel currently only supports run for single target. Running multiple targets requires separate Bazel command executions and busy waiting for build processes.

Fix

Implement a mrun command to run multiple targets with iBazel

ibazel mrun target1 --arg=t1_arg1 --arg=t1_arg2 target2 --arg=t2_arg1 --arg=t2_arg2

Append arbitrary number of targets after mrun with the corresponding arguments, prefixed with --arg=.

@borkaehw
Copy link
Contributor Author

Hey @achew22, sorry for the mess yesterday. I created another PR, hopefully this is clearer. I am happy to take any suggestion for improvement. Thanks in advance.

iBazel now watches the log warning from Bazel outputs. Users can watch for
specific commands and apply them automatically. The command matching is
implemented by regex and it is configurable.

Fixes: bazelbuild#18
@alexeagle
Copy link
Collaborator

What's the tradeoff of just changing the behavior of run command? Is it that we can't distinguish a second target to run from an argument to the first program? I'm not sure that your scheme for disambiguating args from additional targets-to-run is sufficient either.
I don't have a better proposal after thinking for a couple of minutes - you could read multiple lines from stdin but I think there may be other reasons to pass things to stdin of ibazel? and might be too subtle from the command line, e.g. echo -n "//path/to:target1 --arg1 --arg2\n//path/to:target2 --arg3 --arg4" | ibazel run

Could you clarify who is expected to use -run_output=config_file_name.json - is that any time a user wants to run ibazel with the fixer enabled?

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Wow, thanks for all this work!
We will want to use this feature for ts_auto_deps too (Gazelle for TypeScript/Angular)
FYI @mprobst

b.ctx, b.cancel = context.WithCancel(context.Background())

args = append([]string{command}, args...)

if b.writeToStderr || b.writeToStdout {
containsColor := false
Copy link
Collaborator

Choose a reason for hiding this comment

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

how is this color change related to the rest of the PR? could it be separated out?

Copy link

Choose a reason for hiding this comment

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

It isn't. I'm pretty sure this is a leftover remnant from an internal modification to make sure ibazel always used color when running bazel commands. It can be pulled out.

@@ -37,9 +42,11 @@ var osExit = os.Exit
var bazelNew = bazel.New
var commandDefaultCommand = command.DefaultCommand
var commandNotifyCommand = command.NotifyCommand
var mrunToFiles = flag.Bool("mrunToFiles", false, "Log mrun to file for simpler log reading")
Copy link
Collaborator

Choose a reason for hiding this comment

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

the need for this feature makes me wonder, if you're going to open two or more log files to see the output, why not just run different ibazel commands in one or more terminals?

I think the answer is that it's hard to script around "start these three programs in three terminals" in any portable way...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we are providing this option for those who want to see the outputs, while others who don't need the outputs can use just one terminal for ibazel. The other reason is that if you're running ten targets but only need two log files, you can just have two terminals instead of ten.

ibazel/ibazel.go Outdated
@@ -229,6 +266,13 @@ func (i *IBazel) Run(target string, args []string) error {
return i.loop("run", i.run, []string{target})
}

// Run the specified target (singular) in the IBazel loop.
func (i *IBazel) RunMulitple(args, target []string, debugArgs [][]string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

spelling: Multiple

}
}

func (i *IBazel) iterationMultiple(command string, commandToRun runnableCommands, targets []string, debugArgs [][]string, argsLength int) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

wow a ton of code is added to this file - is it possible to simplify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible. Since test, build, and run share a few functions, mrun has its own functions with different signatures but do similar things, we can try to merge those functions and make mrun a special case. It requires a lot of works and involves changing code for test and build. Should we do it?

@jjudd
Copy link

jjudd commented Aug 1, 2018

Pulling in @BryantN for the mrun vs run change. We were split when we made the change and he was the main proponent for mrun.

iBazel currently only supports "run" for single target. Running multiple
targets requires separate Bazel command executions and busy waiting for
build processes.

Implement "mrun" command which takes in arbitrary number of targets, and
run all of them at once. We can pass in corresponding arguments with
prefix "--arg=".
@borkaehw
Copy link
Contributor Author

borkaehw commented Aug 1, 2018

@alexeagle Yes. -run_output=config_file_name.json is a file that users can define what bazel outputs they are watching for. The only use case for us so far is to watch for buildozer command and fix the deps problem, we might encounter other use case.

@borkaehw borkaehw closed this Aug 3, 2018
@borkaehw borkaehw deleted the merge2upstream branch August 17, 2018 19:57
@alexeagle
Copy link
Collaborator

Hey @borkaehw - what happened with the multi_binary proposal to capture the mrun use case? Do you have any write-up, or should I start one?

Andrew gathered some notes

[multi_binary(
	name = “all_my_binaries”,
	binaries = {
		“//path/to/binary1”: {
			“args”: mode + [“a”, “b”, “c”],
		},
		“//path/to/binary2”, {
			“args”: mode + [“1”, “2”, “3”],
		},
	},
) for mode in [[], [“--debug”]]]

multi_test(
	name = “tests”,
	binaries = “:all_my_binaries”
	test = “:test”,	
)

@borkaehw
Copy link
Contributor Author

borkaehw commented Oct 2, 2018

Hey @alexeagle, thanks for asking. We are currently tackling this Bazel issue
bazelbuild/bazel#2832
Once we have the multiplex worker ready, multiple run seems to be a great project.

Feel free to start one though.

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