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

Add MySql and Sqlite drivers/packages to CI testing #322

Closed
jamescheney opened this issue Jan 26, 2018 · 12 comments · Fixed by #893
Closed

Add MySql and Sqlite drivers/packages to CI testing #322

jamescheney opened this issue Jan 26, 2018 · 12 comments · Fixed by #893
Assignees
Milestone

Comments

@jamescheney
Copy link
Contributor

So that we can say that they are "supported" (and decrease the likelihood that they bit rot).

@dhil
Copy link
Member

dhil commented Jun 12, 2019

I believe the current status is as follows

  • Both PostgreSQL and SQLite3 get installed during CI
  • Only PostgreSQL is tested during CI
  • MySQL remains unsupported

@jamescheney
Copy link
Contributor Author

I think that's correct.

Testing SQLite 3 could just be a matter of adapting the existing tests to create/initialize and connect to a SQLite 3 database - theoretically both flat and shredded queries should work now thanks to #495.

@dhil
Copy link
Member

dhil commented Jun 12, 2019

Reeled in from #505

It seems Ubuntu 16.04 (as provided by Azure Pipelines) only offers SQLite3 version 3.11, whilst we require at least version 3.25 (c.f. release notes). So I suppose we need to find a third-party repository which offers a more recent version of SQLite3 before we can enable the shredding tests.

@jamescheney
Copy link
Contributor Author

SQLite 3.25 (which added support for ROW_NUMBER etc.) is only needed if we want to test shredding for SQLite3 on the CI. I suggest that we don't wait around indefinitely and add SQLite3 and MySQL CI support but just run the plain database tests, not the shredding ones. Whenever the default SQLite3 and/or MySQL versions available have support for the needed features, enabling the shredding tests for them should be a one-or-two-liner.

@jamescheney jamescheney modified the milestones: 0.9.0, Snooze, 0.9.1 Aug 7, 2019
@jamescheney jamescheney modified the milestones: 0.9.1, 0.9.2 Dec 13, 2019
@jamescheney jamescheney self-assigned this May 7, 2020
@jamescheney
Copy link
Contributor Author

It also looks like some work will be needed for the OCaml MySQL library to become compatible with Mysql 8...

ygrek/ocaml-mysql#13

so for now the ambition will just be to get the old mysql driver working again with Mysql 5.*, and adapt the CI to run / test the flat query examples with mysql and sqlite3.

@jamescheney
Copy link
Contributor Author

WIP in branch issue322 has reinstated the links-mysql package, with changes needed to make it compile. However, when attempting to dynamically load the links-mysql library I get this error:

links> @load "factorials.links";
compiling to IR
compiled IR
debug: found loadable database driver
- /Users/jcheney/src/links/_build/install/default/lib/links-mysql/links_mysql.cmxs
- /Users/jcheney/src/links/_build/install/default/share/links-mysql/links_mysql_dependencies.json

***: Error: Cannot load plugin dependency '/Users/jcheney/.opam/4.07.0/lib/mysql/mysql.cmxs' (link error: no implementation available for Stdlib__stdLabels)
 

I've tried to make the build process for links-mysql as close as possible to that for links-postgresql; the OPAM mysql installation is fresh. Any thoughts @dhil or @frank-emrich ?

@jamescheney
Copy link
Contributor Author

It seems the ocaml mysql library does use the StdLabels libraries (though not in any essential way) so presumably Dynlink is complaining because the rest of Links doesn't use them... I've tried a couple of ways to force these modules to be referenced but without success so far.

@jamescheney
Copy link
Contributor Author

Adding the mysql library to core/dune and adding a reference to it from somewhere in Links.core makes the dynamic loading succeed, which enables testing that the old mysql driver still works. However, we don't want to do this, both to avoid the unnecessary dependency and because the OPAM mysql package won't install unless mysql libraries are actually present, thus forcing an additional dependency on system libraries that we don't want.

I ran across the original code @Yuren-Zhong wrote to force loading of dependencies:

https://github.com/Yuren-Zhong/Modularizing_and_Dynamic_Linking_in_Links/blob/master/dynload.ml

but doing the same thing here doesn't work.

