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

Module keeps the name github.com/gocql/gocql #161

Closed
karol-kokoszka opened this issue Mar 19, 2024 · 19 comments
Closed

Module keeps the name github.com/gocql/gocql #161

karol-kokoszka opened this issue Mar 19, 2024 · 19 comments
Assignees

Comments

@karol-kokoszka
Copy link

karol-kokoszka commented Mar 19, 2024

ScyllaDB fork of gocql driver, keeps the module name of the origin.
See

gocql/go.mod

Line 1 in 3c32c6c

module github.com/gocql/gocql

It leads to the situation, where whenever someone wants to add the dependency, cannot simply
go get github.com/scylladb/gocql, but must use replace directive as described here
https://github.com/scylladb/gocql/blob/master/README.md#installation

Why is that ?
Is the intention to keep this module temporary and to merge everything to the upstream ?
Otherwise, why we cannot have the separate name of the module ?
I.e. it would allow to read the documentation via https://pkg.go.dev/search?q=gocql&m=package

User may face problems having the module A that is dependent to github.com/scylladb/gocql and trying to make the dependency to A from another project.

@roydahan
Copy link
Collaborator

@avelanarius do you know what was the original reason for this desicion?

@piodul
Copy link
Collaborator

piodul commented Jun 17, 2024

I don't know whether that was the reason as I didn't make the decision, but if I had to guess it would be because of the ability to affect third-party modules. If you use the replace directive, third-party modules will also use our fork and will benefit from the ScyllaDB-specific optimizations.

Being able to replace the upstream module in the project requires our fork to keep the same module path. If our fork had a different module name then you would have to fork third-party libraries and modify them to use our fork - not sure if there is an easier way.

Ref: https://go.dev/ref/mod#go-mod-file-replace

@sylwiaszunejko
Copy link
Collaborator

@karol-kokoszka is @piodul reasoning enough for your or the module name is still something we should consider changing in your opinion?

@karol-kokoszka
Copy link
Author

Being able to replace the upstream module in the project requires our fork to keep the same module path. If our fork had a different module name then you would have to fork third-party libraries and modify them to use our fork - not sure if there is an easier way.

This is true. However, it is still possible if the ScyllaDB fork of gocql changes the name of the module to, for example:

module github.com/scylladb/gocql

The replace directive would then be:

replace github.com/gocql/gocql => github.com/scylladb/gocql vX.Y.Z

This approach would have been preferable from the beginning. A different module name does not prevent the use of the replace directive.

By changing the module name, we confirm that our gocql driver is not just a temporary repository with the intention to merge everything upstream and close the fork. Based on the comparison (apache/cassandra-gocql-driver@master...scylladb:gocql:master), the fork is 189 commits ahead of the master. It doesn't seem likely that these changes will be merged upstream.

With the current approach of keeping the original module name, if a project (let's call it Project A) depends on github.com/scylladb/gocql via the replace directive, and another project (Project B) wants to use Project A as a dependency, Project B might also need to include the replace directive. This can lead to confusion and complexity, especially as the dependency tree grows.

Using a separate module name would make documentation easier to navigate and discover via tools like pkg.go.dev.

@karol-kokoszka
Copy link
Author

karol-kokoszka commented Jul 2, 2024

If we would have the module name github.com/scylladb/gocql, user can just go get github.com/scylladb/gocql and use it in imports or use replace directive.
So he has alternative.

This change is rather no invasive. End user can keep the replace directive even if the module name is changed.

@sylwiaszunejko
Copy link
Collaborator

makes sense, @roydahan FYI

@mykaul
Copy link

mykaul commented Jul 3, 2024

BTW, what version does it advertise then? For example, is it now 1.14.1 in the STARTUP message? This is critical.

@roydahan
Copy link
Collaborator

roydahan commented Jul 3, 2024

makes sense, @roydahan FYI

Thx.
I quite agree, especially since the upstream gocql project is quite dead and we further ahead of it.

One thing to consider is versioning and the upcoming change to v3, we can possibly connect all these together just for the ease of differentiation for users.

@dkropachev
Copy link
Collaborator

@roydahan , to make it clear, did we decide to change it to scylladb/gocql ?

@roydahan
Copy link
Collaborator

I think it's the ideal option and name, but depends on your findings of what's needed to be done.

@dkropachev
Copy link
Collaborator

dkropachev commented Jul 31, 2024

Changing module name is a breaking change, users will have to remove "replace" directive from go.mod and change import path from github.com/gocql/gocql to github.com/scylladb/gocql in all files.
I think it will be appropriate to follow it with bumping up version major, in total go.mod header in out repo is going to look like this:

module github.com/scylladb/gocql/v2

@karol-kokoszka , @sylwiaszunejko , @roydahan , @Lorak-mmk are you guys ok with that

@Lorak-mmk
Copy link

Changing module name is a breaking change, users will have to remove "replace" directive from go.mod and change import path from github.com/gocql/gocql to github.com/scylladb/gocql in all files. I think it will be appropriate to follow it with bumping up version major, in total go.mod header in out repo is going to look like this:

module github.com/scylladb/gocql/v2

@karol-kokoszka , @sylwiaszunejko , @roydahan , @Lorak-mmk are you guys ok with that

I'm not very familiar with go and gocql - I don't know what stability guarantees gocql has, I don't know if semver should be used here properly as it is in Rust (because if it is, we can make a breaking change, but we need to increase major version number), and I don't know what are the typical expectations of a Go library in this regard.
Someone more familiar with Go ecosystem should decide.

@sylwiaszunejko
Copy link
Collaborator

If we would have the module name github.com/scylladb/gocql, user can just go get github.com/scylladb/gocql and use it in imports or use replace directive.
So he has alternative.
This change is rather no invasive. End user can keep the replace directive even if the module name is changed.

@dkropachev What you say differs a lot from the conclusion that @karol-kokoszka had before

@dkropachev
Copy link
Collaborator

dkropachev commented Jul 31, 2024

If we would have the module name github.com/scylladb/gocql, user can just go get github.com/scylladb/gocql and use it in imports or use replace directive.
So he has alternative.
This change is rather no invasive. End user can keep the replace directive even if the module name is changed.

@dkropachev What you say differs a lot from the conclusion that @karol-kokoszka had before

I took gocqlx and changed it to use gocql with changed module name, and end up wtih following error:

# go mod tidy
go: github.com/scylladb/gocqlx/v3 imports
        github.com/gocql/gocql imports
        github.com/scylladb/gocql: github.com/scylladb/[email protected]: parsing go.mod:
        module declares its path as: github.com/gocql/gocql
                but was required as: github.com/scylladb/gocql

If I remove replace directive from go.mod and use github.com/scylladb/gocql directly it works great.
So, we can't have both.

@dkropachev dkropachev added enhancement and removed bug labels Aug 1, 2024
@karol-kokoszka
Copy link
Author

I took gocqlx and changed it to use gocql with changed module name, and end up wtih following error:

go mod tidy
go: github.com/scylladb/gocqlx/v3 imports
github.com/gocql/gocql imports
github.com/scylladb/gocql: github.com/scylladb/[email protected]: parsing go.mod:
module declares its path as: github.com/gocql/gocql
but was required as: github.com/scylladb/gocql

The problem here is that the go mod is looking for old version of scylladb/gocql which is tagged with 1.14.2. This version still keeps the old module name.
That's why the error is:

        module declares its path as: github.com/gocql/gocql
                but was required as: github.com/scylladb/gocql

Out of curiousity I've made another test where I forked the github.com/scylladb/gocql repo to https://github.com/karol-kokoszka/gocql , changed the module name and created explicit tag v1.0.0
See https://github.com/karol-kokoszka/gocql/tags

Having that imported to go.mod of gocqlx in replace directive works fine
The go.mod is:

module github.com/scylladb/gocqlx/v3

go 1.17

require (
	github.com/gocql/gocql v0.0.0-20211015133455-b225f9b53fa1
	github.com/google/go-cmp v0.6.0
	github.com/psanford/memfs v0.0.0-20210214183328-a001468d78ef
	github.com/scylladb/go-reflectx v1.0.1
	golang.org/x/sync v0.7.0
	gopkg.in/inf.v0 v0.9.1
)

require (
	github.com/golang/snappy v0.0.4 // indirect
	github.com/hailocab/go-hostpool v0.0.0-20160125115350-e80d13ce29ed // indirect
)

replace github.com/gocql/gocql => github.com/karol-kokoszka/gocql v1.0.0

and the go mod tidy works OK.

➜  gocqlx git:(master) ✗ go mod download
➜  gocqlx git:(master) ✗ go mod tidy
➜  gocqlx git:(master) ✗ 

The repo keeps the new module name:
https://github.com/karol-kokoszka/gocql/blob/704507ae45d7a32f276f8010bfac950a1dc141a3/go.mod#L1

@dkropachev
Copy link
Collaborator

@karol-kokoszka , looks like there is some issue in module proxy server.

I have started a new repo, making sure it does have exactly same tags as scylladb/gocql:

git clone [email protected]:scylladb/gocql.git
git remote add test [email protected]:scylladb-solutions/gocql.git
git fetch test
git push --tags -u test

Then I have pushed commit to test/master, with changing it's module name:

git tag v1.15.0
git push -u test --tags -f

Then I went to gocqlx and updated it to use github.com/scylladb-solutions/[email protected]:

replace github.com/gocql/gocql => github.com/scylladb-solutions/gocql v1.15.0

And run go mod tidy:

# go mod tidy ; echo $?
go: finding module for package github.com/scylladb-solutions/gocql
go: finding module for package github.com/scylladb-solutions/gocql/internal/murmur
go: finding module for package github.com/scylladb-solutions/gocql/scyllacloud
go: finding module for package github.com/scylladb-solutions/gocql/internal/streams
go: finding module for package github.com/scylladb-solutions/gocql/internal/lru
go: found github.com/scylladb-solutions/gocql in github.com/scylladb-solutions/gocql v1.15.0
go: found github.com/scylladb-solutions/gocql/internal/lru in github.com/scylladb-solutions/gocql v1.15.0
go: found github.com/scylladb-solutions/gocql/internal/murmur in github.com/scylladb-solutions/gocql v1.15.0
go: found github.com/scylladb-solutions/gocql/internal/streams in github.com/scylladb-solutions/gocql v1.15.0
go: found github.com/scylladb-solutions/gocql/scyllacloud in github.com/scylladb-solutions/gocql v1.15.0
go: github.com/scylladb-solutions/[email protected] used for two different module paths (github.com/gocql/gocql and github.com/scylladb-solutions/gocql)
1

As you can see it ended up in failure and go.sum was not updated.
And go build . fails as expected:

# go build . ; echo $? 
/home/dmitry.kropachev/go/pkg/mod/github.com/scylladb-solutions/[email protected]/conn.go:22:2: no required module provides package github.com/scylladb-solutions/gocql/internal/lru; to add it:
        go get github.com/scylladb-solutions/gocql/internal/lru
/home/dmitry.kropachev/go/pkg/mod/github.com/scylladb-solutions/[email protected]/token.go:16:2: no required module provides package github.com/scylladb-solutions/gocql/internal/murmur; to add it:
        go get github.com/scylladb-solutions/gocql/internal/murmur
/home/dmitry.kropachev/go/pkg/mod/github.com/scylladb-solutions/[email protected]/conn.go:23:2: no required module provides package github.com/scylladb-solutions/gocql/internal/streams; to add it:
        go get github.com/scylladb-solutions/gocql/internal/streams
gocqlx.go:13:2: missing go.sum entry for module providing package github.com/scylladb/go-reflectx (imported by github.com/scylladb/gocqlx/v3); to add:
        go get github.com/scylladb/gocqlx/v3
1

@karol-kokoszka
Copy link
Author

We checked the issue again with @dkropachev .
The example that I gave missed one step - changing the imports in the forked gocql to the new name.

After applying this change, I end up with the same error.

go: github.com/karolorg/[email protected] used for two different module paths (github.com/gocql/gocql and github.com/karolorg/gocql)

It appears that changing the module name may be the breaking change here indeed.

@karol-kokoszka
Copy link
Author

To sum up.

replace directive is not the alternative to import with the new module name.

@dkropachev
Copy link
Collaborator

Great, now we established that it is a breaking change, only proper way to release it is to make it part of major change.
I don't want it to be only change for a v2.
I am closing this issue, let's continue discussion about v2 in here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants