search

xo / usql

Universal command-line interface for SQL databases
MIT License
8.88k stars 347 forks source link

Downgrade thrift (again) #347

Closed anthonyroussel closed 2 years ago

anthonyroussel commented 2 years ago

Hello :)

The usql build is failing with the all tag:

../../../go/pkg/mod/sqlflow.org/gohive@v0.0.0-20200521083454-ed52ee669b84/hiveserver2/gen-go/tcliservice/TCLIService.go:730:16: not enough arguments in call to iprot.ReadStructBegin
    have ()
    want (context.Context)
../../../go/pkg/mod/sqlflow.org/gohive@v0.0.0-20200521083454-ed52ee669b84/hiveserver2/gen-go/tcliservice/TCLIService.go:736:37: not enough arguments in call to iprot.ReadFieldBegin
    have ()
    want (context.Context)
[...]

This regression was introduced with commit 1f27d36e which upgrade Thrift from v0.13 to v0.14.2, see https://github.com/xo/usql/commit/1f27d36e

However since Thrift >= v0.13, there is a breaking change which was introduced, see https://github.com/apache/thrift/commit/e79f764f09afd

And sqlflow.org/gohive needs to Thrift <= v0.13 to work.

A resolution is to downgrade thrift to v0.13 again:

$ go get github.com/apache/thrift@0.13.0
downgraded github.com/apache/thrift v0.15.0 => v0.13.1-0.20191017214740-b75e88a33d67
downgraded github.com/beltran/gohive v1.5.2 => v1.3.0

Because this problem already occured and was fixed with https://github.com/xo/usql/pull/206, I propose to build usql with the all tag during the test pipeline to detect this kind of regression more easily :)

anthonyroussel commented 2 years ago

I also opened a pull-request to gohive to upgrade the Thrift version to v0.16.0 and fix this issue definitively :)

See https://github.com/sql-machine-learning/gohive/pull/70

I hope we can update sqlflow.org/gohive once this gohive's pull request is merged.

kenshaw commented 2 years ago

@anthonyroussel appreciate the work. Do you actually use this driver, or were you just testing with build -tags all?

anthonyroussel commented 2 years ago

@anthonyroussel appreciate the work. Do you actually use this driver, or were you just testing with build -tags all?

Thank you for your review! :)

No I don't use this driver.

My problem was that I could not update the usql package to v0.11 in nixpkgs because it builds usql with all the drivers by default:

kenshaw commented 2 years ago

If you're distributing usql, please don't build with all, as there are drivers that break output (ie, write logs to standard out and/or take over global logging, or have other unwanted side effects that don't play well with the other drivers) when imported.

I'd highly suggest using the set of tags within the build-release.sh:

    most    
    sqlite_app_armor    
    sqlite_fts5    
    sqlite_introspect    
    sqlite_json1    
    sqlite_stat4    
    sqlite_userauth    
    sqlite_vtable   
   no_adodb

I would even suggest using build-release.sh if you can within your build scripts.

anthonyroussel commented 2 years ago

If you're distributing usql, please don't build with all, as there are drivers that break output (ie, write logs to standard out and/or take over global logging, or have other unwanted side effects that don't play well with the other drivers) when imported.

I'd highly suggest using the set of tags within the build-release.sh:

    most    
    sqlite_app_armor    
    sqlite_fts5    
    sqlite_introspect    
    sqlite_json1    
    sqlite_stat4    
    sqlite_userauth    
    sqlite_vtable   
   no_adodb

I would even suggest using build-release.sh if you can within your build scripts.

Thanks for your advice! :)

I will check this to make sure that in nixpkgs we only use these tags to build usql.

Do you plan to release a v0.11.1 of usql soon?

kenshaw commented 2 years ago

@anthonyroussel likely this weekend if I have enough time. Unfortunately, I have to manually build the release artifacts on VMs because of the CGO dependencies. I was going to do it yesterday, but became busy with other development tasks.