-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Better documentation for Rust #20088
Comments
(Picking bits of this between tasks; thanks for tagging me, this needs comprehensive addressing, but bits for now)
|
That is OK. Thing is, due to RIOT's prevalence of macros we need to run both binden to and c2rust to translate header files, and they each define their own types for structs, and their own functions, and they differ b/c the types differ in identity (but not in layout). You're only seeing these warnings now b/c cargo won't show you warnigns originating from crates you take from crates.io or git, whereas it will show you warnings from crates you have checked out (expecting that given they're local, you may be in a position to fix them). |
@chrysn thanks for the fast reply, this is valuable information. I will try out your suggestions and write down a log, perhaps that will help with building the documentation later. About editing and editor/IDE support: seems like rust-analyzer supports lots of editors. Like I mentioned, I use rust-analyzer in vscode, but it fails to discover the C bindings. If rust-analyzer works for you in vim, then I suspect there is a configuration issue. If you use a custom configuration, could you share it please? |
In vim I can specify a custom invocation as let g:LanguageClient_serverCommands = {
\ 'rust': ['~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/rust-analyzer'],
\ } (which is not really what fixes things, for that I'd need to export the per-project environment variables that could be copied out from |
Indeed, rust-analyzer is failing when building According to the rust-analyzer documentation, it calls |
I modified {
"rust-analyzer.cargo.extraEnv": {
"CC": "",
"CFLAGS": "",
"CPPFLAGS": "",
"CXXFLAGS" : "",
"RIOT_COMPILE_COMMANDS_JSON" : "/home/[email protected]/proj/lara_test_rust/bin/native/cargo-compile-commands.json",
"RIOT_USEMODULE" : "at auto_init auto_init_ztimer board board_common_init core core_idle_thread core_init core_lib core_msg core_panic core_thread cpu fmt frac isrpipe isrpipe_read_timeout libc native_drivers periph periph_common periph_gpio periph_gpio_linux periph_init periph_init_gpio periph_init_gpio_linux periph_init_led0 periph_init_led1 periph_init_led2 periph_init_led3 periph_init_led4 periph_init_led5 periph_init_led6 periph_init_led7 periph_init_leds periph_init_pm periph_init_timer periph_init_uart periph_pm periph_timer periph_uart preprocessor preprocessor_successor stdio_native sys tsrb ztimer ztimer_convert ztimer_convert_frac ztimer_convert_shift ztimer_core ztimer_extend ztimer_init ztimer_msec ztimer_periph_timer ztimer_usec"
},
"rust-analyzer.check.overrideCommand": [
"cargo", "check", "--target", "i686-unknown-linux-gnu", "--message-format=json"],
"rust-analyzer.cargo.buildScripts.overrideCommand": [
"cargo", "check", "--target", "i686-unknown-linux-gnu", "--message-format=json"
]
} So this confirms the missing env variables where the problem. But this is no clean solution. Sadly, rust-analyzer does not support a unified configuration file across editors, although this feature seems to be planned. I think the solution would be a wrapper around |
What scares me a bit about configuring editors to run some wrapper are the security implications. Once your IDE is configured to run some wrapper for cargo-check (or some command that generates environment variables), all of a sudden opening a file in the editor becomes a vector for code execution. Sure, that's also the case when running I'd like to better understand how this is used in IDEs -- are these per-project settings.json that are configured, or is this a global file? What are the general security expectations when users open projects in an IDE? |
I understand. For the record,
The only risk I see (please correct me if I'm wrong) is someone modifying Vscode opens any new workspace in restricted mode -- that is, it should disable anything that could execute code, e.g. build scripts, extension features. The user must always actively mark the workspace as trusted. That been said, I personally use vscode exclusively for convenience. I am willing to try others in the hope that I will find something else that works and isn't Microsoft. So I am really in favor of not assuming any editor, and finding a solution that safely and securely works for everyone. |
Right -- so given that, anything that calls rust-analyzer needs to sandbox it, and that also goes for any wrapper to rust-analyzer. I think a good path forward would be to provide some make targets (make cargo-check, make cargo-update, make rustanalyzer) along with a more generic target that just prints what you need to prefix to your custom calls. As for vscode ... I don't care what people use, but I do care that if that environment's behavior is sensible, it should work with our code :-) |
So, how do we get from where we are now to easier use? I've looked at the settings.json you've shown, and As a first step, in #20319, I'm adding output to |
I looks good, my only suggestion would be that the contents of |
Yeah, then I'll just go that route. Next up:
|
I think we're mixing up stuff here. Sorry for being verbose, I just want to make sure we are on the same page, and I didn't understand something wrong. Speaking of |
Thanks for elaborating, I was unaware of the scope of compile-commands.json and of what goes into rust-analyzer's startup. #20319 is now up to date, and contains a documentation section on IDE/editor setup. Can you check whether that works for you, maybe as a review there? |
So, |
#20319 updated riot-sys and -wrappers to the latest git version; did your application follow that? |
Oh, my bad! |
I've taken the liberty of editing your original post into a checklist, and ticking off "needs proper integration with IDEs" (while leaving "with at least one editor" and "vscode" unchecked because they don't work out of the box yet). Any priorities what to tackle next? My suggestion is on the riot-headers.h -- your option ii is what I think is the right way, and the only reason the warnings show is that once you're working from a checkout you get to see warnings that were previously not shown (compiler goes "Oh you got it locally, great, so now that your attention is at it, you may want to know that things are not ideal there"). Reducing the number of warnings is certainly an important task on its own, but it may suffice to explain them (and at any rate, a PR adding a header file can be merged within minutes, no discussion there if it builds). Do you agree with that resolution? |
Yes, I also think |
Regarding an IDE independent way to pass env variables to rust-analyzer one could use the |
I just tried this out. Put the following in [env]
RIOTBUILD_CONFIG_HEADER_C = "<Project>/bin/native/riotbuild/riotbuild.h"
RIOT_COMPILE_COMMANDS_JSON = "<Project>/bin/native/cargo-compile-commands.json"
[build]
target = "i686-unknown-linux-gnu" # I test this on native
message-format = "json" And it worked like a charm for both VSCode and neovim, with zero extra project-specific configuration. This is great! |
I was trying things out once again, and there is a case where rust-analyzer still fails, and that is on external rust module creation. But this depends on where the module is located.
This behavior is independent of the editor, I tried it out with both neovim and vscode. The program compiles otherwise just fine. Sym-linking the module in the application directory doesn't work either. Another observation is that, in case 1. (module located in the app directory), the path to riot-sys has to be overriden in the [patch.crates-io]
# we modify the riot-sys crate to include the headers we need
riot-sys = { path = "../../modules/rust-riot-sys" } has to be present int both app's and module's This, and the fact that rust-analyzer starts a new scan when jumping from an app source-file to a module's makes me suspect that rust analyzer tries to analyze the module as a standalone-thing, but that is not possible because all the bindings are in the app's UPDATE: |
Description
Some background
Currently, getting anything more complex than
examples/rust-hello-world
running is undocumented, and really hard without advanced knowledge of how the RIOT build-system works.I personally have only limited experience with Rust, Cargo and RIOT. In the following, I will list some difficulties I encountered.
Areas that require documentation
Creating a RIOT Rust module or RIOT wrapper
riot-wrappers
crateSetting up out-of-tree builds
Using RIOT C-modules that are neither in
riot-wrappers
nor inriot-sys
riot-headers.h
.riot-headers.h
be extended?.cargo
registry works, but this is no clean solutionriot-wrappers
andriot-sys
from git, modifying them, and then pointing to them inCargo.toml
kinda works. Even if I check-out to the exact version stated in the.cargo
registry, I get some warnings (e.g.gnrc_netif_dev_is_6lo redeclared with a different signature
) -- this solution cannot be right eitherMismatched riot-sys exports:
riot_sys::mutex_lock()
andriot_sys::inline::mutex_lock()
work withriot_sys::inline::mutex_t
and will not compile when supplied withriot_sys::mutex
. But usingriot_sys::inline::mutex_t
will then not work with e.g.riot_sys::ztimer_mutex_lock()
. What is the workaround in this case?Getting some sweet language support
.cargo/config.toml
see discussion belowmy experience with vscode: the C bindings generated byriot-sys
in./target
are not seen by rust-analyzer. This perhaps boils down to the fact that this is no pure cargo build.would be nice to have a proper integration in at least one code editor- seems that rust-analyzer can be configured independentlyThe text was updated successfully, but these errors were encountered: