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

Add crosslink tidylist command #642

Merged
merged 6 commits into from
Dec 19, 2024

Conversation

jade-guiton-dd
Copy link
Contributor

@jade-guiton-dd jade-guiton-dd commented Dec 13, 2024

Context

While looking into this issue, it turned out that the current make tidy target on the collector-contrib repo does not properly converge: sometimes updates in one module require updates in its dependent modules, so when running go mod tidy in alphabetical order, multiple runs may be necessary to fully propagate changes, potentially as many as the longest dependency chain in the repository.

To solve this issue, I decided to write a script that could run go mod tidy on all modules in a repo in an order that guarantees full propagation of all changes. I wrote an initial prototype in Python to check that the theory worked, but as Collector developers may not have Python installed or know Python well enough to maintain the script, it was suggested that I rewrite it in Go.

Description

This PR adds a new subcommand to crosslink: crosslink tidy.

(I decided to add my script as a subcommand because crosslink already had most of the code necessary to read the intra-repository module dependency graph and because the code may be useful in other multi-module Go repositories (eg. the core Collector repo). If you believe it does not belong in this tool, I can split the code into its own tool, or just add it in collector-contrib.)

Details

This tool handles circular dependencies, although the schedule it generates will grow quadratically with the size of the largest strongly-connected component in the dependency graph. For reference, in collector-contrib there are 2 SCCs with more than 1 module: one with 2, and one with 3, so I don't believe this would be a problem in practice. Nevertheless, to avoid the unexpected addition of circular dependencies, this tool checks modules that have them against a provided allowlist.

The tool also has a flag to run a naive validation step on the computed solution: it simply checks that all possible dependency chains are subsequences of the schedule, so changes in any upstream module should propagate to any other downstream module. This validation runs in half a second on my machine, but I disabled it by default as it could potentially be quite expensive.

Note that this tool makes the assumption that in the presence of a circular dependency of A on itself, A will not need to be tidied multiple times for the same change (ie. we need to guarantee propagation along paths A → B and A → B → C, but not A → B → C → B). Considering this worrying description of go mod tidy never converging, I cannot guarantee that this assumption is always true.

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 79.23497% with 38 lines in your changes missing coverage. Please review.

Project coverage is 48.86%. Comparing base (1bce46e) to head (1bef0a6).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crosslink/internal/tidylist.go 81.37% 18 Missing and 9 partials ⚠️
crosslink/internal/common.go 55.55% 6 Missing and 2 partials ⚠️
crosslink/cmd/root.go 83.33% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #642      +/-   ##
==========================================
+ Coverage   47.13%   48.86%   +1.72%     
==========================================
  Files          56       57       +1     
  Lines        3214     3381     +167     
==========================================
+ Hits         1515     1652     +137     
- Misses       1548     1568      +20     
- Partials      151      161      +10     

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

crosslink/internal/tidy.go Outdated Show resolved Hide resolved
crosslink/internal/tidy.go Outdated Show resolved Hide resolved
crosslink/internal/tidy.go Outdated Show resolved Hide resolved
crosslink/internal/tidy.go Outdated Show resolved Hide resolved
crosslink/internal/tidy.go Outdated Show resolved Hide resolved
crosslink/cmd/root.go Outdated Show resolved Hide resolved
@mx-psi mx-psi changed the title Add crosslink tidy command Add crosslink tidylist command Dec 17, 2024
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

@mx-psi mx-psi merged commit c70d42c into open-telemetry:main Dec 19, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants