Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update go-sqlite3 #2209

Closed
wants to merge 1 commit into from
Closed

update go-sqlite3 #2209

wants to merge 1 commit into from

Conversation

wkloucek
Copy link
Contributor

@wkloucek wkloucek commented Jun 23, 2021

Description

This PR would remove the annoying / confusing SQLite compile warning upon compiling oCIS.

go build ./cmd/ocis
# github.com/mattn/go-sqlite3
sqlite3-binding.c: In function ‘sqlite3SelectNew’:
sqlite3-binding.c:128049:10: warning: function may return address of local variable [-Wreturn-local-addr]
128049 |   return pNew;
       |          ^~~~
sqlite3-binding.c:128009:10: note: declared here
128009 |   Select standin;
       |          ^~~~~~~

Background

github.com/mattn/go-sqlite3 has some weird versions. The highest version number is v2.0.5, but the newest version is v1.14.7. This leads to a very chaotic situation. Even https://github.com/gobuffalo/pop/blob/master/go.mod does use a replace directive...

Considerations

  • overriding a dependency is never nice
  • create PRs at the libraries using the old version of the sqlite lib
  • the compile time warning is not too bad

I wouldn't even know where to start in our upstream libs to update go-sqlite. go mod why github.com/mattn/go-sqlite3 for oCIS gives us:

# github.com/mattn/go-sqlite3
github.com/owncloud/ocis/ocis/pkg/command
github.com/owncloud/ocis/storage/pkg/command
github.com/cs3org/reva/cmd/revad/runtime
github.com/cs3org/reva/pkg/storage/fs/loader
github.com/cs3org/reva/pkg/storage/fs/local
github.com/cs3org/reva/pkg/storage/utils/localfs
github.com/mattn/go-sqlite3

So this is coming from REVA. go mod why gives us:

go mod why github.com/mattn/go-sqlite3
# github.com/mattn/go-sqlite3
github.com/cs3org/reva/pkg/storage/utils/localfs
github.com/mattn/go-sqlite3

Which is only the half truth (direct deps) and go mod graph | grep sqlite shows the full picture:

github.com/cs3org/reva github.com/mattn/[email protected]+incompatible
github.com/gobuffalo/pop/[email protected] github.com/mattn/[email protected]+incompatible
github.com/jmoiron/[email protected] github.com/mattn/[email protected]
github.com/gobuffalo/[email protected] github.com/mattn/[email protected]+incompatible
github.com/gobuffalo/[email protected] github.com/mattn/[email protected]+incompatible
github.com/luna-duclos/[email protected] github.com/mattn/[email protected]
github.com/gobuffalo/pop/[email protected] github.com/mattn/[email protected]+incompatible
github.com/ory/[email protected] github.com/mattn/[email protected]
github.com/gobuffalo/[email protected] github.com/mattn/[email protected]

You will notice github.com/gobuffalo/pop again which itself uses a replace directive...

My first attempt was a go get github.com/mattn/[email protected] in REVA, but that leads to a downgrade of github.com/ory/fosite, which lacks some interfaces needed by REVA.
If you check why fosite needs go-sqlite3:

go mod graph | grep sqlite
github.com/gobuffalo/pop/[email protected] github.com/mattn/[email protected]+incompatible
github.com/jmoiron/[email protected] github.com/mattn/[email protected]
github.com/gobuffalo/[email protected] github.com/mattn/[email protected]+incompatible
github.com/gobuffalo/[email protected] github.com/mattn/[email protected]+incompatible
github.com/luna-duclos/[email protected] github.com/mattn/[email protected]
github.com/gobuffalo/pop/[email protected] github.com/mattn/[email protected]+incompatible
github.com/ory/[email protected] github.com/mattn/[email protected]
github.com/gobuffalo/[email protected] github.com/mattn/[email protected]

You will again notice github.com/gobuffalo/pop ....

@wkloucek wkloucek force-pushed the update_go-sqlite3 branch from 78a47c4 to bb21573 Compare June 23, 2021 13:01
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@wkloucek
Copy link
Contributor Author

wkloucek commented Jul 2, 2021

@refs what do you think about this? should we just stick the the compile time warning or should we go the replace step? I don't think that we can solve this on some upstream lib since it is a huuuge mess.

@butonic
Copy link
Member

butonic commented Jul 2, 2021

@labkode @ishank011 I think we should remove the ory fosite based openidprovider in reva. It really was a temporary hack until a proper idp was available.

@wkloucek
Copy link
Contributor Author

wkloucek commented Jul 2, 2021

@labkode @ishank011 I think we should remove the ory fosite based openidprovider in reva. It really was a temporary hack until a proper idp was available.

That would also work: cs3org/reva#1860

@refs
Copy link
Member

refs commented Jul 2, 2021

I think that'd be the closest solution. I'm sure someone®️ has a replace on an obscure branch just like etcd had, but that's an insidious option. I'd +1 @butonic proposal

@wkloucek
Copy link
Contributor Author

was solved in reva

@wkloucek wkloucek closed this Jul 19, 2021
@wkloucek wkloucek deleted the update_go-sqlite3 branch July 19, 2021 14:41
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.

3 participants