Skip to content

Commit b50557c

Browse files
romtsnclaude
andauthored
fix(bundle-jvm): Strip source-set prefix from bundle URLs (#3275)
Strip the `[<module>/]src/<sourceset>/<lang>/` prefix from bundle URLs so Symbolicator can resolve source context from JVM stack traces. JVM stack traces reference classes by fully qualified name (e.g. `io.sentry.android.core.ANRWatchDog`) with no knowledge of which Gradle module or source-set directory on disk produced them. The previous URL format included the full on-disk prefix (e.g. `~/sentry-android-core/src/main/java/io/sentry/android/core/ANRWatchDog.jvm`), so Symbolicator's URL-based lookup against the bundle manifest could never match a stack frame. Stripping the prefix produces URLs that match the package layout Symbolicator expects (`~/io/sentry/android/core/ANRWatchDog.jvm`). Because stripping collapses different source sets into the same URL (e.g. `src/main/` and `src/debug/` both contributing `com.example.Foo` in an Android build variant), the command now tracks seen URLs and emits a `warn!` on collision, skipping all but the first-seen file. The warning points users at `--exclude` so they can scope the bundle to a single variant rather than bundling sources that would otherwise silently overwrite each other in the zip. Follow-up to #3260, which enabled running `bundle-jvm` directly on a multi-module project root. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 342be6c commit b50557c

3 files changed

Lines changed: 332 additions & 23 deletions

File tree

CHANGELOG.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,15 @@
11
# Changelog
22

3+
## Unreleased
4+
5+
### Improvements
6+
7+
- (bundle-jvm) Warn and skip subsequent duplicates when multiple files strip to the same URL (e.g. Android build variants contributing the same FQCN). The warning points users at `--exclude` to scope the bundle to a single variant ([#3275](https://github.com/getsentry/sentry-cli/pull/3275)).
8+
9+
### Fixes
10+
11+
- (bundle-jvm) Strip the `[<module>/]src/<sourceset>/<lang>/` prefix from bundle URLs so Symbolicator can resolve them from package-based stack traces (e.g. `sentry-android-core/src/main/java/io/sentry/android/core/ANRWatchDog.java` → `~/io/sentry/android/core/ANRWatchDog.jvm`) ([#3275](https://github.com/getsentry/sentry-cli/pull/3275)).
12+
313
## 3.4.0
414

515
### Features

src/commands/debug_files/bundle_jvm.rs

Lines changed: 308 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2,26 +2,115 @@
22

33
use crate::config::Config;
44
use crate::utils::args::ArgExt as _;
5-
use crate::utils::file_search::ReleaseFileSearch;
5+
use crate::utils::file_search::{ReleaseFileMatch, ReleaseFileSearch};
66
use crate::utils::file_upload::SourceFile;
77
use crate::utils::fs::path_as_url;
88
use crate::utils::source_bundle::{self, BundleContext};
99
use anyhow::{bail, Context as _, Result};
1010
use clap::{Arg, ArgAction, ArgMatches, Command};
11-
use log::debug;
11+
use log::{debug, warn};
12+
use regex::Regex;
1213
use sentry::types::DebugId;
13-
use std::collections::BTreeMap;
14+
use std::collections::hash_map::Entry;
15+
use std::collections::{BTreeMap, HashMap};
1416
use std::ffi::OsStr;
1517
use std::fs;
1618
use std::path::{Path, PathBuf};
1719
use std::str::FromStr as _;
18-
use std::sync::Arc;
20+
use std::sync::{Arc, LazyLock};
1921
use symbolic::debuginfo::sourcebundle::SourceFileType;
2022

2123
const JVM_EXTENSIONS: &[&str] = &[
2224
"java", "kt", "scala", "sc", "groovy", "gvy", "gy", "gsh", "clj", "cljc",
2325
];
2426

27+
/// Directory names that mark the root of a JVM source set (i.e. the parent of
28+
/// the package hierarchy). Matches the Gradle/Maven convention
29+
/// `src/<sourceset>/<lang>/<package>/...`.
30+
const SOURCE_SET_LANGS: &[&str] = &["java", "kotlin", "scala", "groovy", "clojure"];
31+
32+
static SOURCE_SET_PREFIX_RE: LazyLock<Regex> = LazyLock::new(|| {
33+
let langs = SOURCE_SET_LANGS.join("|");
34+
Regex::new(&format!(
35+
r"(?:^|[/\\])src[/\\][^/\\]+[/\\](?:{langs})[/\\](.+)$"
36+
))
37+
.expect("valid regex")
38+
});
39+
40+
/// Strips the `[<module>/]src/<sourceset>/<lang>/` prefix from a relative source
41+
/// path so the remaining portion matches what Symbolicator looks up by URL
42+
/// (e.g. `io/sentry/android/core/ANRWatchDog.java`). This is needed because
43+
/// JVM stack traces reference classes by their package path, with no knowledge
44+
/// of the containing Gradle module or source-set layout on disk.
45+
///
46+
/// Returns the path unchanged if no `src/<sourceset>/<lang>/` segment is found.
47+
fn strip_source_set_prefix(relative_path: &Path) -> PathBuf {
48+
relative_path
49+
.to_str()
50+
.and_then(|s| SOURCE_SET_PREFIX_RE.captures(s))
51+
.map(|caps| PathBuf::from(&caps[1]))
52+
.unwrap_or_else(|| relative_path.to_path_buf())
53+
}
54+
55+
/// Builds the Symbolicator-compatible URL for a relative source path
56+
/// (e.g. `~/io/sentry/android/core/ANRWatchDog.jvm`).
57+
fn build_source_url(relative_path: &Path) -> String {
58+
let package_path = strip_source_set_prefix(relative_path);
59+
let package_path_jvm_ext = package_path.with_extension("jvm");
60+
format!("~/{}", path_as_url(&package_path_jvm_ext))
61+
}
62+
63+
/// Turns walked source files into `SourceFile`s for bundling, filtering out
64+
/// build-output directories and deduplicating by URL.
65+
///
66+
/// Android build variants can contribute the same FQCN from different source
67+
/// sets (e.g. `src/main/` and `src/debug/` both defining `com.example.Foo`).
68+
/// After stripping, both map to the same URL — this keeps the first-seen
69+
/// entry and warns the user about the rest.
70+
fn build_source_files(sources: Vec<ReleaseFileMatch>) -> Vec<SourceFile> {
71+
let candidates = sources.into_iter().filter_map(|source| {
72+
let local_path = source.path.strip_prefix(&source.base_path).unwrap();
73+
if is_in_ambiguous_build_dir(local_path) {
74+
debug!("excluding (build output): {}", source.path.display());
75+
return None;
76+
}
77+
let url = build_source_url(local_path);
78+
Some((url, source))
79+
});
80+
81+
let mut seen_urls: HashMap<String, usize> = HashMap::new();
82+
let mut files: Vec<SourceFile> = Vec::new();
83+
84+
for (url, source) in candidates {
85+
match seen_urls.entry(url) {
86+
Entry::Occupied(existing) => {
87+
warn!(
88+
"URL collision on {}: skipping '{}' (already bundled from '{}'). \
89+
Use --exclude to drop the unwanted source set \
90+
(e.g. --exclude='**/src/debug/**').",
91+
existing.key(),
92+
source.path.display(),
93+
files[*existing.get()].path.display(),
94+
);
95+
}
96+
Entry::Vacant(slot) => {
97+
let url = slot.key().clone();
98+
slot.insert(files.len());
99+
files.push(SourceFile {
100+
url,
101+
path: source.path,
102+
contents: Arc::new(source.contents),
103+
ty: SourceFileType::Source,
104+
headers: BTreeMap::new(),
105+
messages: vec![],
106+
already_uploaded: false,
107+
});
108+
}
109+
}
110+
}
111+
files
112+
}
113+
25114
/// Safe to exclude globally — can never be valid JVM package names.
26115
const SAFE_EXCLUDES: &[&str] = &[
27116
".cxx",
@@ -149,27 +238,10 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
149238
.extensions(JVM_EXTENSIONS.iter().copied())
150239
.ignores(all_excludes)
151240
.respect_ignores(true)
241+
.sort_entries(true)
152242
.collect_files()?;
153243

154-
let files = sources.into_iter().filter_map(|source| {
155-
let local_path = source.path.strip_prefix(&source.base_path).unwrap();
156-
if is_in_ambiguous_build_dir(local_path) {
157-
debug!("excluding (build output): {}", source.path.display());
158-
return None;
159-
}
160-
let local_path_jvm_ext = local_path.with_extension("jvm");
161-
let url = format!("~/{}", path_as_url(&local_path_jvm_ext));
162-
163-
Some(SourceFile {
164-
url,
165-
path: source.path,
166-
contents: Arc::new(source.contents),
167-
ty: SourceFileType::Source,
168-
headers: BTreeMap::new(),
169-
messages: vec![],
170-
already_uploaded: false,
171-
})
172-
});
244+
let files = build_source_files(sources);
173245

174246
let tempfile = source_bundle::build(context, files, Some(*debug_id))
175247
.context("Unable to create source bundle")?;
@@ -180,6 +252,57 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
180252
Ok(())
181253
}
182254

255+
#[cfg(test)]
256+
mod log_capture {
257+
use log::{Level, LevelFilter, Log, Metadata, Record};
258+
use std::cell::RefCell;
259+
use std::sync::Once;
260+
261+
thread_local! {
262+
static BUFFER: RefCell<Vec<(Level, String)>> = const { RefCell::new(Vec::new()) };
263+
}
264+
265+
struct CaptureLogger;
266+
267+
impl Log for CaptureLogger {
268+
fn enabled(&self, _: &Metadata) -> bool {
269+
true
270+
}
271+
272+
fn log(&self, record: &Record) {
273+
BUFFER.with(|b| {
274+
b.borrow_mut()
275+
.push((record.level(), record.args().to_string()))
276+
});
277+
}
278+
279+
fn flush(&self) {}
280+
}
281+
282+
static LOGGER: CaptureLogger = CaptureLogger;
283+
284+
/// Installs the capture logger (once per process) and clears this
285+
/// thread's buffer so a test starts from a clean slate.
286+
pub fn setup() {
287+
static ONCE: Once = Once::new();
288+
ONCE.call_once(|| {
289+
let _ = log::set_logger(&LOGGER);
290+
log::set_max_level(LevelFilter::Trace);
291+
});
292+
BUFFER.with(|b| b.borrow_mut().clear());
293+
}
294+
295+
pub fn warnings() -> Vec<String> {
296+
BUFFER.with(|b| {
297+
b.borrow()
298+
.iter()
299+
.filter(|(lvl, _)| *lvl == Level::Warn)
300+
.map(|(_, msg)| msg.clone())
301+
.collect()
302+
})
303+
}
304+
}
305+
183306
#[cfg(test)]
184307
mod tests {
185308
use super::*;
@@ -237,6 +360,113 @@ mod tests {
237360
)));
238361
}
239362

363+
#[test]
364+
fn test_strip_source_set_prefix_drops_module_and_source_set() {
365+
assert_eq!(
366+
strip_source_set_prefix(Path::new(
367+
"sentry-android-core/src/main/java/io/sentry/android/core/ANRWatchDog.java"
368+
)),
369+
Path::new("io/sentry/android/core/ANRWatchDog.java")
370+
);
371+
assert_eq!(
372+
strip_source_set_prefix(Path::new("src/main/kotlin/com/example/Foo.kt")),
373+
Path::new("com/example/Foo.kt")
374+
);
375+
}
376+
377+
#[test]
378+
fn test_strip_source_set_prefix_kt_under_java_source_set() {
379+
// Mixed Java/Kotlin projects commonly place .kt files under src/main/java/
380+
// — stripping is driven by the directory name, not the file extension.
381+
assert_eq!(
382+
strip_source_set_prefix(Path::new("src/main/java/com/example/Foo.kt")),
383+
Path::new("com/example/Foo.kt")
384+
);
385+
assert_eq!(
386+
strip_source_set_prefix(Path::new(
387+
"app/src/main/java/io/sentry/android/core/ANRWatchDog.kt"
388+
)),
389+
Path::new("io/sentry/android/core/ANRWatchDog.kt")
390+
);
391+
}
392+
393+
#[test]
394+
fn test_strip_source_set_prefix_handles_nested_modules() {
395+
assert_eq!(
396+
strip_source_set_prefix(Path::new(
397+
"sentry-opentelemetry/sentry-opentelemetry-agent/src/main/java/io/sentry/opentelemetry/agent/Foo.java"
398+
)),
399+
Path::new("io/sentry/opentelemetry/agent/Foo.java")
400+
);
401+
}
402+
403+
#[test]
404+
fn test_strip_source_set_prefix_handles_android_variants() {
405+
assert_eq!(
406+
strip_source_set_prefix(Path::new("app/src/debug/java/com/example/Foo.java")),
407+
Path::new("com/example/Foo.java")
408+
);
409+
assert_eq!(
410+
strip_source_set_prefix(Path::new("lib/src/release/kotlin/com/example/Bar.kt")),
411+
Path::new("com/example/Bar.kt")
412+
);
413+
}
414+
415+
#[test]
416+
fn test_strip_source_set_prefix_supports_scala_and_groovy() {
417+
assert_eq!(
418+
strip_source_set_prefix(Path::new("mod/src/main/scala/com/example/Foo.scala")),
419+
Path::new("com/example/Foo.scala")
420+
);
421+
assert_eq!(
422+
strip_source_set_prefix(Path::new("mod/src/main/groovy/com/example/Foo.groovy")),
423+
Path::new("com/example/Foo.groovy")
424+
);
425+
}
426+
427+
#[test]
428+
fn test_strip_source_set_prefix_supports_clojure() {
429+
assert_eq!(
430+
strip_source_set_prefix(Path::new("mod/src/main/clojure/com/example/foo.clj")),
431+
Path::new("com/example/foo.clj")
432+
);
433+
assert_eq!(
434+
strip_source_set_prefix(Path::new("mod/src/main/clojure/com/example/foo.cljc")),
435+
Path::new("com/example/foo.cljc")
436+
);
437+
}
438+
439+
#[test]
440+
fn test_strip_source_set_prefix_handles_default_package() {
441+
assert_eq!(
442+
strip_source_set_prefix(Path::new("src/main/java/NoPackage.java")),
443+
Path::new("NoPackage.java")
444+
);
445+
}
446+
447+
#[test]
448+
fn test_strip_source_set_prefix_falls_back_when_no_match() {
449+
// No `src/<sourceset>/<lang>/` triplet — path is returned unchanged.
450+
assert_eq!(
451+
strip_source_set_prefix(Path::new("sources/com/example/Foo.java")),
452+
Path::new("sources/com/example/Foo.java")
453+
);
454+
assert_eq!(
455+
strip_source_set_prefix(Path::new("Foo.java")),
456+
Path::new("Foo.java")
457+
);
458+
}
459+
460+
#[test]
461+
fn test_strip_source_set_prefix_does_not_match_package_named_like_lang() {
462+
// `kotlin` as a package name (under `src/main/java/`) must not be
463+
// mistaken for the source-set language dir.
464+
assert_eq!(
465+
strip_source_set_prefix(Path::new("src/main/java/com/example/kotlin/Foo.java")),
466+
Path::new("com/example/kotlin/Foo.java")
467+
);
468+
}
469+
240470
#[test]
241471
fn test_keeps_files_without_ambiguous_dirs() {
242472
assert!(!is_in_ambiguous_build_dir(Path::new(
@@ -247,4 +477,59 @@ mod tests {
247477
"app/src/main/java/Foo.java"
248478
)));
249479
}
480+
481+
fn fake_source(base: &str, relative: &str) -> ReleaseFileMatch {
482+
ReleaseFileMatch {
483+
base_path: PathBuf::from(base),
484+
path: PathBuf::from(base).join(relative),
485+
contents: Vec::new(),
486+
}
487+
}
488+
489+
#[test]
490+
fn test_build_source_files_warns_on_collision_for_android_variants() {
491+
// Sources arrive pre-sorted from `ReleaseFileSearch` (which configures
492+
// `WalkBuilder::sort_by_file_name`); the first-seen wins in the dedup.
493+
log_capture::setup();
494+
495+
let debug_src = fake_source("/app", "src/debug/java/com/example/Config.java");
496+
let main_src = fake_source("/app", "src/main/java/com/example/Config.java");
497+
let kept_display = debug_src.path.display().to_string();
498+
let skipped_display = main_src.path.display().to_string();
499+
500+
let files = build_source_files(vec![debug_src, main_src]);
501+
502+
assert_eq!(files.len(), 1);
503+
assert_eq!(files[0].url, "~/com/example/Config.jvm");
504+
assert_eq!(files[0].path.display().to_string(), kept_display);
505+
506+
let warnings = log_capture::warnings();
507+
assert_eq!(warnings.len(), 1);
508+
let msg = &warnings[0];
509+
assert!(
510+
msg.contains("URL collision on ~/com/example/Config.jvm"),
511+
"{msg}"
512+
);
513+
assert!(
514+
msg.contains(&skipped_display),
515+
"missing skipped path '{skipped_display}' in: {msg}"
516+
);
517+
assert!(
518+
msg.contains(&kept_display),
519+
"missing kept path '{kept_display}' in: {msg}"
520+
);
521+
}
522+
523+
#[test]
524+
fn test_build_source_files_keeps_distinct_urls() {
525+
log_capture::setup();
526+
527+
let sources = vec![
528+
fake_source("/app", "src/main/java/com/example/Foo.java"),
529+
fake_source("/app", "src/main/java/com/example/Bar.java"),
530+
];
531+
let files = build_source_files(sources);
532+
assert_eq!(files.len(), 2);
533+
assert!(log_capture::warnings().is_empty());
534+
}
250535
}

0 commit comments

Comments
 (0)