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

Rust packaging #1741

Merged
merged 1 commit into from
Jul 4, 2023
Merged

Rust packaging #1741

merged 1 commit into from
Jul 4, 2023

Conversation

benjaminwinger
Copy link
Collaborator

I fixed a few things to get rust crates working properly:

  • The kuzu-src symlink seems to mess with how cargo package ignores untracked files in git repositories, which would pull in things like build files, so I added a file include list to make sure only the necessary source files are included in the crate.
  • Technically rust crates should only modify files within the directory indicated by the OUT_DIR environment variable, so it should really be an out of source build. As such, I set it up to run the arrow and kuzu cmake builds manually within build.rs instead of running them through the Makefile.
  • I added a way to link against the release binaries using environment variables to point to their path (documented in the root of the rustdocs).

Additionally:

  • I noticed that I used the tempdir crate (abandoned and merged into tempfile) instead of the tempfile crate in the tests, so I replaced that.
  • I made building the shell and python API optional (enabled by default to match the current behaviour) so that the rust crate doesn't need to build them when building from source (which also removes the python dependency when using the rust API).

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (8f029ed) 91.39% compared to head (d2a2482) 91.39%.

❗ Current head d2a2482 differs from pull request most recent head 6173f97. Consider uploading reports for the commit 6173f97 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1741   +/-   ##
=======================================
  Coverage   91.39%   91.39%           
=======================================
  Files         759      760    +1     
  Lines       27416    27433   +17     
=======================================
+ Hits        25056    25072   +16     
- Misses       2360     2361    +1     

see 37 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@benjaminwinger
Copy link
Collaborator Author

It looks like doing the rust build inside the correct build directory means that it's too long (there's a 260 character limit for paths on windows by default).

The path that failed was: C:/actions-runner/1/_work/kuzu/kuzu/tools/rust_api/target/debug/build/kuzu-d1344c3c27bf3fe9/out/build-arrow/build/arrow/src/apache_arrow-build/zlib_ep-prefix/src/zlib_ep-build/CMakeFiles/CMakeScratch/TryCompile-b41ewj/CMakeFiles/cmTC_67d44.dir/./.

It looks like external projects use a lot of nested directories, and kuzu builds arrow as an external project; arrow builds zstd as an external project, and zstd might also be building something else, I can't tell (cmake tries to reduce the length of the path by hashing the names).

I removed the path length restriction on the runner (possible from a particular build of windows 10 onwards), so building the rust API on windows may just require either that, or linking against a release build.

@benjaminwinger
Copy link
Collaborator Author

I don't think enabling the long paths worked. Apparently the software may need to be "long path aware", which might be the issue.
I've tried reducing the length of the build path in CI instead.

- Make building the shell and python API optional
- Switch rust test dependency tempdir to tempfile
  tempdir is an archived package which was merged into tempfile
- Only include necessary files in rust crate
- un-bundle rust build
- Build kuzu out of source when doing rust build
- Use shorter paths in windows CI to work around path length limits
@benjaminwinger benjaminwinger merged commit 7ff1fe5 into master Jul 4, 2023
@benjaminwinger benjaminwinger deleted the rust-packaging branch July 4, 2023 21:21
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.

2 participants