Skip to content

use godror instead of oci8#170

Merged
rubenv merged 3 commits intorubenv:masterfrom
gunsluo:master
Apr 2, 2020
Merged

use godror instead of oci8#170
rubenv merged 3 commits intorubenv:masterfrom
gunsluo:master

Conversation

@gunsluo
Copy link
Copy Markdown
Contributor

@gunsluo gunsluo commented Apr 1, 2020

use godror instead of oci8

@gunsluo
Copy link
Copy Markdown
Contributor Author

gunsluo commented Apr 1, 2020

@rubenv sorry for close previous PR by my mistake(#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
Copy link
Copy Markdown
Owner

rubenv commented Apr 1, 2020

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
Copy link
Copy Markdown
Contributor Author

gunsluo commented Apr 1, 2020

@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.
2. 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
Copy link
Copy Markdown
Contributor Author

gunsluo commented Apr 1, 2020

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
Copy link
Copy Markdown
Owner

rubenv commented Apr 1, 2020

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
Copy link
Copy Markdown
Contributor Author

gunsluo commented Apr 1, 2020

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
Copy link
Copy Markdown
Contributor Author

gunsluo commented Apr 1, 2020

go version
go version go1.14 darwin/amd64

@rubenv
Copy link
Copy Markdown
Owner

rubenv commented Apr 1, 2020

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
Copy link
Copy Markdown
Contributor Author

gunsluo commented Apr 1, 2020

[@-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
Copy link
Copy Markdown
Owner

rubenv commented Apr 1, 2020

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

@gunsluo
Copy link
Copy Markdown
Contributor Author

gunsluo commented Apr 1, 2020

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
Copy link
Copy Markdown
Owner

rubenv commented Apr 1, 2020

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
Copy link
Copy Markdown
Contributor Author

gunsluo commented Apr 1, 2020

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

@rubenv
Copy link
Copy Markdown
Owner

rubenv commented Apr 1, 2020

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
Copy link
Copy Markdown
Contributor Author

gunsluo commented Apr 1, 2020

I will implement dialect godror later.

Signed-off-by: luoji <gunsluo@gmail.com>
@gunsluo
Copy link
Copy Markdown
Contributor Author

gunsluo commented Apr 1, 2020

@rubenv please view.

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

Signed-off-by: luoji <gunsluo@gmail.com>
@rubenv
Copy link
Copy Markdown
Owner

rubenv commented Apr 1, 2020

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

Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread migrate.go Outdated
Comment thread sql-migrate/oracle.go Outdated
Signed-off-by: luoji <gunsluo@gmail.com>
@gunsluo
Copy link
Copy Markdown
Contributor Author

gunsluo commented Apr 2, 2020

@rubenv please review.

@gunsluo
Copy link
Copy Markdown
Contributor Author

gunsluo commented Apr 2, 2020

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

@rubenv
Copy link
Copy Markdown
Owner

rubenv commented Apr 2, 2020

I think it looks good now, good to go!

@rubenv rubenv merged commit 797e86f into rubenv:master Apr 2, 2020
@rubenv
Copy link
Copy Markdown
Owner

rubenv commented Apr 2, 2020

Thanks a lot for your hard work!

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