felfert commented 1 year ago

Description

As announced in #22 , I implemented a uhid driver service together with an alternative cli that acts as a drop-in replacement for cherryrgb_cli. See docs/UHID-driver.md.

Fixes # 22

Type of change

Please delete options that are not relevant.

[x] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Adds/updates Documentation

Code Checklist

[x] I have performed a self-review of my own code
[x] I have made corresponding changes to the documentation
[x] My changes generate no new warnings
[ ] I have implemented respective test(s)
[x] I have run lints and tests (cargo fmt && cargo clippy && cargo test) that prove my fix is effective or that my feature works

felfert commented 1 year ago

Strange, that all except fmt failed. builds fine here?!

felfert commented 1 year ago

Looks like the build system is missing llvm-config and libclang:

2023-04-26T20:10:00.8926365Z warning: could not execute `llvm-config` one or more times, if the LLVM_CONFIG_PATH environment variable is set to a full path to valid `llvm-config` executable it will be used to try to find an instance of `libclang` on your system: "couldn't execute `llvm-config --prefix` (path=llvm-config) (error: No such file or directory (os error 2))"
2023-04-26T20:10:00.8928739Z 
2023-04-26T20:10:00.8929255Z error: failed to run custom build command for `clang-sys v1.6.1`
2023-04-26T20:10:00.8929867Z 
2023-04-26T20:10:00.8930107Z Caused by:
2023-04-26T20:10:00.8931102Z   process didn't exit successfully: `/home/runner/work/cherryrgb-rs/cherryrgb-rs/target/debug/build/clang-sys-dad454f8793fe135/build-script-build` (exit status: 101)
2023-04-26T20:10:00.8931743Z   --- stdout
2023-04-26T20:10:00.8933133Z   cargo:warning=could not execute `llvm-config` one or more times, if the LLVM_CONFIG_PATH environment variable is set to a full path to valid `llvm-config` executable it will be used to try to find an instance of `libclang` on your system: "couldn't execute `llvm-config --prefix` (path=llvm-config) (error: No such file or directory (os error 2))"
2023-04-26T20:10:00.8933853Z 
2023-04-26T20:10:00.8934329Z   --- stderr
2023-04-26T20:10:00.8951960Z   thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: "couldn't find any valid shared libraries matching: ['libclang.so', 'libclang-*.so'], set the `LIBCLANG_PATH` environment variable to a path where one of these files can be found (invalid: [])"', /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/clang-sys-1.6.1/build/dynamic.rs:206:45
2023-04-26T20:10:00.8954113Z   note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
2023-04-26T20:10:00.8954661Z warning: build failed, waiting for other jobs to finish...
2023-04-26T20:10:01.1370052Z ##[error]Process completed with exit code 101.
2023-04-26T20:10:01.1466111Z Post job cleanup.

This new dependency is introduced indirectly by uhid-virt:

   └── uhid-virt v0.0.6
│       ├── enumflags2 v0.7.7
│       │   └── enumflags2_derive v0.7.7 (proc-macro)
│       │       ├── proc-macro2 v1.0.56 (*)
│       │       ├── quote v1.0.26 (*)
│       │       └── syn v2.0.15 (*)
│       ├── libc v0.2.142
│       └── uhidrs-sys v1.0.2
│           [build-dependencies]
│           └── bindgen v0.63.0
│               ├── bitflags v1.3.2
│               ├── cexpr v0.6.0
│               │   └── nom v7.1.3
│               │       ├── memchr v2.5.0
│               │       └── minimal-lexical v0.2.1
│               ├── clang-sys v1.6.1

felfert commented 1 year ago

Forget that:

~~Have a look here:~~

The step named "Install dependencies" apparently does the trick

clippy:
    name: Clippy
    runs-on: ubuntu-latest
    steps:
      - name: Install dependencies
        run: sudo apt-get update && sudo apt-get install -y libclang-dev clang llvm
      - uses: actions/checkout@v3
      - uses: dtolnay/rust-toolchain@stable
        with:
          components: clippy
      - name: Cargo Clippy
        run: cargo clippy -- -D warnings
      - name: Cargo Clippy Example
        run: cargo clippy --manifest-path example/Cargo.toml -- -D warnings

