Skip to content
This repository has been archived by the owner on Jun 28, 2018. It is now read-only.

adapted the sqlite driver for v3 (#165) #238

Merged
merged 2 commits into from Jun 5, 2017
Merged

adapted the sqlite driver for v3 (#165) #238

merged 2 commits into from Jun 5, 2017

Conversation

maxvw
Copy link
Contributor

@maxvw maxvw commented May 27, 2017

When I discovered your project I thought it was perfect, just what I needed! However soon after that I discovered the latest version lacked SQLite support. So I've copied the Ql driver and adapted it to work for SQLite, I think I've covered everything but if I forgot something please let me know!

I wasn't sure which naming convention to use, in the previous version I see it was called sqlite3 but in the new version there was a directory reserved called sqlite. I've opted for the sqlite3 naming as the actual driver is also called sqlite3 so I think that made more sense.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.6%) to 60.128% when pulling 8b4ce58 on maxvw:v3-sqlite into 3682bcf on mattes:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+1.6%) to 60.128% when pulling 8b4ce58 on maxvw:v3-sqlite into 3682bcf on mattes:master.

@bt
Copy link

bt commented May 30, 2017

Hey @mattes, I desperately need this functionality. Could you please merge this PR in?

Thank you! 😄

@mattes
Copy link
Owner

mattes commented May 30, 2017

can you add the build file (ex https://github.com/mattes/migrate/blob/master/cli/build_ql.go) for this driver, please?

thanks for the PR! will merge after this!

@coveralls
Copy link

Coverage Status

Coverage increased (+1.6%) to 60.128% when pulling decee4a on maxvw:v3-sqlite into 3682bcf on mattes:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+1.6%) to 60.128% when pulling decee4a on maxvw:v3-sqlite into 3682bcf on mattes:master.

@maxvw
Copy link
Contributor Author

maxvw commented May 30, 2017

@mattes done, that should work now too.

(I had completely overlooked it as I only use the functionality in Go itself, not the command-line version)

@mattes mattes merged commit c3d6185 into mattes:master Jun 5, 2017
@mattes
Copy link
Owner

mattes commented Jun 5, 2017

merged! thank you so much!

@mattes
Copy link
Owner

mattes commented Jun 5, 2017

@maxvw I see some errors: https://travis-ci.org/mattes/migrate/jobs/239468284#L1513 Any idea what's going on?

@maxvw
Copy link
Contributor Author

maxvw commented Jun 5, 2017

@mattes on first inspection this seems to be due to the fact that go-sqlite3 is a cgo package, I'm still relatively new to Go and didn't know this was a thing / wasn't aware of the impact that has when it comes to cross-compiling. I'm not sure what the fix would be to make this work right now, from initial reading it sounds like you need to set the correct CC env for each GOOS and also set CGO_ENABLED=1.

Someone mentioning a similar issue: mattn/go-sqlite3#384

But if it makes cross-compiling more challenging people will also have this issue in their own project if they include the migrate library, right? So the question is if that's desired or not. I have not found a better sqlite3 go package unfortunately (this is also the same one you originally used in v1)

@mattes
Copy link
Owner

mattes commented Jun 5, 2017

ok, thanks! I just enabled CGO based on: mattn/go-sqlite3@98981b4

@maxvw
Copy link
Contributor Author

maxvw commented Jun 5, 2017

@mattes alright nice, hopefully that solves it!

dhui added a commit to golang-migrate/migrate that referenced this pull request Jan 21, 2018
    - Removed sqlite3 support from binaries
        - It was never working anyways: mattes/migrate#244
    - Don't use cgo as is requires a cross-compiler to build for other platforms
        - cgo was originally added for this reason: mattes/migrate#238 (comment)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants