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

Implement linux target #38

Merged
merged 14 commits into from
Jun 24, 2021

Conversation

chippmann
Copy link
Contributor

@chippmann chippmann commented Feb 26, 2021

This implements linuxX64 as a target with the corresponding linux specific actual implementations.
Primarily this was implemented to hopefully pave the way for cashapp/sqldelight#1835.

I did some minor testing with a local build of SQLDelight and so far everything seems to work.
I did not figure out how your test setup with that arbitrary script could work and based on @kpgalligan's comment on the above mentioned issue I did not spend too much time on it as it's probably rewritten anyways? But if you require it I'm of course willing to spend some more time to figure that one out as well. Tests are run after latest rebase.

}

internal fun databaseDirPath(): String {
return getHomeDirPath() ?: memScoped {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually here I'm not sure where the fallback path should lead to?
I just defined the home dir but of course this is suboptimal. But another arbitrary fallback dir is also IMO. But you did something similar on windows as well and one should configure the path anyways in most cases.
Maybe you have a preference where the fallback path should be? Or is home fine for you?

@chippmann chippmann force-pushed the feature/support_linux_target branch 2 times, most recently from 894ccd3 to ea5fc0b Compare April 26, 2021 06:58
@chippmann
Copy link
Contributor Author

chippmann commented Apr 26, 2021

Any chance this could get looked at or is there no interest in it?

@chippmann chippmann force-pushed the feature/support_linux_target branch from 6a06e05 to 046bb60 Compare April 26, 2021 07:43
@carlonzo
Copy link

carlonzo commented May 6, 2021

@kpgalligan any chance we can have this reviewed/merged? would be great to support Linux targets on SQLDelight

@chippmann chippmann force-pushed the feature/support_linux_target branch from 046bb60 to e96d980 Compare May 6, 2021 11:07
@kpgalligan
Copy link
Contributor

I tried running tests and got a failure with the following:

e: /home/runner/.konan/dependencies/x86_64-unknown-linux-gnu-gcc-8.3.0-glibc-2.19-kernel-4.9-2/x86_64-unknown-linux-gnu/bin/ld.gold invocation reported errors

> Task :sqliter-driver:linkDebugTestLinuxX64 FAILED
The /home/runner/.konan/dependencies/x86_64-unknown-linux-gnu-gcc-8.3.0-glibc-2.19-kernel-4.9-2/x86_64-unknown-linux-gnu/bin/ld.gold command returned non-zero exit code: 1.
output:
/usr/lib/x86_64-linux-gnu/libsqlite3.so: error: undefined reference to 'log', version 'GLIBC_2.29'
/usr/lib/x86_64-linux-gnu/libsqlite3.so: error: undefined reference to 'fcntl64', version 'GLIBC_2.28'

Can provide more log if needed.

@kpgalligan
Copy link
Contributor

To clarify above, I made another branch with your changes and pushed to get the linux tests running (rather than set up a local virtual machine). The error above is from the test log. I have roughly zero linux dev experience that wasn't on the JVM, so it would probably take me a bit to sort out that issue. If you have any thoughts, please share.

@audkar
Copy link

audkar commented Jun 24, 2021

Were libsqlite or lib binaries prebuilt on other CI instance/machine? Probably some binary was built on OS which had newer glibc. To ensure compatibility need to ensure that everything is built on older OS with older glibc

@chippmann
Copy link
Contributor Author

Hmm to me it seems the GLIB_C version mismatches. Probably the konan toolchain uses a different one now. It probably broke with my last rebase as there the kotlin version changed to 1.5.x IIRC and i didn't notice it.
I'll look into it this weekend.

@chippmann
Copy link
Contributor Author

Were libsqlite or lib binaries prebuilt on other CI instance/machine? Probably some binary was built on OS which had newer glibc. To ensure compatibility need to ensure that everything is built on older OS with older glibc

The problem with this is that konan (AFAIK) bundles fixed versions: https://github.com/JetBrains/kotlin-native/blob/1d2f55ce23d050cad2f3611a61261bf6a509a36f/konan/konan.properties#L411
Not sure if one can change the version used.

@chippmann
Copy link
Contributor Author

It seems @audkar was right. Simply using ubuntu 18.04 to build it was enough.
I updated the workflows accordingly.
@kpgalligan Could you either approve the workflow in this PR or trigger it yourself to test whether this also works in CI?

@kpgalligan
Copy link
Contributor

Just tried the same thing and it seems like it's working: https://github.com/touchlab/SQLiter/actions/runs/968436878. I'll let that finish then approve.

@chippmann
Copy link
Contributor Author

Just tried the same thing and it seems like it's working: https://github.com/touchlab/SQLiter/actions/runs/968436878. I'll let that finish then approve.

Very nice!
Thx a lot!

@kpgalligan kpgalligan merged commit 8655cff into touchlab:main Jun 24, 2021
@chippmann chippmann deleted the feature/support_linux_target branch June 24, 2021 15:46
@kpgalligan
Copy link
Contributor

Thanks for the PR. Sorry for the delay. Going to see about getting this into SQLDelight.

@chippmann
Copy link
Contributor Author

Thanks for the PR. Sorry for the delay. Going to see about getting this into SQLDelight.

No problem.
Once a release is done, I'll create a PR in the SQLDelight repo.

@chippmann
Copy link
Contributor Author

@kpgalligan it seems i forgot to update one linux-latest for the deploy:

if: matrix.os == 'ubuntu-latest'

Sorry about that.
Do you update it?

@kpgalligan
Copy link
Contributor

I'll update...

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.

None yet

4 participants