rubenv / sql-migrate

SQL schema migration tool for Go.
MIT License
3.18k stars 270 forks source link

use godror instead of oci8 #170

Closed gunsluo closed 4 years ago

gunsluo commented 4 years ago

use godror instead of oci8

gunsluo commented 4 years ago

@rubenv sorry for close previous PR by my mistake(https://github.com/rubenv/sql-migrate/pull/169).

Hi, @rubenv I run the following command and get this error in your master branch. I check go-sqlite3 should be enable CGO_ENABLED

CGO_ENABLED=0 GO111MODULE=on go build .
# github.com/mattn/go-sqlite3
../../../../../pkg/mod/github.com/mattn/go-sqlite3@v1.12.0/sqlite3_opt_preupdate.go:12:16: undefined: SQLiteConn
../../../../../pkg/mod/github.com/mattn/go-sqlite3@v1.12.0/sqlite3_opt_preupdate_omit.go:1
rubenv commented 4 years ago

That's the command-line tool, right?

It's the library that should build without cgo. For the CLI, we can't avoid it.

Also, I'd love to hear from users who currently use oci8. Will changing this break their code? If so: we can't change it. I don't want people's code to break because of a change here.

Maybe it's better to add this as an additional option rather than replacing oci8?

gunsluo commented 4 years ago

@rubenv

Also, I'd love to hear from users who currently use oci8.

we have two ways to use sql-migrate, run it on command line and as a library. for 1. it doesn't matter. just to successfully built sql-migrate.

  1. it's up to the user by itself. The following codes will succeed
import _ "github.com/godror/godror"
n, err := migrate.Exec(db, "oci8", migrations, migrate.Up)

or

import _ "github.com/mattn/go-oci8"
n, err := migrate.Exec(db, "oci8", migrations, migrate.Up)
gunsluo commented 4 years ago

sorry, import _ "github.com/mattn/go-oci8" is unsuccessful.

Maybe it's better to add this as an additional option rather than replacing oci8?

Maybe we need to add a new dialect godror, what do you think?

It's the library that should build without cgo. For the CLI, we can't avoid it.

Agree, but run CGO_ENABLED=0 GO111MODULE=on go build . in the currenly master branch and get an error. please try and check.

rubenv commented 4 years ago

Maybe we need to add a new dialect godror, what do you think?

Yup, same thing I was thinking, that won't break old code

Agree, but run CGO_ENABLED=0 GO111MODULE=on go build . in the currenly master branch and get an error. please try and check.

Which directory / go version are you running this with? Works just fine here.

Also added a check to CI, where it works as well: https://travis-ci.org/github/rubenv/sql-migrate/builds/669573072

gunsluo commented 4 years ago
git clone https://github.com/rubenv/sql-migrate.git
cd sql-migrate
CGO_ENABLED=0 GO111MODULE=on go build .

# github.com/mattn/go-sqlite3
../../../../../../pkg/mod/github.com/mattn/go-sqlite3@v1.12.0/sqlite3_opt_preupdate.go:12:16: undefined: SQLiteConn
../../../../../../pkg/mod/github.com/mattn/go-sqlite3@v1.12.0/sqlite3_opt_preupdate_omit.go:19:10: undefined: SQLiteConn

please try.

gunsluo commented 4 years ago

go version go version go1.14 darwin/amd64

rubenv commented 4 years ago
docker run --rm -ti golang bash
root@964b7eec7f70:/go# git clone https://github.com/rubenv/sql-migrate.git
Cloning into 'sql-migrate'...
remote: Enumerating objects: 50, done.
remote: Counting objects: 100% (50/50), done.
remote: Compressing objects: 100% (35/35), done.
remote: Total 899 (delta 24), reused 33 (delta 15), pack-reused 849
Receiving objects: 100% (899/899), 227.22 KiB | 738.00 KiB/s, done.
Resolving deltas: 100% (529/529), done.
root@964b7eec7f70:/go# cd sql-migrate/
root@964b7eec7f70:/go/sql-migrate# CGO_ENABLED=0 GO111MODULE=on go build -v .
go: downloading gopkg.in/gorp.v1 v1.7.2
go: extracting gopkg.in/gorp.v1 v1.7.2
go: finding gopkg.in/gorp.v1 v1.7.2
github.com/rubenv/sql-migrate/sqlparse
gopkg.in/gorp.v1
net
vendor/golang.org/x/net/http/httpproxy
net/textproto
crypto/x509
vendor/golang.org/x/net/http/httpguts
crypto/tls
net/http/httptrace
net/http
github.com/rubenv/sql-migrate
root@964b7eec7f70:/go/sql-migrate# echo $?
0

Works on my machine, in a clean Docker container and on CI.

Not sure why it behaves differently for you, but it shouldn't compile sqlite3 for the main library.

Please paste the output of go build -v or go build -v -x

gunsluo commented 4 years ago
[@-MacBook-Pro sql-migrate (master)]$ CGO_ENABLED=0 GO111MODULE=on go build -v -x .
WORK=/var/folders/w0/5cnk2_p90dbdczd14cxyxkr40000gn/T/go-build054187301
github.com/mattn/go-sqlite3
mkdir -p $WORK/b104/
cat >$WORK/b104/importcfg << 'EOF' # internal
# import config
packagefile crypto/sha1=/Users/luoji/go/pkg/darwin_amd64/crypto/sha1.a
packagefile crypto/sha256=/Users/luoji/go/pkg/darwin_amd64/crypto/sha256.a
packagefile crypto/sha512=/Users/luoji/go/pkg/darwin_amd64/crypto/sha512.a
packagefile database/sql=/Users/luoji/go/pkg/darwin_amd64/database/sql.a
packagefile database/sql/driver=/Users/luoji/go/pkg/darwin_amd64/database/sql/driver.a
packagefile errors=/Users/luoji/go/pkg/darwin_amd64/errors.a
packagefile fmt=/Users/luoji/go/pkg/darwin_amd64/fmt.a
packagefile reflect=/Users/luoji/go/pkg/darwin_amd64/reflect.a
packagefile strconv=/Users/luoji/go/pkg/darwin_amd64/strconv.a
packagefile time=/Users/luoji/go/pkg/darwin_amd64/time.a
EOF
cd /Users/luoji/gopath/pkg/mod/github.com/mattn/go-sqlite3@v1.12.0
/Users/luoji/go/pkg/tool/darwin_amd64/compile -o $WORK/b104/_pkg_.a -trimpath "$WORK/b104=>" -p github.com/mattn/go-sqlite3 -complete -buildid YxcjyNpZ5H-STZnmEhBt/YxcjyNpZ5H-STZnmEhBt -goversion go1.14 -D "" -importcfg $WORK/b104/importcfg -pack -c=4 ./convert.go ./doc.go ./sqlite3_func_crypt.go ./sqlite3_opt_preupdate.go ./sqlite3_opt_preupdate_omit.go ./static_mock.go
# github.com/mattn/go-sqlite3
../../../../../pkg/mod/github.com/mattn/go-sqlite3@v1.12.0/sqlite3_opt_preupdate.go:12:16: undefined: SQLiteConn
../../../../../pkg/mod/github.com/mattn/go-sqlite3@v1.12.0/sqlite3_opt_preupdate_omit.go:19:10: undefined: SQLiteConn

/Users/luoji/gopath/pkg/mod/github.com/mattn/go-sqlite3@v1.12.0

rubenv commented 4 years ago

That doesn't make any sense? What folder are you running this in?

gunsluo commented 4 years ago

Any directory, because I use go mod.

and I check the codes of go-sqlite3@v1.12.0. Some C code is in this package.

rubenv commented 4 years ago

No I'm also using go modules.

My feeling is that you're running this in the folder containing the executable. Not the project root.

gunsluo commented 4 years ago

yes, I am run the command on directory github.com/rubenv/sql-migrate/sql-migrate.

rubenv commented 4 years ago

That's normal, that's the CLI, which depends on SQLite.

Run it in github.com/rubenv/sql-migrate and it'll work without CGO. That's where the library lives.

gunsluo commented 4 years ago

I will implement dialect godror later.

gunsluo commented 4 years ago

@rubenv please view.

one more thing, do we use // +build ico8 instead of 1 // +build oracle. What do you think?

rubenv commented 4 years ago

one more thing, do we use // +build ico8 instead of 1 // +build oracle. What do you think?

We can't change that, people might already have that value in their build scripts

gunsluo commented 4 years ago

@rubenv please review.

gunsluo commented 4 years ago

@rubenv Hi, Can you merge this PR if there is no problem. because of we now cant use godror and sql-migrate.

rubenv commented 4 years ago

I think it looks good now, good to go!

rubenv commented 4 years ago

Thanks a lot for your hard work!