Make installer a library with capturable stdout #691
Conversation
untitaker
force-pushed
the
installer-library
branch
2 times, most recently
from
4b5d204 to
0e9ff71
Compare
|
It seems like you've both rewritten chunks of and renamed the installer's main.rs -- can you cut those changes into separate commits so this is easier to review? |
installer/src/lib.rs
Outdated
| /// // Without callback (uses stdout/stderr) | ||
| /// let result = installer::run_with_callback( | ||
| /// vec!["installer", "tplink"], | ||
| /// None | ||
| /// ); |
There was a problem hiding this comment.
This is pretty ugly and boilerplatey. Why do we need this extra layer instead of just calling run?
There was a problem hiding this comment.
I don't think its necessarily ugly, if its useful I'm fine with it but I'm unclear right now on what the intended purpose is since we don't currently use it, its just dead code for now, but maybe there is some future plan for it.
There was a problem hiding this comment.
I updated the code to make use of it in installer-gui to demonstrate what I mean. As a result the build instructions for Tauri simplify, and I can embed it in my Android app.
| tokio-stream = "0.1.17" | ||
|
|
||
| [target.'cfg(target_os = "linux")'.dependencies.adb_client] | ||
| [target.'cfg(all(target_os = "linux", not(target_os = "android")))'.dependencies.adb_client] |
There was a problem hiding this comment.
We were running into problems on Android?
e: We spoke OOB: yes we were.
There was a problem hiding this comment.
yes. fwiw i have not added any CI for android because I am building the installer in another project. trying to keep the changes here relatively light. I've not really dug into the build failures for adb_client but all the USB installers are not really useful on Android anyway.
installer/src/lib.rs
Outdated
| /// let version = installer::version(); | ||
| /// assert!(!version.is_empty()); |
|
hi @oopsbagel, as discussed in private i think i opened the PR too prematurely. i added context in the PR description. |
cooperq
left a comment
cooperq
left a comment
There was a problem hiding this comment.
approved but please address the remaining comment.
installer/src/lib.rs
Outdated
| /// // Without callback (uses stdout/stderr) | ||
| /// let result = installer::run_with_callback( | ||
| /// vec!["installer", "tplink"], | ||
| /// None | ||
| /// ); |
There was a problem hiding this comment.
I don't think its necessarily ugly, if its useful I'm fine with it but I'm unclear right now on what the intended purpose is since we don't currently use it, its just dead code for now, but maybe there is some future plan for it.
cooperq
left a comment
cooperq
left a comment
There was a problem hiding this comment.
Actually sorry, also please make sure that the GUI installer still works with these changes or fix anything that breaks.
bmw
left a comment
bmw
left a comment
There was a problem hiding this comment.
i am in favor of turning the installer into a library like this as it makes building installer-gui significantly simpler. i'm slightly concerned that introducing differences between how the CLI and GUI installers call this code will lead to differences in output, especially around things like panics which i'd very much like to avoid, but this seems solvable
mostly in response to your PR description, i want to share that i'm personally hesitant to have users pass CLI arguments through installer-gui in the long term, even as an escape hatch, but don't have a better idea right now and i think my opinion will depend a lot on what the glue between the GUI frontend and the installer library ends up looking like. i think this design makes a lot of sense for your android app though where "just download and run the CLI installer if you want to use CLI args" isn't as sane advice. as you know tho, passing around CLI arguments like this is the current approach in installer-gui so i think at least at a high level, the design in this PR makes a lot of sense (i didn't get into the weeds) and i think this is worth supporting while it's potentially useful to either of the GUIs
also, there's really only so much we can do with all the dependencies involved, but if there are ways you think we/i can simplify the process of building installer-gui, i'd be happy to work on that. i'm in favor of keeping the GUIs as "dumb" as possible despite any improvements we make there, but if there are pain points that could be addressed, i suspect doing so is worth it regardless
installer/src/lib.rs
Outdated
| /// Run the CLI installer (for use by the installer binary only) | ||
| /// | ||
| /// This function is public so the binary can call it, but library users | ||
| /// should use the typed functions like `install_orbic_network` instead. |
There was a problem hiding this comment.
is this supposed to be orbic_network::install?
also, what are you wanting the GUIs to call? i assume the answer is run_with_callback but this code comment makes me unsure
There was a problem hiding this comment.
yeah it's run_with_callback, this docstring was outdated from previous iterations
untitaker
force-pushed
the
installer-library
branch
3 times, most recently
from
6bfe142 to
2734cc8
Compare
untitaker
force-pushed
the
installer-library
branch
from
d8d0642 to
765b806
Compare
Motivation
In #687 we introduce a GUI that asks the user for command-line args, shells out to the existing CLI to drive the installation process, and displays the installer output in the GUI. using subprocesses for this makes the build process more complicated. if we instead expose the CLI + its output as a library, we can bundle the installer CLI into the GUI without the use of
include_bytes!and we save one invocation of cargo (one invocation for rayhunter-daemon, one for the installer, one for the GUI, ... now becomes just rayhunter-daemon, then GUI)In an unrelated project I am bundling the installer CLI into a Kotlin Android app (just as an experiment... it's not ready yet), with a similar user experience as above's installer desktop GUI. The user selects a wifi network, types out CLI parameters and the installer will ask the system to connect to that wifi network, disable various "connectivity tests" by Android and run the installer against that WiFi network without routing the phone's full internet traffic through the orbic/tplink. This special network routing is useful because the same app also can monitor rayhunter for alerts without disrupting the phone's internet connection. All other apps will run via phone's 4G/5G, and not through the mobile hotspot.
In order to do the routing Android provides an API to "bind processes to networks". A process is bound to a network using special functions in Android's libc, which means that when you shell out to static binaries, that functionality just doesn't exist. Any subprocess spawned will just go to the default route, which is whatever Android thinks provides a "useful internet connection." The SIM card in my tplink isn't activated though.
So TL;DR, due to various networking shenanigans, I need the installer built as a dynamic library so I can run it in the same process as the rest of the Kotlin app and using Android's system libc. For this purpose I expose the installer as a rust library, then (within the Android app project) wrap that in a JNI interface.
Why this weird API?
I basically have two requirements, I need to be able to capture the output of the installer, and I need to be able to provide it arguments without modifying global state like argv.
This is the simplest thing I can come up with. Ideally you'd provide a proper typed Rust API, but given that the current GUIs we have ask the user for command-line arguments, this seems like the easier choice at the moment.
Random thoughts about GUI design
This is elaboration on the comment I did in #687.
I'm also a bit concerned if we ever change the GUI to something "nicer", that over time we end up accumulating a ton of business logic in the GUIs. Particularly if we expose the Command enum, the installer GUI will have to have a ton of glue code to map whatever checkbox exists in the UI to some command's option. It will become harder to just add an option to an existing installer. What concerns me is not the chore of adding to the glue code, but testing that change locally (i.e. having to build the Tauri app locally).
Given that, I think i'd like the GUI to always stay a "dumb" wrapper around the CLI (not some other API), and to always allow the user to specify additional CLI args (or write their own) that will be passed through to the CLI. I think we can still make the interface nice by providing "suggested commands" to the user that pre-fill the installer args textbox.