Add a wrapper function for generate_device_id, as suggested in comments on D5341.
Details
Details
Insert the following to didFinishLaunchingWithOptions in AppDelegate.mm:
comm::Logger::log(comm::generateDeviceID("MOBILE"));Run ios and see that when app launches a string subject to "^(ks|mobile|web):[a-zA-Z0-9]{64}$" is logged to the console
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
CI fails because we haven't yet made rust work on android on CI. This is tracked in ENG-1852
Comment Actions
| native/cpp/CommonCpp/CryptoTools/DeviceID.h | ||
|---|---|---|
| 11 ↗ | (On Diff #17636) | Personally, I always encapsulate such functionalities inside a class, even if it is stateless and all its methods are static. We also follow this convention in most CommonCpp files. Moreover if we ever happen to need to export this to Java via JNI then having a class is a must. Nevertheless, at this moment having a class here is rather my feeling about what a high quality code looks like than a real technical necessity so I will not block this revision on this. |
Comment Actions
Some minor inline comments, but overall it looks ok
| native/cpp/CommonCpp/CryptoTools/DeviceID.h | ||
|---|---|---|
| 6 ↗ | (On Diff #17636) | We can consider this a constant and use CONSTANT_CASE or as a variable and use camelCase, but we should start a variable name with uppercase letter as it is reserved for e.g. classes. (this isn't actually a constant because it can be modified) (adding const somewhere would also be a good idea) |
| 6 ↗ | (On Diff #17636) | In this case the performance shouldn't matter, but when we don't care about ordering we should prefer hash table to a tree (use unordered_map instead of map). |
Comment Actions
Hmm... strange that we're still seeing Android build failures after a rebase. Here's what I see from the Buildkite log:
In file included from /workdir/native/cpp/CommonCpp/CryptoTools/DeviceID.cpp:1:
/workdir/native/cpp/CommonCpp/CryptoTools/DeviceID.h:1:10: fatal error: 'lib.rs.h' file not found
#include "lib.rs.h"
^~~~~~~~~~
1 error generated.Comment Actions
Wondering if this might be connected to what @marcin experienced in his approach https://linear.app/comm/issue/ENG-1758/implement-c-wrappers-around-blob-client-rust-functionality#comment-3ddeb97a
In C++ files that were calling rust (generated CXX bindings) I needed to include:
#include "native_rust_library/src/lib.rs.h"
This is in contrary to iOS where we jest include #include "lib.rs.h"
Comment Actions
@varun created an issue to track: https://linear.app/comm/issue/ENG-2108/potential-rust-android-ci-issue