Details

Reviewers

tomek
varun
• max
inka
ashoat
• jon

Commits

rCOMM0b485545330b: [services] Backup - Add Rust

Summary

Depends on D4860

Adding basic rust to the backup service.

Test Plan

Backup service builds.

To use rust functions in the c++ code:

#include "rust_lib/src/lib.rs.h"

test_function();

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• karol created this revision.Aug 17 2022, 3:54 AM

Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptAug 17 2022, 3:54 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 17 2022, 3:55 AM

Harbormaster failed remote builds in B11415: Diff 15704!

• karol edited the summary of this revision. (Show Details)Aug 17 2022, 4:11 AM

• karol edited the test plan for this revision. (Show Details)

• karol added reviewers: tomek, varun, • max, inka.

• karol added a parent revision: D4860: [services] Tunnelbroker - Move install corrosion script to lib.

fix cmake indents

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 17 2022, 4:14 AM

Harbormaster failed remote builds in B11416: Diff 15705!

fix cmake

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 17 2022, 4:19 AM

Harbormaster failed remote builds in B11417: Diff 15706!

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.

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.

This revision now requires changes to proceed.Aug 17 2022, 5:13 AM

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?

• karol added a child revision: D4867: [services] Backup - Rust lib - Add initialize.Aug 18 2022, 6:07 AM

• karol mentioned this in D4867: [services] Backup - Rust lib - Add initialize.Aug 18 2022, 6:11 AM

• karol mentioned this in D4869: [services] Backup - Rust lib - Add process.

• karol mentioned this in D4870: [services] Backup - Rust lib - Add terminate.

In D4861#140658, @karol wrote:

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.

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...

• max requested changes to this revision.Aug 18 2022, 7:43 AM

• max added inline comments.

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. We can just add it to the Dockerfile.

This revision now requires changes to proceed.Aug 18 2022, 7:43 AM

In D4861#140927, @karol wrote:

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?

Thanks, @karol ! I've added a few small notes.

• karol added a child revision: D4884: [services] Backup - Rust lib - Add is initialized.Aug 19 2022, 1:32 AM

• karol removed a child revision: D4867: [services] Backup - Rust lib - Add initialize.

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.

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.

address feedback + cmake workaround

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

Harbormaster failed remote builds in B11488: Diff 15800!Aug 22 2022, 2:37 AM

• karol planned changes to this revision.Aug 22 2022, 4:01 AM

Harbormaster completed remote builds in B11490: Diff 15802.Aug 22 2022, 4:09 AM

ignored the .cmake file since we're going with max's

tomek accepted this revision.Aug 24 2022, 3:23 AM

tomek added inline comments.

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?

@max you've been blocking this for almost a week now, could you please do a review?

In D4861#142965, @karol wrote:

@max you've been blocking this for almost a week now, could you please do a review?

This diff is blocked on D4805 to use the shared cmake file for cxx+corrosion for android and services.
As a workaround, you can use this one in this diff, but please create a follow-up task to remove it and use the shared one after D4805 will be landed.

services/backup/rust_lib/Cargo.toml
16 ↗	(On Diff #15802)	We must move `debug = true` to the `[profile.dev]` section as @tomek mentioned.

This revision is now accepted and ready to land.Aug 24 2022, 4:53 AM

Sorry @karol but I would prefer to wait until D4805 lands (hopefully tomorrow), then remove all of the CMake from here before landing. Given past experience I suspect we will end up forking things... there will be some Backlog task to merge the CMakes and it will keep getting punted...

This revision now requires changes to proceed.Aug 24 2022, 9:05 AM

• jon added inline comments.Aug 24 2022, 10:29 AM

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+

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.

In D4861#143177, @ashoat wrote:

Sorry @karol but I would prefer to wait until D4805 lands (hopefully tomorrow), then remove all of the CMake from here before landing. Given past experience I suspect we will end up forking things... there will be some Backlog task to merge the CMakes and it will keep getting punted...

D4805 is landed now.