Skip to content
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] Use crosslink tidylist in make gotidy #37142

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jade-guiton-dd
Copy link
Contributor

@jade-guiton-dd jade-guiton-dd commented Jan 10, 2025

Context

The make gotidy command currently runs go mod tidy on every module in the repository once, in alphabetical order. Under some circumstances, this turns out to not converge in one run, leaving modules in a "updates to go.mod needed" state, which has caused issues in CI such as core#11795.

To solve this problem, I recently added a tidylist subcommand to crosslink, which analyzes the dependency graph of the repo and generates a list of modules in topological order, which should eliminate most (if not all) of these non-convergence cases.

Description

This PR adds a make tidylist and a corresponding CI check to maintain an internal/tidylist/tidylist.txt file, which is then used in the modified make gotidy command to tidy all modules in the repo in order.

Since the otelcontribcol and oteltestbedcol are not gitted and may or may not exist in a local copy of the repo, in make tidylist I manually remove them from the list generated by crosslink to avoid a discrepancy that might fail the CI check.

For dependency graphs that contain cycles, such as the modules in Contrib, the tool uses an extension of topological sorting. However, because circular dependencies are hard to work with for multiple reasons, the tool will check the list of modules that have them against an allowlist (internal/tidylist/allow-circular.txt), which will have to be updated manually if dependency cycles are added or removed.

Link to tracking issue

Updates core#11795 (the next step will be to unrevert core#11670)

Testing

Testing this is a bit difficult as the errors this solves are quite rare. But the tool implements the same algorithm as the Python script I tried in #36723, which I successfully tested against the release process that failed in core#11795, so I have reason to believe the algorithm is correct. I expect any potential issues arising from this PR to be related to the integration of the tool rather than the logic in itself.

@jade-guiton-dd jade-guiton-dd marked this pull request as ready for review January 10, 2025 14:18
@jade-guiton-dd jade-guiton-dd requested a review from a team as a code owner January 10, 2025 14:18
@jade-guiton-dd jade-guiton-dd requested a review from mwear January 10, 2025 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants