scylladb / gocql

Package gocql implements a fast and robust ScyllaDB client for the Go programming language.
https://docs.scylladb.com/stable/using-scylla/drivers/cql-drivers/scylla-go-driver.html
BSD 3-Clause "New" or "Revised" License
181 stars 57 forks source link

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

Closed karol-kokoszka closed 1 month ago

karol-kokoszka commented 6 months ago

ScyllaDB fork of gocql driver, keeps the module name of the origin. See https://github.com/scylladb/gocql/blob/3c32c6cd75e529433ed0885d8c04ec4c96cf22a9/go.mod#L1

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 commented 6 months ago

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

piodul commented 3 months ago

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 commented 3 months ago

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

karol-kokoszka commented 3 months ago

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 (https://github.com/gocql/gocql/compare/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 commented 3 months ago

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 commented 3 months ago

makes sense, @roydahan FYI

mykaul commented 3 months ago

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

roydahan commented 3 months ago

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 commented 2 months ago

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

roydahan commented 2 months ago

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

dkropachev commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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/gocql@v1.14.2: 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.

karol-kokoszka commented 1 month ago

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/gocql@v1.14.2: 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 commented 1 month ago

@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 git@github.com:scylladb/gocql.git
git remote add test git@github.com: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/gocql@v.15.0:

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/gocql@v1.15.0 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/gocql@v1.15.0/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/gocql@v1.15.0/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/gocql@v1.15.0/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 commented 1 month ago

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/gocql@v1.15.0 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 commented 1 month ago

To sum up.

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

dkropachev commented 1 month ago

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