diff --git a/rust/gui-client/src-tauri/src/client/gui.rs b/rust/gui-client/src-tauri/src/client/gui.rs index bb08e3037..a0100dcbd 100644 --- a/rust/gui-client/src-tauri/src/client/gui.rs +++ b/rust/gui-client/src-tauri/src/client/gui.rs @@ -170,8 +170,7 @@ pub(crate) fn run( let ctlr_tx = ctlr_tx.clone(); tokio::spawn(async move { if let Err(error) = smoke_test(ctlr_tx).await { - tracing::error!(?error, "Error during smoke test"); - tracing::error!("Crashing on purpose so a dev can see our stacktraces"); + tracing::error!(?error, "Error during smoke test, crashing on purpose so a dev can see our stacktraces"); unsafe { sadness_generator::raise_segfault() } } }); @@ -324,8 +323,8 @@ async fn smoke_test(ctlr_tx: CtlrTx) -> Result<()> { .await .context("Failed to get zip file metadata")? .len(); - if zip_len == 0 { - bail!("Exported log zip has 0 bytes"); + if zip_len <= 22 { + bail!("Exported log zip just has the file header"); } tokio::fs::remove_file(&path) .await diff --git a/rust/gui-client/src-tauri/src/client/logging.rs b/rust/gui-client/src-tauri/src/client/logging.rs index 767003516..502231f7e 100644 --- a/rust/gui-client/src-tauri/src/client/logging.rs +++ b/rust/gui-client/src-tauri/src/client/logging.rs @@ -82,7 +82,7 @@ pub(crate) async fn clear_logs() -> StdResult<(), String> { #[tauri::command] pub(crate) async fn export_logs(managed: tauri::State<'_, Managed>) -> StdResult<(), String> { - export_logs_inner(managed.ctlr_tx.clone()).map_err(|e| e.to_string()) + show_export_dialog(managed.ctlr_tx.clone()).map_err(|e| e.to_string()) } #[derive(Clone, Default, Serialize)] @@ -112,7 +112,7 @@ pub(crate) async fn clear_logs_inner() -> Result<()> { } /// Pops up the "Save File" dialog -pub(crate) fn export_logs_inner(ctlr_tx: CtlrTx) -> Result<()> { +fn show_export_dialog(ctlr_tx: CtlrTx) -> Result<()> { let now = chrono::Local::now(); let datetime_string = now.format("%Y_%m_%d-%H-%M"); let stem = PathBuf::from(format!("firezone_logs_{datetime_string}")); @@ -144,15 +144,18 @@ pub(crate) fn export_logs_inner(ctlr_tx: CtlrTx) -> Result<()> { /// * `stem` - A directory containing all the log files inside the zip archive, to avoid creating a ["tar bomb"](https://www.linfo.org/tarbomb.html). This comes from the automatically-generated name of the archive, even if the user changes it to e.g. `logs.zip` pub(crate) async fn export_logs_to(path: PathBuf, stem: PathBuf) -> Result<()> { tracing::info!("Exporting logs to {path:?}"); + // Use a temp path so that if the export fails we don't end up with half a zip file + let temp_path = path.with_extension(".zip-partial"); // TODO: Consider https://github.com/Majored/rs-async-zip/issues instead of `spawn_blocking` spawn_blocking(move || { - let f = fs::File::create(path).context("Failed to create zip file")?; + let f = fs::File::create(&temp_path).context("Failed to create zip file")?; let mut zip = zip::ZipWriter::new(f); for log_path in log_paths().context("Can't compute log paths")? { add_dir_to_zip(&mut zip, &log_path.src, &stem.join(log_path.dst))?; } zip.finish().context("Failed to finish zip file")?; + fs::rename(&temp_path, &path)?; Ok::<_, anyhow::Error>(()) }) .await diff --git a/scripts/tests/smoke-test-gui-linux.sh b/scripts/tests/smoke-test-gui-linux.sh index dfb2fc5f3..40a3f3a39 100755 --- a/scripts/tests/smoke-test-gui-linux.sh +++ b/scripts/tests/smoke-test-gui-linux.sh @@ -1,15 +1,20 @@ #!/usr/bin/env bash +# Put `set -euox` in the top-level scripts directly. If it's only sourced, +# and the source path is wrong, it will not throw an error and it'll be hard +# to debug. +# set -euox pipefail BUNDLE_ID="dev.firezone.client" FZ_GROUP="firezone-client" -#DEVICE_ID_PATH="/var/lib/$BUNDLE_ID/config/firezone-id.json" LOGS_PATH="$HOME/.cache/$BUNDLE_ID/data/logs" + DUMP_PATH="$LOGS_PATH/last_crash.dmp" -SETTINGS_PATH="$HOME/.config/$BUNDLE_ID/config/advanced_settings.json" +IPC_LOGS_PATH="/var/log/$BUNDLE_ID" RAN_BEFORE_PATH="$HOME/.local/share/$BUNDLE_ID/data/ran_before.txt" +SETTINGS_PATH="$HOME/.config/$BUNDLE_ID/config/advanced_settings.json" SYMS_PATH="../target/debug/firezone-gui-client.syms" PACKAGE=firezone-gui-client @@ -26,6 +31,9 @@ ls -lash ../target/debug sudo groupadd --force "$FZ_GROUP" sudo adduser "$USER" "$FZ_GROUP" +# Make the IPC log dir so that the zip export doesn't bail out +sudo mkdir -p "$IPC_LOGS_PATH" + function run_fz_gui() { pwd # Does what it says @@ -55,7 +63,8 @@ function smoke_test() { # Make sure the files were written in the right paths # TODO: Inject some bogus sign-in sequence to test the actor_name file # https://stackoverflow.com/questions/41321092 - bash -c "stat \"${LOGS_PATH}/\"connlib*log" + # TODO: Smoke test the IPC service + # bash -c "stat \"${LOGS_PATH}/\"connlib*log" stat "$SETTINGS_PATH" # stat "$DEVICE_ID_PATH" # `ran_before` is now only written after a successful sign-in diff --git a/scripts/tests/smoke-test-gui-windows.sh b/scripts/tests/smoke-test-gui-windows.sh index 0d9bd24d1..9fd9e8d02 100755 --- a/scripts/tests/smoke-test-gui-windows.sh +++ b/scripts/tests/smoke-test-gui-windows.sh @@ -3,7 +3,7 @@ # Read it before running on a dev system. # This script must run from an elevated shell so that Firezone won't try to elevate. -source "./scripts/tests/lib.sh" +set -euox pipefail # This prevents a `shellcheck` lint warning about using an unset CamelCase var if [[ -z "$ProgramData" ]]; then @@ -12,15 +12,18 @@ if [[ -z "$ProgramData" ]]; then fi BUNDLE_ID="dev.firezone.client" -DEVICE_ID_PATH="$ProgramData/$BUNDLE_ID/config/firezone-id.json" DUMP_PATH="$LOCALAPPDATA/$BUNDLE_ID/data/logs/last_crash.dmp" +IPC_LOGS_PATH="$ProgramData/$BUNDLE_ID/data/logs" PACKAGE=firezone-gui-client +# Make the IPC log dir so that the zip export doesn't bail out +mkdir -p "$IPC_LOGS_PATH" + function smoke_test() { + # This array used to have more items + # TODO: Smoke-test the IPC service files=( "$LOCALAPPDATA/$BUNDLE_ID/config/advanced_settings.json" - "$LOCALAPPDATA/$BUNDLE_ID/data/wintun.dll" - "$DEVICE_ID_PATH" ) # Make sure the files we want to check don't exist on the system yet @@ -34,25 +37,11 @@ function smoke_test() { # Run the smoke test normally cargo run --bin "$PACKAGE" -- smoke-test - # Note the device ID - DEVICE_ID_1=$(cat "$DEVICE_ID_PATH") - - # Run the test again and make sure the device ID is not changed - cargo run --bin "$PACKAGE" -- smoke-test - DEVICE_ID_2=$(cat "$DEVICE_ID_PATH") - - if [ "$DEVICE_ID_1" != "$DEVICE_ID_2" ] - then - echo "The device ID should not change if the file is intact between runs" - exit 1 - fi - # Make sure the files were written in the right paths for file in "${files[@]}" do stat "$file" done - stat "$LOCALAPPDATA/$BUNDLE_ID/data/logs/"connlib*log # Clean up so the test can be cycled for file in "${files[@]}"