Skip to content

Conversation

dnaeon
Copy link
Contributor

@dnaeon dnaeon commented Sep 16, 2025

Description

This PR migrates the existing internal/tools to the new recommended approach for managing tool dependencies.

The makefile targets have been updated, including test/build scripts as well.

All tools have been migrated to the to the new go tool approach, with one exception - modernize tool.

The reason why modernize has been left out is because of this issue.

The issue here is that the modernize command, currently (but not fundamentally) part of the gopls module, is governed by the gopls release process, and you can't just upgrade the x/tools package to a later one, as happens when installing stringer, because the gopls release branch needs a particular version and x/tools does not present a stable internal API to gopls. In short, no-one should ever add gopls as a module dependency, and that means you cannot use go get -tool with gopls. I will update the documentation to make this clear.

Please check this issue for full details:

Based on the comment above it would appear that even in the current internal/tools/go.mod we have a dependency on gopls, which we should not.

cc: @mx-psi

Link to tracking issue

Fixes #13721

Testing

Manual testing and running the various Makefile targets, including all target.

Documentation

N/A

@dmathieu dmathieu added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Sep 16, 2025
@dnaeon dnaeon force-pushed the feats/tools-directive branch from b0c521b to b92dcc5 Compare September 16, 2025 11:35
@dnaeon
Copy link
Contributor Author

dnaeon commented Sep 16, 2025

I've set the min Go version to 1.24 for the tools, which should fix the majority of the pipeline failures, since they are running on the old stable version.

Copy link

codecov bot commented Sep 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.67%. Comparing base (cd46300) to head (b471232).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13844      +/-   ##
==========================================
- Coverage   91.69%   91.67%   -0.02%     
==========================================
  Files         652      652              
  Lines       42506    42506              
==========================================
- Hits        38976    38968       -8     
- Misses       2725     2731       +6     
- Partials      805      807       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dnaeon
Copy link
Contributor Author

dnaeon commented Sep 17, 2025

Please hold this one for now, as it seems there is an issue related to how go tool ... interprets relative replace statements in go.mod when using the -modfile flag, which is the reason for this pipeline failure.

go tool -modfile /home/runner/work/opentelemetry-collector/opentelemetry-collector/internal/tools/go.mod pdatagen
go.opentelemetry.io/collector/internal/cmd/pdatagen: go.opentelemetry.io/collector/internal/cmd/[email protected]: replacement directory ../cmd/pdatagen does not exist
make: *** [Makefile:256: genpdata] Error 1
Error: Process completed with exit code 2.

It appears that the relative replace statement is relative to the current directory of the calling process (in this case the repo root), instead of location of the go.mod file itself.

I will follow up with additional details, as soon as I have them, but for now please hold this PR. Thanks!

@dnaeon
Copy link
Contributor Author

dnaeon commented Sep 19, 2025

Please hold this PR for now, as there is a known limitation with how go tool -modfile <tool-name> works when being called outside of the tools module.

See this one for more details.

@dmathieu , in order to workaround this limitation we may need to remove the replace directive in the internal/tools module.

Are you okay with such changes? Also, is there a specific reason why pdatagen is not exported/published elsewhere, and requires a replace directive?

Thanks!

@dmathieu
Copy link
Member

Removing the replace directive would be fine by me, if the behavior remains the same.
pdatagen should stay in this repository however. It's a tool that generates code used only by this repository, and has no reason to be used elsewhere. So extracting it would just make things harder to maintain.

@dnaeon dnaeon force-pushed the feats/tools-directive branch from 73c52a5 to 9f0ca9b Compare September 19, 2025 13:47
@dnaeon
Copy link
Contributor Author

dnaeon commented Sep 19, 2025

Removing the replace directive would be fine by me, if the behavior remains the same. pdatagen should stay in this repository however. It's a tool that generates code used only by this repository, and has no reason to be used elsewhere. So extracting it would just make things harder to maintain.

Makes sense, I've just updated and rebased the PR.

@dnaeon dnaeon force-pushed the feats/tools-directive branch from 8dbc713 to ac3cc9e Compare September 22, 2025 17:14
@dnaeon
Copy link
Contributor Author

dnaeon commented Sep 23, 2025

@dmathieu , @mx-psi , as far as I can tell I can safely ignore this pipeline failure?

@dnaeon dnaeon force-pushed the feats/tools-directive branch from ac3cc9e to b9e6b80 Compare September 23, 2025 11:27
@mx-psi
Copy link
Member

mx-psi commented Sep 23, 2025

@dmathieu , @mx-psi , as far as I can tell I can safely ignore this pipeline failure?

Yes, feel free to ignore that error, it is not related to this PR

github-merge-queue bot pushed a commit that referenced this pull request Sep 24, 2025
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

This PR adds a new flag (`-C path`) to the `pdatagen` utility, which
allows specifying the workdir for the tool.

Currently `pdatagen` expects to find the respective files in`pdata`
directory of the `$cwd`. When `pdatagen` is called outside of the
`$repo_root` the tool fails, because it cannot find `pdata` and panics.

This PR adds an optional `-C path` flag, which would allow `pdatagen` to
use an alternative workdir, and also be able to be called outside of the
`$repo_root` directory.

See #13844 for more details about this PR and the proposed changes. The
commit from this PR have been extracted from #13844 .

<!-- Issue number if applicable -->
#### Link to tracking issue

<!--Describe what testing was performed and which tests were added.-->
#### Testing

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->
@mx-psi
Copy link
Member

mx-psi commented Sep 24, 2025

Feel free to rebase this PR now!

See [1] for more details about the new recommended approach for managing tool
dependencies.

[1]: https://go.dev/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module
`modernize' should not be installed via `go get -tool', since gopls release
branch needs a specific version and x/tools does not present a stable internal
API to gopls.

See [1] for more details about this one.

[1]: golang/go#73279
@dnaeon dnaeon force-pushed the feats/tools-directive branch from b9e6b80 to b471232 Compare September 24, 2025 09:57
@dnaeon
Copy link
Contributor Author

dnaeon commented Sep 24, 2025

Feel free to rebase this PR now!

... and done :)

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!!

@mx-psi mx-psi added this pull request to the merge queue Sep 24, 2025
Merged via the queue into open-telemetry:main with commit 8658c56 Sep 24, 2025
60 of 61 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 24, 2025
Copy link
Contributor

otelbot bot commented Sep 24, 2025

Thank you for your contribution @dnaeon! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.

mx-psi added a commit that referenced this pull request Sep 24, 2025
github-merge-queue bot pushed a commit that referenced this pull request Sep 24, 2025
Reverts #13844 since it caused
#13897

cc @dnaeon (no worries, we can accept your PR again once we are sure it
won't cause further breakage)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate tool dependencies to tools directive
3 participants