Depends on D4860
Adding basic rust to the backup service.
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Weird that some cmake rules don't allow upper case letters for variable names (which is pretty standard I'd say), the regex is: [a-z][a-z0-9_]+
@atul should we fix this?
I'm publishing this because I don't think this is an error.
Comment Actions
Please take a look at D4805, the diff is to add corrosion_cxx.cmake with the modifications for our needs instead of just copy pasting the rusty_cmake cmake file:
- Windows-related code was removed.
- Automatically detect Rust target architecture using detect_cargo_target_architecture function.
- Unnecessary comments were removed.
The main problem that will appear is: that when using the original cmake file from rusty you should find somehow the target architecture and pass it to it.
Comment Actions
Ok, once your diff is landed, I'm going to rebase to it and take your version removing mine, no problem. How about the rest of this diff?
Comment Actions
I'm not sure if there is a good reason for this (CMake can have strange requirements...) or maybe it is just a "style guide" thing for CMake.
That said, does the variable type really matter? It doesn't seem like a great use of people's time to figure out a way to revise this...
I think @jon is more likely to know the answer here than @atul, but my general suggestion is just to use lowercase variable names to appease the CI...
| services/backup/Dockerfile | ||
|---|---|---|
| 18 ↗ | (On Diff #15706) | Why do we export it here? Seems that the corrosion install works without it. |
| services/lib/docker/install_rust.sh | ||
| 5 ↗ | (On Diff #15706) | I don't think we should create an additional bash script for a one-line installation. |
Comment Actions
That said, does the variable type really matter? It doesn't seem like a great use of people's time to figure out a way to revise this...
It's about names, not types. I think we should resolve this. The same checks seem to run on the CMakelist.txts and *.cmake files but still with the corrosion.cmake we get errors and with the rest of the files we do not even though there seem to be very similar definitions in both files.
- thie is error set(ONE_VALUE_KEYWORDS PATH NAMESPACE CXX_BRIDGE_SOURCE_FILE)
- this is not set(BUILD_TESTING OFF CACHE BOOL "Turn off tests" FORCE)
So I perceive this as an invalid CI action (false positive).
I think @jon is more likely to know the answer here than @atul, but my general suggestion is just to use lowercase variable names to appease the CI...
That doesn't solve anything. I think we should either turn off this rule or get it fixed.
Comment Actions
Sorry, I meant variable name not type. If the variable name is your decision then I think we can just follow CMake Lint's requirements. If the variable name is decided by somebody else, then we can disable the rule like we do here.
Curious for @jon's perspective on this one too.
Comment Actions
Sorry, but it seems to me like you didn't really address what I said:
The same checks seem to run on the CMakelist.txts and *.cmake files but still with the corrosion.cmake we get errors and with the rest of the files we do not even though there seem to be very similar definitions in both files.
- this is error set(ONE_VALUE_KEYWORDS PATH NAMESPACE CXX_BRIDGE_SOURCE_FILE)
- this is not set(BUILD_TESTING OFF CACHE BOOL "Turn off tests" FORCE)
So I perceive this as an invalid CI action (false positive).
How about this?
I do not think that CI should behave like this, it's not logical to me, and would be cool to find out what exactly is going on here.
I will disable the rule but I think it's more like a dirty workaround than a reasonable solution.
| services/backup/Dockerfile | ||
|---|---|---|
| 18 ↗ | (On Diff #15706) | You're right, it seems to be working, thanks! |
| services/lib/docker/install_rust.sh | ||
| 5 ↗ | (On Diff #15706) | Ok, I Don't mind |
| services/backup/rust_lib/Cargo.toml | ||
|---|---|---|
| 15–16 ↗ | (On Diff #15802) | Why do we have debug=true for release? Shouldn't this be set just for dev? |
Comment Actions
| services/backup/rust_lib/build.rs | ||
|---|---|---|
| 2 ↗ | (On Diff #15802) | we will likely need the following flag here as well for use with Clang. Currently this only works with GCC9+ |
Comment Actions
I think @jon is more likely to know the answer here than @atul, but my general suggestion is just to use lowercase variable names to appease the CI...
There's no rhyme or reason with CMake naming conventions. Every ecosystem seems to have created their own convention over CMake's history. It's just that cmake-lint establishes some conventions which make it unambiguous about variable usage.
Why it errors in some usages, but not other usages; I have no idea.
Comment Actions
Why it errors in some usages, but not other usages; I have no idea.
I think that's the real problem.
At this point, nearly every workflow requires additional steps. Maybe we can think about a way to make this seem better in the future -- users should know before reading the footnote that Nix isn't going to do everything for them. But, it will do most of the work for them, and anything else will be smaller.
While the user might still know this, the footnote isn't really helping right now. It makes it seem like we told them Nix will do a lot of things, and then later slid in that they will have to do more things.
Currently, the footnote for these sections has a lot of examples for "additional steps." Plus, "additional steps" is broad and could mean any amount of steps, especially to a user who has no familiarity with the dev environment.
So we should think about that, since most of the time footnotes should be used sparingly. If they end up affecting a large portion of a section, we should just make it information the user knows outside of just the footnote. But this can be a separate issue.