windows: reduce dependency of go-shell, make build succeed on windows#47
Conversation
|
Possible fallout, on circleci can be seen Parameter is coming from .promu.yml build.flags. To fix, need to split up flags on spaces outside quotes. Working on a update... |
Patch updated with a fix and a test. Ready for review! |
| func runCommand(name string, arg ...string) error { | ||
| if viper.GetBool("verbose") { | ||
| cmdText := name + " " + strings.Join(arg, " ") | ||
| fmt.Fprintln(os.Stderr, " > ", cmdText) |
There was a problem hiding this comment.
Why Stderr ? This is not a error message.
There was a problem hiding this comment.
Nevermind, this is how set -x is working. I would prefer a + prefix instead of >, like a regular command trace.
There was a problem hiding this comment.
Because the corresponding code in go-shell wrote to stderr, see https://github.com/progrium/go-shell/blob/master/shell.go#L142
There was a problem hiding this comment.
Changed from > to +, patch updated
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package cmd |
There was a problem hiding this comment.
Given that this is not a promu command, I think it would be better to move it in the util folder like the retry package.
There was a problem hiding this comment.
Re-using the shell name collides with already imported go-shell, so renamed it to "sh"
| "time" | ||
|
|
||
| shell "github.com/progrium/go-shell" | ||
| "github.com/prometheus/promu/util/sh" |
There was a problem hiding this comment.
nits: we generally like to split internal and external packages. Could you move the ulil/sh at the end of the import section with a empty line between external and internal packages.
There was a problem hiding this comment.
Sure, patch updated and squashed
|
|
||
| "github.com/mcuadros/go-version" | ||
| "github.com/progrium/go-shell" | ||
| "github.com/prometheus/promu/util/sh" |
There was a problem hiding this comment.
Same here, external/internal package separation.
| "path/filepath" | ||
|
|
||
| "github.com/progrium/go-shell" | ||
| "github.com/prometheus/promu/util/sh" |
There was a problem hiding this comment.
Same here, external/internal package separation.
sdurrheimer
left a comment
sdurrheimer
left a comment
There was a problem hiding this comment.
Ok, seems good to me.
I think there is more work to do for good Windows compatibility, but the most important, I guess, is the build command. This one work now with your changes, thanks.
We should also be able to get rid of the go-shell package. I will work on that.
@grobie your thoughts ?
| return cmd.Run() | ||
| } | ||
|
|
||
| // ShellQuote ... |
There was a problem hiding this comment.
Since we have sh package, we can rename this to Quote.
There was a problem hiding this comment.
done, patch updated
| ) | ||
|
|
||
| // RunCommand ... | ||
| func RunCommand(name string, arg ...string) error { |
There was a problem hiding this comment.
Possible to have real comment sentence here for exposed function ? Like RunCommand executes a shell command.
Same for ShellQuote and SplitParameters.
There was a problem hiding this comment.
done, patch updated
grobie
left a comment
grobie
left a comment
There was a problem hiding this comment.
Getting rid of go-shell sounds good to me, its error handling doesn't seem to be very explicit.
3 participants
This makes promu successfully compile https://github.com/martinlindhe/wmi_exporter on Windows. Tested on Win 7.
The issues were:
There is still some dependencies on go-shell, but they seem to be pretty trivial to replace with stdlib code.
Fixes #44
This was triggered by trying to provide build info to wmi_exporter (prometheus-community/windows_exporter#24)