-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[chore] Restore Lint on Windows #34656
[chore] Restore Lint on Windows #34656
Conversation
yep, there are some things to be fixed before we can consider merging this one... |
@@ -54,7 +54,7 @@ $(TOOLS_BIN_DIR): | |||
mkdir -p $@ | |||
|
|||
$(TOOLS_BIN_NAMES): $(TOOLS_BIN_DIR) $(TOOLS_MOD_DIR)/go.mod | |||
cd $(TOOLS_MOD_DIR) && $(GOCMD) build -o $@ -trimpath $(filter %/$(notdir $@),$(TOOLS_PKG_NAMES)) | |||
cd $(TOOLS_MOD_DIR) && GOOS="" $(GOCMD) build -o $@ -trimpath $(filter %/$(notdir $@),$(TOOLS_PKG_NAMES)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems reasonable to enforce this here since these tools are be used locally - at first I can't see a typical case in which we want this to follow GOOS
. This is also showing that the step to check the cache, used to skip make install-tools
is also wrong.
Opened #34691 to track the |
Lint is not happening for `GOOS=windows` . This change restores lint for Windows on CI runs. We may have to fix a bunch of lint issues before being able to merge this one. Noticed while looking at the failures from open-telemetry#34639
Lint is not happening for
GOOS=windows
. This change restores lint for Windows on CI runs.We may have to fix a bunch of lint issues before being able to merge this one.
Noticed while looking at the failures from #34639