skraus-dev commented 1 year ago

Thanks alot for doing this major task!

Now, to get this merged, there needs to be some conditional compiling happening - so non-linux user can still build the project.

Checklist:

[x] Conditionally compile uhid-exclusive functionality in the main lib related to target_os (target_os: linux, reference: https://doc.rust-lang.org/reference/conditional-compilation.html#the-cfg-attribute)
- [x] module: vkbd
- [x] structs: models.RpcAnimation
[x] Conditionally include the dependency uhid-virt, only on the linux-platform
[ ] CI: Ensure build dependencies get installed (natively for x86_64, inside cross container for i386, arm)
[ ] CI: Ensure correct build command is used per platform (build --all for linux, plain build for the others)
[ ] CI: Ensure correct binaries and auxilary files (udev rules, service file, config file) are bundled in build artifact / release bundle

With these changes it should already work from what I can tell.

Linux: cargo build --all Non-Linux: cargo build (it should not be attempting to build everything, aka. only builds the CLI)

felfert commented 1 year ago

Thanks alot for doing this major task!

Now, to get this merged, there needs to be some conditional compiling happening - so non-linux user can still build the project.

Checklist:
* [x]  Conditionally compile uhid-exclusive functionality in the main lib related to target_os (target_os: linux, reference: https://doc.rust-lang.org/reference/conditional-compilation.html#the-cfg-attribute)

  * [x]  module: vkbd
  * [x]  structs: models.RpcAnimation

* [x]  Conditionally include the dependency `uhid-virt`, only on the linux-platform
Wahh, you are too quick, I have the above already - just not yet committed to this PR's branch
* [ ]  CI: Ensure build dependencies get installed (natively for x86_64, inside cross container for i386, arm)

I tried github's official rust action instead and it works nicely. (not yet tested for i36 and arm)

* [ ]  CI: Ensure correct build command is used per platform (`build --all` for linux, plain `build` for the others)

That's what I am struggling with right now: I cannot believe, that there is NO way of disabling build of specific workspace members based on target-os or features. What a pity! (There are some Issues open in cargo's repo, but no solution exists yet. I experimented with conditionals in both service and ncli, but this is ridiculously ugly. This are 2 very weak spots in cargo and rustc - to say the least.

* [ ]  CI: Ensure correct binaries and auxilary files (udev rules, service file, config file) are bundled in build artifact / release bundle

Huh: Did you mean CD?

With these changes it should already work from what I can tell.

Linux: cargo build --all Non-Linux: cargo build (it should not be attempting to build everything, aka. only builds the CLI)

felfert commented 1 year ago

I think, triggering the CD action on any pull request is NOT a good idea, because - as far as i understand it - the publishing to releases and cargo runs in YOUR context (using the secrets you provide in your configuration). Instead, the matrix-build and the copying to an output directory should be moved to the CI action and the resulting output directory then stashed for later retrieval by the CD action. Then the CD action, just retrieves the previouslly stashed artifacts and publishes them.

felfert commented 1 year ago

* [ ]  CI: Ensure build dependencies get installed (natively for x86_64, inside cross container for i386, arm)
llvm and clang are capable of cross-compiling per se, so no problem there, but libclang-dev (and possible dependencies) is going to be tricky, because there are no pre-made packages:

a complete cross env would have to be created

libclang-dev installed inside of that one

This is a) much bloat, b) quite time-consuming if done in the regular CI action. I will try to generate a separate action that perfoms this and then stashes the files of the lib for retrieve by the CI action. This might take some time.

skraus-dev commented 1 year ago

@felfert no worries, the build dependency task is a tiny one, involving the cross rust toolchain

https://github.com/cross-rs/cross/wiki/Configuration

We just need to decide whether we want a seperate Cross.toml or we put the pre-build dependency installation step inside the existing Cargo.toml

