Page MenuHomePhabricator
Differential D5432

[services] Tunnelbroker - Creating `main.rs` entry point file for Rust
ClosedPublic
Actions

Authored by • max on Oct 20 2022, 7:38 AM.
Tags
None
Referenced Files
F6272969: D5432.diff
Fri, Apr 25, 4:08 PM
Unknown Object (File)
Thu, Apr 24, 4:01 PM
Unknown Object (File)
Wed, Apr 23, 3:09 AM
Unknown Object (File)
Wed, Apr 9, 3:32 AM
Unknown Object (File)
Wed, Apr 9, 3:32 AM
Unknown Object (File)
Wed, Apr 9, 3:32 AM
Unknown Object (File)
Wed, Apr 9, 3:32 AM
Unknown Object (File)
Wed, Apr 9, 3:32 AM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

Details

Reviewers
• jon
varun
marcin
tomek
Commits
rCOMM7c253be3d099: [services] Tunnelbroker - Creating `main.rs` entry point file for Rust
Summary

This diff is a part of the flat diffs stack to transform the current Tunnelbroker project into a Rust app by calling the current C++ codebase from Rust. The related transition task is ENG-1072.

This diff creates a Rust application entry point file main.rs and calls the initialization function (D5429) from the current C++ codebase.

Related Linear task: ENG-2092

CI Notice:
The CI will fail on this and further diffs until the D5461 where the changes to CI build commands are made.
This stack will be landed all in one to prevent CI from failing on diffs out of this stack.

Test Plan

1. CI Testing:
These diff changes are included as a part of the stack in the later D5461 diff, where the changes to CI build commands are made. The CI succeded on the D5461 which reflects that this changes are passing the build.

2. Manually testing:
These diff changes can be tested in the middle of the stack at D5436. To test it patch to D5436 using arc patch D5436, run nix develop. Go to tunnelbroker service directory services/tunnelbroker and run the Cargo building process cargo build or cargo run.
The Rust application will be successfully built.

Diff Detail

Repository
rCOMM Comm
Branch
add-main-rs
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• max created this revision.Oct 20 2022, 7:38 AM
• max held this revision as a draft.
Herald added a reviewer: • jon. · View Herald TranscriptOct 20 2022, 7:38 AM
Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript
• max added a parent revision: D5431: [services] Tunnelbroker - Removing of deprecated C++ files.Oct 20 2022, 7:38 AM
• max added a child revision: D5434: [services] Tunnelbroker - Creating `build.rs` file for building and linking C++ dependencies.
Harbormaster failed remote builds in B12925: Diff 17716!Oct 20 2022, 7:43 AM
• max mentioned this in D5429: [services] Tunnelbroker - Initialization function exposed to Rust.Oct 20 2022, 8:00 AM
• max edited the summary of this revision. (Show Details)Oct 20 2022, 8:09 AM
• max edited the test plan for this revision. (Show Details)
• max added reviewers: varun, marcin. • max added 1 blocking reviewer(s): • jon.
• max published this revision for review.Oct 20 2022, 8:22 AM
• jon requested changes to this revision.Oct 21 2022, 9:26 AM
• jon added inline comments.
services/tunnelbroker/src/main.rs
4 ↗(On Diff #17716)

I guess this is highly unrecommended: https://doc.rust-lang.org/std/macro.include.html

Leads to potential footguns around what is "defined" through it

This revision now requires changes to proceed.Oct 21 2022, 9:26 AM
• max requested review of this revision.Oct 21 2022, 10:19 AM
• max marked an inline comment as done.
• max added inline comments.
services/tunnelbroker/src/main.rs
4 ↗(On Diff #17716)

I guess this is highly unrecommended: https://doc.rust-lang.org/std/macro.include.html

Leads to potential footguns around what is "defined" through it

We should include the C++ codebase exposed functions to the Rust app.
In a regular Rust app we shouldn't do this, but in our scenario, seems that this is the only way to include headers according to the cxx documentation.

• max edited parent revisions, added: D5430: [services] Tunnelbroker - Moving CMakeLists file to use for gTest only; removed: D5431: [services] Tunnelbroker - Removing of deprecated C++ files.Oct 24 2022, 3:07 AM
• max updated this revision to Diff 17841.Oct 24 2022, 3:22 AM
• max marked an inline comment as done.

Update on a stack reordering.

Harbormaster failed remote builds in B12983: Diff 17841!Oct 24 2022, 3:22 AM
• max updated this revision to Diff 17864.Oct 24 2022, 9:27 AM

Update on the stack changes.

Harbormaster failed remote builds in B13005: Diff 17864!Oct 24 2022, 9:28 AM
• max edited the summary of this revision. (Show Details)Oct 24 2022, 9:28 AM
• max edited the test plan for this revision. (Show Details)
• max added 1 blocking reviewer(s): • jon.
• jon accepted this revision.Oct 26 2022, 1:51 PM

don't feel like blocking this work, since we have the goal of transitioning away from C++ eventually. So the issue of include! hygiene might not be relevant in the future.

This revision is now accepted and ready to land.Oct 26 2022, 1:51 PM
• max added a reviewer: tomek.Oct 27 2022, 2:32 AM
This revision now requires review to proceed.Oct 27 2022, 2:32 AM
tomek accepted this revision.Oct 27 2022, 4:37 AM
This revision is now accepted and ready to land.Oct 27 2022, 4:37 AM
tomek added inline comments.Oct 27 2022, 4:38 AM
services/tunnelbroker/src/main.rs
10 ↗(On Diff #17864)

I would rather mane a function a a verb initialize

• max updated this revision to Diff 18105.Nov 7 2022, 4:53 AM

Rebasing on parent changes.

Harbormaster failed remote builds in B13198: Diff 18105!Nov 7 2022, 4:54 AM
• max marked an inline comment as done.Nov 7 2022, 4:54 AM
• max added inline comments.
services/tunnelbroker/src/main.rs
10 ↗(On Diff #17864)

I would rather mane a function a a verb initialize

Looks ok to me to change it. Done in https://phab.comm.dev/D5429#164086.

• max mentioned this in rCOMMe8d2e66a98bc: [services] Tunnelbroker - Initialization function exposed to Rust.Nov 7 2022, 5:52 AM
• max updated this revision to Diff 18116.Nov 7 2022, 6:02 AM
• max marked an inline comment as done.

Rebasing.

This revision was landed with ongoing or failed builds.Nov 7 2022, 6:02 AM
Closed by commit rCOMM7c253be3d099: [services] Tunnelbroker - Creating `main.rs` entry point file for Rust (authored by • max). · Explain Why
This revision was automatically updated to reflect the committed changes.
• max added a commit: rCOMM7c253be3d099: [services] Tunnelbroker - Creating `main.rs` entry point file for Rust.
Harbormaster failed remote builds in B13205: Diff 18116!Nov 7 2022, 6:02 AM

Revision Contents
Changeset List

PathSize
services/
tunnelbroker/
src/
11 lines

Diff 17841

View Options

services/tunnelbroker/src/main.rs

  • This file was added.
// @flow
import { createDrawerNavigator } from '@react-navigation/drawer';
import * as React from 'react';
import { Dimensions, View } from 'react-native';
import { useStyles } from '../themes/colors';
import type { AppNavigationProp } from './app-navigator.react';
import CommunityDrawerContent from './community-drawer-content.react';
import { TabNavigatorRouteName } from './route-names';
import type { NavigationRoute } from './route-names';
import TabNavigator from './tab-navigator.react';
const Drawer = createDrawerNavigator();
const communityDrawerContent = () => <CommunityDrawerContent />;
type Props = {
+navigation: AppNavigationProp<'CommunityDrawerNavigator'>,
+route: NavigationRoute<'CommunityDrawerNavigator'>,
};
// eslint-disable-next-line no-unused-vars
function CommunityDrawerNavigator(props: Props): React.Node {
const styles = useStyles(unboundStyles);
const screenOptions = React.useMemo(
() => ({
drawerStyle: styles.drawerStyle,
}),
[styles.drawerStyle],
);
return (
<View style={styles.drawerView}>
<Drawer.Navigator
screenOptions={screenOptions}
drawerContent={communityDrawerContent}
tomekUnsubmitted
Not Done
Inline Actions
screenOptions={screenOptions}
- drawerContent={communityDrawerContent}
+ drawerContent={CommunityDrawerContent}
>
<Drawer.Screen name={TabNavigatorRouteName} component={TabNavigator} />

Are we sure that Flow doesn't allow us to use CommunityDrawerContent directly? Ultimately, it is a function that doesn't take arguments, so it should be possible to use here (but maybe Flow disagrees with that).

tomek: Are we sure that Flow doesn't allow us to use `CommunityDrawerContent` directly? Ultimately, it…
inkaAuthorUnsubmitted
Done
Inline Actions

Flow doesn't allow that. It says that React.ComponentType is incompatible with function type.
React.ComponentType comes from the memoization of CommunityDrawerContent

inka: Flow doesn't allow that. It says that `React.ComponentType is incompatible with function type`.
>
<Drawer.Screen name={TabNavigatorRouteName} component={TabNavigator} />
</Drawer.Navigator>
</View>
);
}
const unboundStyles = {
drawerView: {
flex: 1,
},
drawerStyle: {
width: Dimensions.get('window').width - 36,
ashoatUnsubmitted
Not Done
Inline Actions

Following ENG-2413 getting cancelled, we should remove this. Mentioning here because there is no longer a Linear task tracking the removal

ashoat: Following [ENG-2413](https://linear.app/comm/issue/ENG-2413/use-a-hook-in-drawer-navigator-for…
},
};
export { CommunityDrawerNavigator };