libelektra icon indicating copy to clipboard operation
libelektra copied to clipboard

Add elektra-rs to the repository

Open lawli3t opened this issue 2 years ago • 5 comments

Basics

  • [ ] Short descriptions of your changes are in the release notes (added as entry in doc/news/_preparation_next_release.md which contains _(my name)_) Please always add something to the release notes.
  • [ ] Details of what you changed are in commit messages (first line should have module: short statement syntax)
  • [ ] References to issues, e.g. close #X, are in the commit messages.
  • [ ] The buildservers are happy. If not, fix in this order:
    • [ ] add a line in doc/news/_preparation_next_release.md
    • [ ] reformat the code with scripts/dev/reformat-all
    • [ ] make all unit tests pass
    • [ ] fix all memleaks
  • [ ] The PR is rebased with current master.

Checklist

  • [ ] I added unit tests for my code
  • [ ] I fully described what my PR does in the documentation (not in the PR description)
  • [ ] I fixed all affected documentation
  • [ ] I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
  • [ ] I updated all meta data (e.g. README.md of plugins and METADATA.ini)
  • [ ] I mentioned every code not directly written by me in reuse syntax

Review

Labels

  • [ ] Add the "work in progress" label if you do not want the PR to be reviewed yet.
  • [ ] Add the "ready to merge" label if the basics are fulfilled and no further pushes are planned by you.

lawli3t avatar May 06 '22 23:05 lawli3t

Great work! Please request review once it is ready!

markus2330 avatar May 12 '22 14:05 markus2330

This should now be able to build my example main.c with CMake and Corrosion in the debian sid docker container. The integration seems very smooth now after a bumpy road at the start. When running the build there should be a binary rust-libc-test available which uses my C-Implementation. If you run it you see some debug output from the function calls that checks the functionality. It's obtuse but I think you get the gist, it's more of an elongated Hello World for testing purposes.

I am working hard on the integration with the tests (linking my resulting library), as we spoke yesterday, there are some minor issues that should be easily fixable (struct names, old vs new names in general,..) that I hope I can iron this out in the next few days. One issue with the translation layer are the VAArgs since cbindgen handles them very badly. I might need to manually intervene for that one but I'll see what the next few days will bring. You wanted to see the simple integration, which is why I tagged you here.

You can find the header file that gets generated by cbindgen under src/libs/rust/libc/elektra_rs.h. This is completely auto-generated from my Rust library and should be automated and only used for validation purposes, since in the end it should be the exact same as the headers for the C variant. I need to automate the generation though, since I currently do it by hand and then just copy the resulting header file into the example... I have included it for now because I thought it might be interesting for you to take a look at and because it is not automatically generated on build for now. The generated file should in the end look exactly like the header file for the C variant and be dynamically created during the build...

Also I had some issues with the csvstorage plugin, it threw some compiler errors even though I didn't touch this file? I will have to look into this as well but I ignored it for now I just wanted my build to complete. I am aware of it. Maybe it's actually a remnant from my attempts at fixing the csvstorage plugin. Just added this part so you're not confused about why this change is included.

Can you maybe try running the build on your machine to see if you can reproduce it? It should work in the docker container, which I ran with the same commands as here

@markus2330

lawli3t avatar Aug 16 '22 15:08 lawli3t

@kodebach Might be interesting for you to look at my generated Headers as well? Some obvious issues like the struct names are there, but I think you can get a good idea of how I implemented it. I will have to take a look at you PR as well to check if our generated header files match. I think there are also some inaccuracies on my part, especially with regards to const pointers, but that should not be too dramatic to fix...

lawli3t avatar Aug 16 '22 15:08 lawli3t

headers should now be managed by CMake as well, although they still need to be manually generated with cbindgen via

# Working directory src/libs/rust/libc
cbindgen --config cbindgen.toml --crate elektra_rs_libc --output resources/headers/elektra_rs.h

Even the newest version currently doesn't have automatic cbindgen integration sadly, and we wouldn't be able to use it anyway since it would require CMake >= 3.19, so I will need to find a way to generate them before the CMake build

lawli3t avatar Aug 16 '22 19:08 lawli3t

You can find the header file that gets generated by cbindgen under src/libs/rust/libc/elektra_rs.h. This is completely auto-generated from my Rust library and should be automated and only used for validation purposes, since in the end it should be the exact same as the headers for the C variant. I need to automate the generation though, since I currently do it by hand and then just copy the resulting header file into the example... I have included it for now because I thought it might be interesting for you to take a look at and because it is not automatically generated on build for now. The generated file should in the end look exactly like the header file for the C variant and be dynamically created during the build...

Yes, please avoid the duplication (Either create it or use our main header file, if possible).

Also I had some issues with the csvstorage plugin, it threw some compiler errors even though I didn't touch this file? I will have to look into this as well but I ignored it for now I just wanted my build to complete. I am aware of it. Maybe it's actually a remnant from my attempts at fixing the csvstorage plugin. Just added this part so you're not confused about why this change is included.

If there is a compilation error with some more recent llvm or similar, please report it in a separate issue.

Can you maybe try running the build on your machine to see if you can reproduce it? It should work in the docker container, which I ran with the same commands as here

Unfortunately, I got following error:

Step 5/46 : RUN apt-get update && apt-get -y install         antlr4         automake         autotools-dev         bison         build-essential         checkinstall         clang-13         clang-format-13         cmake         curl         dh-lua         ed         flex         gawk         git         gnupg2         gobject-introspection         icheck         lcov         libantlr4-runtime-dev         libaugeas-dev         libc6-dbg         libdbus-1-dev         libev-dev         libgcrypt20-dev         libgirepository1.0-dev         libgit2-dev         libglib2.0-dev         libgpgme-dev         liblua5.3-dev         libpcre++-dev         libpcre3-dev         libpython3-dev         libssl-dev         libsystemd-dev         libuv1-dev         libxerces-c-dev         libyajl-dev         libyaml-cpp-dev         libzmq3-dev         llvm-13         moreutils         ninja-build         npm         openjdk-11-jdk         pkg-config         python3-dev         python3-pip         ruby         ruby-dev         sloccount         swig4.0         systemd         tclcl-dev         unzip         valgrind         wget     && rm -rf /var/lib/apt/lists/*
 ---> Running in 55a02f146aa3
Get:1 http://deb.debian.org/debian sid InRelease [192 kB]
Err:1 http://deb.debian.org/debian sid InRelease
  The following signatures couldn't be verified because the public key is not available: NO_PUBKEY 648ACFD622F3D138 NO_PUBKEY 0E98404D386FA1D9
Reading package lists...
W: GPG error: http://deb.debian.org/debian sid InRelease: The following signatures couldn't be verified because the public key is not available: NO_PUBKEY 648ACFD622F3D138 NO_PUBKEY 0E98404D386FA1D9
E: The repository 'http://deb.debian.org/debian sid InRelease' is not signed

It is probably unrelated to your changes but I didn't have time to investigate.

markus2330 avatar Aug 17 '22 07:08 markus2330

I mark this stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping by writing a message here or create a new PR with the remainder of this PR. Thank you for your contributions :sparkling_heart:

github-actions[bot] avatar Aug 24 '23 01:08 github-actions[bot]

I closed this now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new PR with the remainder of this PR. Thank you for your contributions :sparkling_heart:

github-actions[bot] avatar Sep 07 '23 01:09 github-actions[bot]