I think, triggering the CD action on any pull request is NOT a good idea, because - as far as i understand it - the publishing to releases and cargo runs in YOUR context (using the secrets you provide in your configuration). Instead, the matrix-build and the copying to an output directory should be moved to the CI action and the resulting output directory then stashed for later retrieval by the CD action. Then the CD action, just retrieves the previouslly stashed artifacts and publishes them.

Kind of like that, yes. My plan is to have the main build action.. and then I will introduce a seperate actions that uses it as "re-usable workflow" that will only run on "main" and "tags".

reference: https://docs.github.com/en/actions/using-workflows/reusing-workflows

felfert commented 1 year ago

Oh, and can we please keep CI/CD related commits out of this PR. I think, these are better collected in a separate PR. I'm closing this, and re-submit a new PR without those.

For now, this is what I have No PR yet (still without the cross stuff, but artifacts are already stashed): https://github.com/felfert/cherryrgb-rs/actions/runs/4830348450

Notes:

Intentionally NOT named CI, so we can have this in addition to the existing one. If everything is finalized, we can replace the original CI with the new one.
Intentionally only manually triggered, so it does not waste resources all the time.

Question: Why do you use dtolnay/rust-toolchain@stable instead of the default that comes preinstalled on githubs native platforms?

felfert commented 1 year ago

Ok, after wasting a whole evening for testing research etc, it turns out, that libclang on ubuntu is seriously broken in regards of multiarch installs. (cross uses ubuntu-16.04 (ancient!) as base-image). Unfortunately, cross also is not very userfriendly in that it does not show any output of the pre-build comand in Cross.toml. So I had to resort to manually entering the container locally and figuring out what went wrong:

Basically, the problem is similar like this: https://askubuntu.com/questions/1446189/problem-doing-multiarch-installs-on-ubuntu The packages of libclang for different architectures all install to the same location. The native amd64 lib is at /usr/lib/llvm-3.8/lib/libclang-3.8.so.1 and the arm64 and i686 packages try to overwrite this - which is obviously fatal:

Snippet from the apt-get install libclang-dev:i386 Unpacking libclang1-3.8:i386 (1:3.8-2ubuntu4) ... dpkg: error processing archive /var/cache/apt/archives/libclang1-3.8_1%3a3.8-2ubuntu4_i386.deb (--unpack): trying to overwrite shared '/usr/lib/llvm-3.8/lib/libclang-3.8.so.1', which is different from other instances of package libclang1-3.8:i386

Same happens with arm64

The correct path would be somewhere below /usr/lib/i386-linux-gnu/ resp. /usr/lib/arm64-linux-gnu/

So unless you know of a different container for cross-builds, I give up on that. It's simply not worth the effort, especially since both architectures are not really widely used.

Well one last try would be manually unpacking the .deb in temporary dir and then moving it to a different location. Given that ubuntu 16.04 is ancient, I dont even think about bug report.

felfert commented 1 year ago

So for now, I have added a featue named uhid and changed the conditional compile directives. In order to build, both target_os must be "linux" AND feature must be "uhid" So in the matrix CI action we use --all --features uhid on native ubuntu latest only

If the libclang mess gets sorted out for the cross platforms, we can add these flags for them later.

The new ~~CI is ready~~, (see here). Currently working on new CD. PR for both coming soon...

Cheers -Fritz

felfert commented 1 year ago

Turns out, artifacts can only be shared in the same Action. So switching to cache. Keeping the artifact for manual checks, if eferything is in order.

felfert commented 1 year ago

Turns out, artifacts can only be shared in the same Action. So switching to cache. Keeping the artifact for manual checks, if eferything is in order.

Also, the github deployment action insists being triggered by a git tag event which makes my idea of how this should work completely useless. I must admit, I never had used github workflow/actions before. Professionally, I am used to a jenkins environment which is quite different in that perspective and I was a probably a bit naive in assuming that the github stuff works in a similar way.

So: PR coming up with fixes for the existing workflows ...

skraus-dev / cherryrgb-rs

Add uhid service and alternative cli #35

Description

Type of change

Code Checklist