fix: Remove an unnecessary dependency to sh #145
Conversation
| @@ -84,7 +84,7 @@ func doLint( | |||
|
|
|||
| flags, err := lint.NewFlags(args) | |||
| if err != nil { | |||
| _, _ = fmt.Fprint(stderr, err) | |||
| _, _ = fmt.Fprintln(stderr, err) | |||
There was a problem hiding this comment.
[memo] It enables it to properly show an error message when err ends up without a new line.
❯❯❯ ./protolint -plugin ./not_existence _example/proto/
failed client.Client(), err=fork/exec ./plugin/greete: no such file or directory
| @@ -40,7 +40,7 @@ func (f *PluginFlag) BuildPlugins(verbose bool) ([]shared.RuleSet, error) { | |||
| client := plugin.NewClient(&plugin.ClientConfig{ | |||
| HandshakeConfig: shared.Handshake, | |||
| Plugins: shared.PluginMap, | |||
| Cmd: exec.Command("sh", "-c", value), | |||
| Cmd: exec.Command(value), | |||
There was a problem hiding this comment.
[memo] It works by directly passing an executable.
There was a problem hiding this comment.
I remember now that sh -c is necessary when giving flags to a plugin like below:
# it works now but doesn't work after this fix.
$ ./protolint -plugin "./plugin_example -go_style=false" _example/proto/
There was a problem hiding this comment.
Could you expand the flags such that they are passed as arguments to the command, instead of simply passing value?
Naive approach would be to split on spaces, but that would break in the case of the flags containing quoted arguments containing spaces. Writing a quick parser to support this shouldn't be too difficult.
eg: Currently, value (from your example) is ./plugin_example -go_style=false. The arguments need to be split up for this to work as expected.
My suggestion would be to use exec.Command(value, args...) instead, where value="./plugin_example" and args=[]string{"-go_style=false"}
Of course, this is very naive, and wouldnt support any fancy expansions that sh supports. Is there a go lib to emulate that?
There was a problem hiding this comment.
Thank you for your nice suggestion.
I'm interested in that go lib.
As I'm not a Windows user, I've been waiting for others' feedbacks, like #144 (comment). These would be used for considering this fix's priority because I have no enough time.
ref. Protolint doesn't work with -plugin in windows · Issue #144 · yoheimuta/protolint