[chore] Fix make generate on Windows #43509
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pjanotti
commented
Oct 13, 2025
| .PHONY: for-all | ||
| for-all: | ||
| @set -e; for dir in $(ALL_MODS); do \ | ||
| @set -e; for dir in $${ALL_MODS}; do \ |
There was a problem hiding this comment.
This is needed given the limits for command-line length on Windows. This way ALL_MODS is passed as an environment variable instead of being expanded in place.
atoulme
approved these changes
Oct 14, 2025
|
It looks like this PR broke the I believe the problem is that |
songy23
pushed a commit
that referenced
this pull request
Oct 15, 2025
#### Description #43509 changed the `make for-all` Makefile target to access the `ALL_MODS` variable through environment variables instead of expanding it into the shell command, in order to bypass Windows' 8192 byte command line length limit. However, the environment variable was not defined, which led to `make for-all` becoming a no-op. This can be confirmed by running `make for-all CMD='echo test'`. (I discovered this issue because [this PR](open-telemetry/opentelemetry-collector#13996) in core repository, which should be failing `contrib-tests`, suddenly started passing them. This was because the job inserts `replace` statements in a copy of contrib using `for-all`; failing to do that caused the contrib tests to run against whatever version of core is imported here rather than against the PR's code.) This PR fixes that oversight, by adding the `export` keyword to the `ALL_MODS` variable.
|
ops, sorry about that @jade-guiton-dd and thanks for the fix. My validation of this was bad, kind of completed an "empty" generate 🤦🏼 The |
ChrsMark
pushed a commit
to ChrsMark/opentelemetry-collector-contrib
that referenced
this pull request
Oct 20, 2025
Fix the `generate` target so developers using Windows can do `make generate` for their components on Windows. Main things in this change: 1. Use of path that can be pre-pended to `PATH`, the one being used contains `:` and even escaping it is not enough to get it to correctly work when it is being included in `PATH` 2. Ensure that on Windows all tools used by `go generate` have the `exe` extension. This is needed because the `cmd` package used by `go generate` checks for the known executable extensions on Windows. Notice that `make` and Git Bash don't require that and same extension is not needed for the other tools. 3. The `generate` target now triggers building the tools it needs as various other targets so it can avoid the full `install-tools`. Future work: - `go.*` targets are really slow on Windows since they trigger the re-evaluation of all shell commands that are really expensive on Windows. Basically any recursive make call from `for-all` is currently too expensive on Windows. - Move the definition of components custom tools to the respective components, possibly using the new go.mod `tool` directive. For now I opted to keep the tool used by `githubreceiver` as it is.
ChrsMark
pushed a commit
to ChrsMark/opentelemetry-collector-contrib
that referenced
this pull request
Oct 20, 2025
#### Description open-telemetry#43509 changed the `make for-all` Makefile target to access the `ALL_MODS` variable through environment variables instead of expanding it into the shell command, in order to bypass Windows' 8192 byte command line length limit. However, the environment variable was not defined, which led to `make for-all` becoming a no-op. This can be confirmed by running `make for-all CMD='echo test'`. (I discovered this issue because [this PR](open-telemetry/opentelemetry-collector#13996) in core repository, which should be failing `contrib-tests`, suddenly started passing them. This was because the job inserts `replace` statements in a copy of contrib using `for-all`; failing to do that caused the contrib tests to run against whatever version of core is imported here rather than against the PR's code.) This PR fixes that oversight, by adding the `export` keyword to the `ALL_MODS` variable.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix the
generatetarget so developers using Windows can domake generatefor their components on Windows. Main things in this change:PATH, the one being used contains:and even escaping it is not enough to get it to correctly work when it is being included inPATHgo generatehave theexeextension. This is needed because thecmdpackage used bygo generatechecks for the known executable extensions on Windows. Notice thatmakeand Git Bash don't require that and same extension is not needed for the other tools.generatetarget now triggers building the tools it needs as various other targets so it can avoid the fullinstall-tools.Future work:
go.*targets are really slow on Windows since they trigger the re-evaluation of all shell commands that are really expensive on Windows. Basically any recursive make call fromfor-allis currently too expensive on Windows.tooldirective. For now I opted to keep the tool used bygithubreceiveras it is.