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] [WIP] Add script that runs go mod tidy in topological order #36723

Closed
wants to merge 2 commits into from

Conversation

jade-guiton-dd
Copy link
Contributor

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

Context

In a repo with multiple Go modules, some of which depend on each other, and where internal dependencies are managed with replace statements, you end up with the situation where an update to the go.mod of one module can require running go mod tidy on modules that depend on it.

We have the make gotidy command available, which runs go mod tidy on every module in the repo. But this is done in alphabetical order, so that if alpha depends on bravo which depends on charlie, and charlie's go.mod is modified, bravo will get an update, but not alpha, as it gets tidied before the other two.

This leads to sometimes needing to run go mod tidy multiple times before things converge. This caused an issue in the latest release process in core, where the recently modified (link to relevant PR) check-contrib check failed because running make gotidy only once caused an error when running make generate (link to the relevant issue).

Description

This PR attempts to solve this issue by introducing a make topotidy command, which runs go mod tidy commands in topological order, ie. in an order such that dependencies always come before their dependents. This makes sure that changes to a go.mod are always fully propagated through the dependency graph.

Unfortunately, topological order doesn't quite cut it, as there are loops in the dependency graph (at the moment, between the two Datadog components, and between the three OTel-Arrow modules). It's not entirely clear if convergence is even possible in this context, but at the very least, we may need to run go mod tidy on a module twice to guarantee that all changes have had a chance to be propagated to every other module.

To solve this, I used Tarjan's algorithm, which sorts the graph topologically while isolating its strongly connected components (SCCs, parts where no topological order is possible), and applied a very naive algorithm to each SCC (which will hopefully remain few and small).

I implemented all this as a Python script in the internal/buildscripts directory. I believe this is the first Python script in this repo, so please reach out if there are concerns about this.

Link to tracking issue

Updates this issue on core. Unless we decide to replace make gotidy entirely with this script, we will need a PR on core to use make topotidy inside make check-contrib.

Testing

I locally replicated the latest release process, and checked that all "updates to go.mod needed" were eliminated when using make topotidy instead of make gotidy inside make check-contrib.

Documentation

I added a comment explaining the rationale and functioning of the script at the top of the script.

Makefile Show resolved Hide resolved
convergence: no module should emit "updates to go.mod needed" errors, and running a second time
should have no effect.

This is achieved by running the command on modules in topological order, ie. running it on
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider having this list generated in a file so we can consult it, and update it via script?

Copy link
Contributor Author

@jade-guiton-dd jade-guiton-dd Dec 11, 2024

Choose a reason for hiding this comment

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

That's probably a good idea, although I'll probably need to implement yet another CI check to prevent the committed version from being out-of-date.

Copy link
Contributor

Choose a reason for hiding this comment

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

No free lunch on that one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about outputting the list to a .gitignored file?

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.

2 participants