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

deps: fix dependency cycle with objx (again) #1567

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dolmen
Copy link
Collaborator

@dolmen dolmen commented Mar 5, 2024

Summary

Update go.mod to break dependency cycle with github.com/stretchr/objx v0.5.2 which depends on testify v1.9.0.

Changes

$ go mod edit -dropexclude=github.com/stretchr/testify@v1.8.0 -exclude=github.com/stretchr/testify@v1.8.4
$ go mod tidy

Motivation

Dependency pollution in downstream projects

Related issues

$ go mod edit -dropexclude=github.com/stretchr/testify@v1.8.0 -exclude=github.com/stretchr/testify@v1.8.4
$ go mod tidy

See stretchr/objx#140
@dolmen dolmen added the dependencies Pull requests that update a dependency file label Mar 5, 2024
@dolmen dolmen self-assigned this Mar 5, 2024
@dolmen dolmen changed the title deps: fix dependency cycle with objx deps: fix dependency cycle with objx (again) Mar 5, 2024
Copy link
Collaborator

@brackendawson brackendawson left a comment

Choose a reason for hiding this comment

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

Can you put a comment after line 10 to explain how to check this is correctly set for the next time dependabot updates objx?

@dolmen
Copy link
Collaborator Author

dolmen commented Mar 6, 2024

It is not yet clear to me if we will have to continuously upgrade the exclude line.

It is quite unfortunate that objx has been upgraded to v0.5.2 just before the testify v1.9.0 release. I had done the minimalist change needed in objx v0.5.1 but v0.5.2 destroyed that with a Go version bump in go.mod.

@brackendawson
Copy link
Collaborator

What is the purpose of this change?

If it is to minimise the size of the module graph; then the exclude should always be set to the version of testify which the version of objx we use imports.

@dolmen
Copy link
Collaborator Author

dolmen commented Mar 6, 2024

The purpose of this line is to cut the circular graph of dependencies of objx and testify, and tell go mod to not bother injecting older versions of testify in go.sum, and tell go mod applied on downstream projects to not bother with older versions of testify.

testify -> objx v0.5.2 -> testify v1.8.2 -> objx v0.5.0 -> testify v1.8.0 -> objx v0.4.0 -> testify v1.7.1 -> objx v0.1.0
(also each version of testify and objx brings its own versions of other dependencies)

@brackendawson
Copy link
Collaborator

How about?

require (
	github.com/davecgh/go-spew v1.1.1
	github.com/pmezard/go-difflib v1.0.0
	github.com/stretchr/objx v0.5.2 // to avoid a cycle the verision of testify used by objx should be excluded
	gopkg.in/yaml.v3 v3.0.1
)

@dolmen
Copy link
Collaborator Author

dolmen commented Mar 8, 2024

@brackendawson I don't understand what you suggest.

@brackendawson
Copy link
Collaborator

I am suggesting to add the above comment to line 10 of the go.mod file.

This change was missed because the person that merged the dependabot PR didn't know there was an additional requirement. The next person to review a dependabot PR may not be you or I. If there is a comment on the line then the reviewer is likely to notice?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants