run-make-support: use a `CommandWrapper` trait instead of `impl_helper_methods` macro about rust HOT 5 CLOSED

FWIW, I tried to remove the macro by implementing Deref/DerefMut<Target=Command> for the wrappers instead. It would have worked.. were it not for the builder pattern :) Because if we implement the helper functions (like env) on Command directly, then they have to return &mut Self, which means that you would have to use all of the top-level command wrapper methods before using any of the Command methods, which is annoying. For example, rustc().crate_type().env() would work, but rustc().env().crate_type() wouldn't work, because env() would return Command, and not Rustc.

I'm not aware of any way of resolving this without using a macro. That being said, I think that the current macro is fine, it just adds a bunch of methods, and does not have any specific requirements on the wrapper apart from having a cmd field. The documentation needs to be updated though (#126188).

from rust.

I'm going to close this issue, because I think using a trait here unfortunately just makes the API worse.

from rust.

Drawback: test writer would need to bring the trait into scope to use the helper methods...

from rust.

There could be inherent methods that simply forward to the trait methods. That's more boilerplate though.

from rust.

We don't need command_output on each wrapper struct anymore as of #126121. I don't think that a trait pulls its weight here, due the tests having to import it (it's a bit annoying since run-make tests are typically not very friendly with IDEs). It would be nice to remove the macro, but we can do that also with other approaches than a trait (e.g. Deref).

from rust.