However, adding the -linkall flag to dune does work. @Yuren-Zhong's attempt to use this ran into a problem with cohttp that no longer appears to be an obstacle. That said, -linkall has its downsides too: for example the size of links.exe goes from 12MB to 15MB.

@dhil
Copy link
Member

dhil commented Jun 17, 2020

WIP in branch issue322 has reinstated the links-mysql package, with changes needed to make it compile. However, when attempting to dynamically load the links-mysql library I get this error:

This issue does not occur when compiling with OCaml 4.10.0. I think the issue here is that for some relative recent version of OCaml the standard library got repackaged. We can either bump the minimum required version of OCaml to compile Links or possibly try to be a bit more clever about dynamic loading for OCaml < 4.x where x is the minor version that repackaged the standard library.

@jamescheney
Copy link
Contributor Author

OK. I am using OCaml 4.07. Note however that since I wrote the comment you replied to on Sunday, I found a workaround using the -linkall flag. Is this flag present or absent in the root dune file in your checkout of branch issue322? If it is present, then does it still work if you remove it?

@dhil
Copy link
Member

dhil commented Jun 17, 2020

It is absent. I deliberately checked out an earlier version of your branch. I have tested with OCaml 4.10.0 (no error) and 4.06.1 (error occurs).

@jamescheney
Copy link
Contributor Author

Thanks! Since there's a workaround I don't think this is a sufficient reason for requiring a higher ocaml version but am happy with that resolution if everyone else prefers that.

If there's a way to make this flag conditional on ocaml version < 4.10 (which is known to not need it) then this would document that the flag can go away once Links's required ocaml version is >= 4.10.

@jamescheney jamescheney mentioned this issue Jul 10, 2020
8 tasks
jamescheney added a commit that referenced this issue Jul 15, 2020
* Adds a new run-database-tests script

* I'll take "Database Queries" for $1000, Alex.
SELECT 42 FROM (SELECT 42) x WHERE 1=0
What is a portable way of constructing an empty result?

* Add -linkall flag to release and ci

Needed for MySql driver dynamic linking in OCaml 4.06 due to dependency of StdLabels. This is apparently no longer needed in later versions such as 4.10 so can remove once Links depends on such a version.

* add sorting to relational lens assertions

* Add command line parameters to control which database(s) to run
tests for, and allow using the local configuration instead if
present.

Modify travis and azure CI scripts to run full database tests
covering all of the working configurations for each test script.

Modify run-tests so that instead of no-db and db-only arguments,
we just consider one argument which may be "database", "shredding",
or "relational-lenses".  If present run that database test suite
with --local enabled; otherwise, run all the tests.  This could
probably be cleaned up further but I wanted to try to stay close
to the existing functionality (while avoiding rerunning the basic
database tests three times as the previous setup does.)

* Database ci: get SQLite3 working (#896)

Allow sqlite multi statement execution
Change printing of booleans for compatibility with sqlite3

Co-authored-by: Rudi Horn <[email protected]>
frank-emrich pushed a commit to frank-emrich/links that referenced this issue Sep 16, 2020
* Adds a new run-database-tests script

* I'll take "Database Queries" for $1000, Alex.
SELECT 42 FROM (SELECT 42) x WHERE 1=0
What is a portable way of constructing an empty result?

* Add -linkall flag to release and ci

Needed for MySql driver dynamic linking in OCaml 4.06 due to dependency of StdLabels. This is apparently no longer needed in later versions such as 4.10 so can remove once Links depends on such a version.

* add sorting to relational lens assertions

* Add command line parameters to control which database(s) to run
tests for, and allow using the local configuration instead if
present.

Modify travis and azure CI scripts to run full database tests
covering all of the working configurations for each test script.

Modify run-tests so that instead of no-db and db-only arguments,
we just consider one argument which may be "database", "shredding",
or "relational-lenses".  If present run that database test suite
with --local enabled; otherwise, run all the tests.  This could
probably be cleaned up further but I wanted to try to stay close
to the existing functionality (while avoiding rerunning the basic
database tests three times as the previous setup does.)

* Database ci: get SQLite3 working (links-lang#896)

Allow sqlite multi statement execution
Change printing of booleans for compatibility with sqlite3

Co-authored-by: Rudi Horn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants