diff --git a/Cargo.lock b/Cargo.lock index 203cb89..08b7a0c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -622,7 +622,6 @@ checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" dependencies = [ "block-buffer 0.10.4", "crypto-common 0.1.7", - "subtle", ] [[package]] @@ -928,9 +927,9 @@ checksum = "e629b9b98ef3dd8afe6ca2bd0f89306cec16d43d907889945bc5d6687f2f13c7" [[package]] name = "gitlab-runner" -version = "0.3.0" +version = "0.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "58f8bfd3287e403d83d657148efc607c1aa744d0ff7ac3a6dcb8797511d0fe59" +checksum = "2d06d0941288bf5a7b298689c2deb7a13384d621890f17d2591064ef194a5f4b" dependencies = [ "async-trait", "bytes", @@ -938,14 +937,14 @@ dependencies = [ "flate2", "futures", "glob", - "hmac 0.12.1", + "hmac", "parking_lot", "pin-project", "rand 0.10.1", "reqwest", "serde", "serde_json", - "sha2", + "sha2 0.11.0", "tempfile", "thiserror 2.0.18", "tokio", @@ -960,9 +959,9 @@ dependencies = [ [[package]] name = "gitlab-runner-mock" -version = "0.3.0" +version = "0.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1b7089aed97831b12b9a80fe2f3767d0211d48bf539acc076654e2cdb00dff99" +checksum = "f2791be42dfb5f8e07fe01345488256ce27a875570a352f58848203545447ba7" dependencies = [ "futures", "http", @@ -1026,15 +1025,6 @@ version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fc0fef456e4baa96da950455cd02c081ca953b141298e41db3fc7e36b1da849c" -[[package]] -name = "hmac" -version = "0.12.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6c49c37c09c17a53d937dfbb742eb3a961d65a994e6bcdcf37e7399d0cc8ab5e" -dependencies = [ - "digest 0.10.7", -] - [[package]] name = "hmac" version = "0.13.0" @@ -1497,7 +1487,7 @@ version = "0.16.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "47bb1e988e6fb779cf720ad431242d3f03167c1b3f2b1aae7f1a94b2495b36ae" dependencies = [ - "sha2", + "sha2 0.10.9", ] [[package]] @@ -1624,7 +1614,7 @@ dependencies = [ [[package]] name = "obo-cli" -version = "0.1.8" +version = "0.2.0" dependencies = [ "async-trait", "camino", @@ -1706,7 +1696,7 @@ dependencies = [ [[package]] name = "obs-gitlab-runner" -version = "0.1.8" +version = "0.2.0" dependencies = [ "async-trait", "camino", @@ -1756,8 +1746,7 @@ checksum = "384b8ab6d37215f3c5301a95a4accb5d64aa607f1fcb26a11b5303878451b4fe" [[package]] name = "open-build-service-api" version = "0.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3b57803d6cf55358b48dfd66339f33ba936ce5c5c9a7c388889bad93c0c5a880" +source = "git+https://github.com/collabora/open-build-service-rs#8c15e37ce07d22c431e3091d57363491b88e1bfc" dependencies = [ "base16ct", "bytes", @@ -1775,8 +1764,7 @@ dependencies = [ [[package]] name = "open-build-service-mock" version = "0.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b8347f969ce3900870331890aead44992608bee174ecc77bf611133342300cd9" +source = "git+https://github.com/collabora/open-build-service-rs#8c15e37ce07d22c431e3091d57363491b88e1bfc" dependencies = [ "base16ct", "base64ct", @@ -1838,7 +1826,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "112d82ceb8c5bf524d9af484d4e4970c9fd5a0cc15ba14ad93dccd28873b0629" dependencies = [ "digest 0.11.3", - "hmac 0.13.0", + "hmac", ] [[package]] @@ -2476,6 +2464,17 @@ dependencies = [ "digest 0.10.7", ] +[[package]] +name = "sha2" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "446ba717509524cb3f22f17ecc096f10f4822d76ab5c0b9822c5f9c284e825f4" +dependencies = [ + "cfg-if", + "cpufeatures 0.3.0", + "digest 0.11.3", +] + [[package]] name = "sharded-slab" version = "0.1.7" @@ -3805,7 +3804,7 @@ dependencies = [ "deflate64", "flate2", "getrandom 0.4.2", - "hmac 0.13.0", + "hmac", "indexmap", "lzma-rust2", "memchr", diff --git a/Cargo.toml b/Cargo.toml index 0e5c2d7..36ffa82 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,6 +8,11 @@ members = [ "obs-gitlab-runner" ] +[workspace.package] +version = "0.2.0" +edition = "2024" +license = "MIT OR Apache-2.0" + [workspace.dependencies] async-trait = "0.1" camino = "1.2" @@ -16,9 +21,9 @@ clap = { version = "4.6", features = ["default", "derive", "env"] } color-eyre = "0.6" derivative = "2.2" futures-util = "0.3" -open-build-service-api = "0.1.0" +open-build-service-api = { git = "https://github.com/collabora/open-build-service-rs" } # open-build-service-api = { path = "../open-build-service-rs/open-build-service-api" } -open-build-service-mock = "0.1.0" +open-build-service-mock = { git = "https://github.com/collabora/open-build-service-rs" } # open-build-service-mock = { path = "../open-build-service-rs/open-build-service-mock" } rstest = "0.26" serde = "1.0" diff --git a/obo-cli/Cargo.toml b/obo-cli/Cargo.toml index 4d1a7c8..6beec2b 100644 --- a/obo-cli/Cargo.toml +++ b/obo-cli/Cargo.toml @@ -1,9 +1,9 @@ [package] name = "obo-cli" description = "OBS Build Orchestrator — command-line frontend" -version = "0.1.8" -edition = "2024" -license = "MIT OR Apache-2.0" +version.workspace = true +edition.workspace = true +license.workspace = true # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html diff --git a/obo-cli/tests/test_cli.rs b/obo-cli/tests/test_cli.rs index e9a9e0f..555cbf4 100644 --- a/obo-cli/tests/test_cli.rs +++ b/obo-cli/tests/test_cli.rs @@ -69,6 +69,13 @@ impl RunBuilder<'_> for CliRunBuilder { self } + fn saves(self, _patterns: I) -> Self + where + I::Item: AsRef, + { + self + } + fn artifacts(mut self, artifacts: Self::ArtifactsHandle) -> Self { self.dependencies.push(artifacts.0); self @@ -102,6 +109,14 @@ impl RunBuilder<'_> for CliRunBuilder { .env("OBS_SERVER", self.obs_server) .env("OBS_USER", TEST_USER) .env("OBS_PASSWORD", TEST_PASS) + .env( + "OBO_LOG", + if should_enable_trace_logging() { + "obo_core=trace,obo_cli=trace" + } else { + "" + }, + ) .env("OBO_TEST_LOG_TAIL", MONITOR_TEST_LOG_TAIL.to_string()) .env("OBO_TEST_SLEEP_ON_BUILDING_MS", "0") .env( @@ -241,6 +256,7 @@ async fn test_monitor_table( let generate = context .run() .command(generate_command) + .saves(&[DEFAULT_MONITOR_TABLE]) .artifacts(dput.clone()) .go() .await; @@ -271,6 +287,7 @@ async fn test_monitor_table( build_info, &enabled.repo_arch, &script, + &[], success, dput_test, log_test, diff --git a/obo-core/Cargo.toml b/obo-core/Cargo.toml index 6c0f2d1..3689a44 100644 --- a/obo-core/Cargo.toml +++ b/obo-core/Cargo.toml @@ -2,8 +2,8 @@ name = "obo-core" description = "OBS Build Orchestrator — core" version = "0.1.0" -edition = "2024" -license = "MIT OR Apache-2.0" +edition.workspace = true +license.workspace = true [dependencies] async-trait.workspace = true diff --git a/obo-test-support/Cargo.toml b/obo-test-support/Cargo.toml index e6c281c..0e1304d 100644 --- a/obo-test-support/Cargo.toml +++ b/obo-test-support/Cargo.toml @@ -1,8 +1,8 @@ [package] name = "obo-test-support" version = "0.1.0" -edition = "2024" -license = "MIT OR Apache-2.0" +edition.workspace = true +license.workspace = true [dependencies] open-build-service-api.workspace = true diff --git a/obo-tests/Cargo.toml b/obo-tests/Cargo.toml index 4b938f7..443ce63 100644 --- a/obo-tests/Cargo.toml +++ b/obo-tests/Cargo.toml @@ -2,8 +2,8 @@ name = "obo-tests" description = "OBS Build Orchestrator — shared tests for different frontends" version = "0.1.0" -edition = "2024" -license = "MIT OR Apache-2.0" +edition.workspace = true +license.workspace = true [dependencies] async-trait.workspace = true diff --git a/obo-tests/src/lib.rs b/obo-tests/src/lib.rs index 7705212..0c33dcf 100644 --- a/obo-tests/src/lib.rs +++ b/obo-tests/src/lib.rs @@ -17,6 +17,10 @@ use obo_test_support::*; use open_build_service_api as obs; use open_build_service_mock::*; +pub fn should_enable_trace_logging() -> bool { + std::env::var_os("OBO_TEST_TRACE").is_some() +} + #[derive(Clone)] pub struct ObsContext { pub client: obs::Client, @@ -51,6 +55,13 @@ pub trait RunBuilder<'context>: Send + Sync + Sized { self.script(&[cmd.into()]) } + // Sets the patterns of files that will be saved by the given commands, so + // that implementations that need to list out saved artifacts like the + // gitlab runner can verify those lists are correct. + fn saves(self, _patterns: I) -> Self + where + I::Item: AsRef; + fn script(self, cmd: &[String]) -> Self; fn artifacts(self, artifacts: Self::ArtifactsHandle) -> Self; fn timeout(self, timeout: Duration) -> Self; @@ -179,6 +190,7 @@ pub async fn test_dput( let dput = context .run() .command(dput_command.replace(dsc1_file, dsc1_bad_file)) + .saves(&[DEFAULT_BUILD_INFO]) .artifacts(artifacts.clone()) .go() .await; @@ -197,6 +209,7 @@ pub async fn test_dput( let mut dput = context .run() .command(&dput_command) + .saves(&[DEFAULT_BUILD_INFO]) .artifacts(artifacts.clone()) .go() .await; @@ -273,6 +286,7 @@ pub async fn test_dput( dput = context .run() .command(&dput_command) + .saves(&[DEFAULT_BUILD_INFO]) .artifacts(artifacts.clone()) .go() .await; @@ -295,6 +309,7 @@ pub async fn test_dput( dput = context .run() .command(format!("{dput_command} --rebuild-if-unchanged")) + .saves(&[DEFAULT_BUILD_INFO]) .artifacts(artifacts.clone()) .go() .await; @@ -399,6 +414,7 @@ pub async fn test_monitoring( build_info: &ObsBuildInfo, repo: &RepoArch, script: &[String], + artifact_paths: &[String], success: bool, dput_test: DputTest, log_test: MonitorLogTest, @@ -504,6 +520,7 @@ pub async fn test_monitoring( let monitor = context .run() .script(script) + .saves(artifact_paths) .artifacts(dput.clone()) .timeout(MONITOR_TEST_OLD_STATUS_SLEEP_DURATION * 20) .go() @@ -529,7 +546,19 @@ pub async fn test_monitoring( ); assert_eq!( - log.contains(&log_contents), + // If trace-level logging is enabled, and the implementation mixes + // together script output with the trace logs (i.e. the CLI), then our + // rather short test log *probably* ended up in the log output at some + // point. In that case, make sure to require a leading newline, so it + // should only match if the logs were explicitly printed on its own + // line(s) vs the debug/trace-level logs that have a header. (This is + // very janky, but it *only* applies in an explicitly opt-in case, so if + // anything actually breaks it's not a huge deal.) + if should_enable_trace_logging() { + log.contains(&format!("\n{log_contents}")) + } else { + log.contains(&log_contents) + }, !success && log_test == MonitorLogTest::Short ); diff --git a/obs-gitlab-runner/Cargo.toml b/obs-gitlab-runner/Cargo.toml index 36dc964..8482d2c 100644 --- a/obs-gitlab-runner/Cargo.toml +++ b/obs-gitlab-runner/Cargo.toml @@ -1,8 +1,8 @@ [package] name = "obs-gitlab-runner" -version = "0.1.8" -edition = "2024" -license = "MIT OR Apache-2.0" +version.workspace = true +edition.workspace = true +license.workspace = true # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html @@ -13,7 +13,7 @@ clap.workspace = true color-eyre.workspace = true derivative.workspace = true futures-util.workspace = true -gitlab-runner = "0.3.0" +gitlab-runner = "0.3.1" # gitlab-runner = { path = "../../gitlab-runner-rs/gitlab-runner" } obo-core = { path = "../obo-core" } obo-test-support = { path = "../obo-test-support" } @@ -35,8 +35,8 @@ url = "2.5" [dev-dependencies] claims.workspace = true -gitlab-runner-mock = "0.3.0" -# gitlab-runner-mock = { path = "../gitlab-runner-rs/gitlab-runner-mock" } +gitlab-runner-mock = "0.3.1" +# gitlab-runner-mock = { path = "../../gitlab-runner-rs/gitlab-runner-mock" } obo-tests = { path = "../obo-tests" } open-build-service-mock.workspace = true rstest.workspace = true diff --git a/obs-gitlab-runner/src/handler.rs b/obs-gitlab-runner/src/handler.rs index e8fb5d0..6844038 100644 --- a/obs-gitlab-runner/src/handler.rs +++ b/obs-gitlab-runner/src/handler.rs @@ -453,7 +453,7 @@ mod tests { use rstest::rstest; use tempfile::TempDir; use tracing::{Level, instrument::WithSubscriber}; - use tracing_subscriber::{Layer, Registry, filter::Targets, prelude::*}; + use tracing_subscriber::{Registry, filter::Targets, prelude::*}; use zip::ZipArchive; use crate::logging::GitLabForwarder; @@ -576,6 +576,7 @@ mod tests { variables: HashMap, handler: Box, timeout: Duration, + artifacts_patterns: Vec, } fn create_obs_job_handler_factory( @@ -629,6 +630,15 @@ mod tests { self } + fn saves(mut self, patterns: I) -> Self + where + I::Item: AsRef, + { + self.artifacts_patterns + .extend(patterns.into_iter().map(|x| x.as_ref().to_owned())); + self + } + fn artifacts(mut self, artifacts: Self::ArtifactsHandle) -> Self { self.dependencies.push(artifacts.0); self @@ -662,15 +672,17 @@ mod tests { ); } - builder.add_artifact( - None, - false, - vec!["*".to_owned()], - Some(MockJobArtifactWhen::Always), - "archive".to_owned(), - Some("zip".to_owned()), - None, - ); + if !self.artifacts_patterns.is_empty() { + builder.add_artifact( + None, + false, + self.artifacts_patterns, + Some(MockJobArtifactWhen::Always), + "archive".to_owned(), + Some("zip".to_owned()), + None, + ); + } for dependency in self.dependencies { builder.dependency(dependency); @@ -729,6 +741,7 @@ mod tests { artifacts: HashMap>, ) -> Self::ArtifactsHandle { self.run() + .saves(artifacts.keys()) .job_handler_factory(|_| PutArtifactsHandler { artifacts: Arc::new(artifacts), }) @@ -776,6 +789,7 @@ mod tests { _phantom: PhantomData, }), timeout: EXECUTION_DEFAULT_TIMEOUT, + artifacts_patterns: vec![], } } } @@ -816,9 +830,19 @@ mod tests { .with( tracing_subscriber::fmt::layer() .with_test_writer() - .with_filter( - Targets::new().with_target("obs_gitlab_runner", Level::TRACE), - ), + .with_filter(if should_enable_trace_logging() { + Targets::new() + .with_target("obo_core", Level::TRACE) + .with_target("obs_gitlab_runner", Level::TRACE) + } else { + // If trace-level logging isn't enabled, we want + // the global filter to match what the runner is + // typically run with as closely as possible. + // Since 'tracing' is very complex, this helps + // ensure we don't break something in a way that + // passes tests but fails in practice. + Targets::new().with_default(Level::INFO) + }), ) .with(tracing_error::ErrorLayer::default()) .with(GitLabForwarder::new(layer)), @@ -851,6 +875,7 @@ mod tests { let generate = context .run() .command(generate_command) + .saves(&[DEFAULT_MONITOR_PIPELINE]) .artifacts(dput.clone()) .go() .await; @@ -897,7 +922,10 @@ mod tests { if download_binaries { assert_eq!( &artifact_paths, - &[DEFAULT_BUILD_LOG, MONITOR_TEST_BUILD_RESULTS_DIR] + &[ + DEFAULT_BUILD_LOG, + &format!("{MONITOR_TEST_BUILD_RESULTS_DIR}/*") + ] ); } else { assert_eq!(&artifact_paths, &[DEFAULT_BUILD_LOG]); @@ -943,6 +971,10 @@ mod tests { build_info, &enabled.repo_arch, &script, + &artifact_paths + .into_iter() + .map(ToOwned::to_owned) + .collect::>(), success, dput_test, log_test, @@ -1153,6 +1185,7 @@ mod tests { } else { context.run().command("generate-monitor tag") } + .saves(&[DEFAULT_MONITOR_PIPELINE]) .artifacts(build_info); let generate = if test == Some(GenerateMonitorTimeoutLocation::HandlerOption) { diff --git a/obs-gitlab-runner/src/logging.rs b/obs-gitlab-runner/src/logging.rs index 41e11b9..8b277a2 100644 --- a/obs-gitlab-runner/src/logging.rs +++ b/obs-gitlab-runner/src/logging.rs @@ -73,12 +73,32 @@ impl LookupSpan<'span>, F: Fi } fn on_event(&self, event: &Event<'_>, ctx: Context<'_, S>) { + // When Filtered's Layer implementation methods are invoked on an event + // *prior* to on_event() (e.g. enabled()), it will set thread-local + // state indicating whether or not the currently-processed event should + // be enabled. Then, within on_event(), if the event was in fact + // disabled, the thread-local state gets reset, to clear things out for + // the next event to be processed. In other words, Filtered holds hard + // assumptions about the order of methods called when processing an + // event, and it uses these assumptions to manage its state. + // + // This also means that, if we don't *always* call on_event here, those + // assumptions are *violated*, and an event being disabled can carry + // over to the processing of the next event. + self.0.on_event(event, ctx.clone()); + if !is_output_field_set_in_event(event) { - // No special behavior needed, so just forward it as-is. - self.0.on_event(event, ctx); + // No special behavior needed, so just leave things as-is. return; } + // If an event had both the obo *and* gitlab-runner output fields set, + // then it would get logged twice (once by the above on_event, and once + // by our bypass below). This is pretty obvious to avoid, but it's still + // worth making sure that in debug builds (e.g. tests) it doesn't + // happen. + debug_assert!(!self.0.filter().enabled(event.metadata(), &ctx)); + let Some(message) = get_event_message(event) else { return; }; @@ -107,7 +127,8 @@ impl LookupSpan<'span>, F: Fi }; // Bypass the filter completely, because the event was almost certainly - // filtered out in its `enabled` due to lacking `gitlab.output`. + // filtered out due to lacking `gitlab.output` (and we already called + // on_event() for the filter at the start of the function). self.0.inner().on_event(&event, ctx); } diff --git a/obs-gitlab-runner/src/pipeline.rs b/obs-gitlab-runner/src/pipeline.rs index a0295fa..6511e80 100644 --- a/obs-gitlab-runner/src/pipeline.rs +++ b/obs-gitlab-runner/src/pipeline.rs @@ -96,7 +96,12 @@ pub fn generate_monitor_pipeline( } .generate_command(), ); - artifact_paths.push(build_results_dir.clone()); + artifact_paths.push(format!( + "{}/*", + build_results_dir + .strip_suffix('/') + .unwrap_or(build_results_dir) + )); } jobs.insert(