Skip to content

Commit 8ae6c83

Browse files
Fix Maven 4 implicit subprojects in vendor (#622) (#1807)
* Start fix for #622 Assisted-by: Claude Code:claude-opus-5-5 * Vendor Maven 4 implicit subprojects as a reactor Maven 4 builds every child directory holding a pom.xml of a 4.1.0 pom-packaged root that lists no <subprojects>. vendor planned such a root as a reactor of one, so the modules that use the patched library kept the base version and a warm ~/.m2 build used Central's unpatched jar, while vendor, vendor --check and vex all reported success. The reactor walk now discovers those subprojects the way Maven 4 does (model version, pom packaging, the maven.project.discoverSubprojects opt-out), and every reactor entry point (plan, check, vex, revert, the not-build-root guard) gets the directory listing it needs. Fixes #622 Assisted-by: Claude Code:claude-opus-5-5 * Probe Maven 4 subprojects through the listing Discovery read each subdirectory's pom.xml to test that it exists. In a git checkout that read went through .git/, which the project reader refuses and records as a read error, so vex and vendor --check would fail on every Maven 4 project that relies on discovery. Test for the pom through the directory listing instead, which lists a refused directory as empty, and share the in-memory listing helper between the gradle and maven test modules. Assisted-by: Claude Code:claude-opus-5-5 * Skip outbound dir links in Maven 4 discovery Discovery lists every child directory to see whether it holds a pom.xml. A directory symlinked outside the checkout, such as a target/ on another disk, was recorded as an escape on that listing alone, so vendor refused an otherwise valid Maven 4 project. The project reader now only peeks at such a directory (files, no subdirectories) without recording the escape. A pom found there still becomes a module, and reading it still refuses the build as one that leaves the checkout. Assisted-by: Claude Code:claude-opus-5-5 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent dad6408 commit 8ae6c83

7 files changed

Lines changed: 575 additions & 75 deletions

File tree

‎crates/socket-patch-cli/tests/e2e_vendor_jvm_build.rs‎

Lines changed: 165 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -575,6 +575,171 @@ fn maven_reactor_vendor_fresh_checkout_offline_build_and_byte_exact_revert() {
575575
);
576576
}
577577

578+
/// A Maven 4 (model 4.1.0) aggregator that lists no subprojects: Maven 4
579+
/// discovers `a` and `b` itself.
580+
const IMPLICIT_ROOT_POM: &str = r#"<?xml version="1.0" encoding="UTF-8"?>
581+
<project xmlns="http://maven.apache.org/POM/4.1.0" root="true">
582+
<modelVersion>4.1.0</modelVersion>
583+
<groupId>com.example</groupId>
584+
<artifactId>root</artifactId>
585+
<version>1.0.0</version>
586+
<packaging>pom</packaging>
587+
</project>
588+
"#;
589+
590+
const IMPLICIT_A_POM: &str = r#"<?xml version="1.0" encoding="UTF-8"?>
591+
<project xmlns="http://maven.apache.org/POM/4.1.0">
592+
<modelVersion>4.1.0</modelVersion>
593+
<parent/>
594+
<artifactId>a</artifactId>
595+
<dependencies>
596+
<dependency>
597+
<groupId>org.apache.commons</groupId>
598+
<artifactId>commons-text</artifactId>
599+
<version>1.10.0</version>
600+
</dependency>
601+
</dependencies>
602+
</project>
603+
"#;
604+
605+
const IMPLICIT_B_POM: &str = r#"<?xml version="1.0" encoding="UTF-8"?>
606+
<project xmlns="http://maven.apache.org/POM/4.1.0">
607+
<modelVersion>4.1.0</modelVersion>
608+
<parent/>
609+
<artifactId>b</artifactId>
610+
<dependencies>
611+
<dependency>
612+
<groupId>com.example</groupId>
613+
<artifactId>a</artifactId>
614+
<version>${project.version}</version>
615+
</dependency>
616+
</dependencies>
617+
</project>
618+
"#;
619+
620+
/// #622: Maven 4 builds `a` and `b` of a 4.1.0 root that lists no
621+
/// `<subprojects>`. Vendoring must plan them as the reactor (suffixed
622+
/// version in `a`, the root pinned), so the developer's own warm `~/.m2`
623+
/// build, which already holds Central's 1.10.0, resolves the patched jar,
624+
/// and so does a fresh offline checkout. Before the fix the root was a
625+
/// reactor of one: `a` kept the base version and built Central's jar.
626+
#[test]
627+
#[ignore = "real Maven + Maven Central (fixture); run with --ignored"]
628+
fn maven_reactor_implicit_subprojects_warm_and_offline_build_and_revert() {
629+
const SUITE: &str = "e2e_vendor_jvm_build::maven_reactor_implicit_subprojects";
630+
let Some(mvn) = Mvn::detect(SUITE) else {
631+
return;
632+
};
633+
if mvn.major() < 4 {
634+
println!(
635+
"{SUITE}: SKIP (Maven {} has no subproject discovery)",
636+
mvn.version
637+
);
638+
return;
639+
}
640+
let tmp = tempfile::tempdir().unwrap();
641+
let root = fixture_root(&tmp);
642+
let m2 = root.join("m2");
643+
let proj = root.join("proj");
644+
let settings = root.join("settings.xml");
645+
write_settings(&settings, &[]);
646+
let Some((jar, _)) = warm_fixture(SUITE, &mvn, &root.join("warm"), &m2, &settings) else {
647+
return;
648+
};
649+
for (rel, body) in [
650+
("pom.xml", IMPLICIT_ROOT_POM),
651+
("a/pom.xml", IMPLICIT_A_POM),
652+
("b/pom.xml", IMPLICIT_B_POM),
653+
] {
654+
let path = proj.join(rel);
655+
std::fs::create_dir_all(path.parent().unwrap()).unwrap();
656+
std::fs::write(path, body).unwrap();
657+
}
658+
let out = with_central_fallback(SUITE, "pre-vendor reactor build", &settings, |s, flags| {
659+
mvn_classpath(&mvn, &proj, &m2, s, flags)
660+
});
661+
assert!(ok(&out), "pre-vendor reactor build:\n{}", dump(&out));
662+
assert_eq!(
663+
std::fs::read(classpath_entry(&proj.join("b"))).unwrap(),
664+
jar,
665+
"pre-vendor `b` resolves Central's jar through the discovered `a`"
666+
);
667+
for dir in ["target", "a/target", "b/target", ".mvn/target"] {
668+
let _ = std::fs::remove_dir_all(proj.join(dir));
669+
}
670+
let _ = std::fs::remove_dir(proj.join(".mvn"));
671+
672+
let (orig, patched) = patched_member(&jar, UUID);
673+
stage_manifest(&proj, &orig, &patched);
674+
let before = snapshot(&proj);
675+
let env = vendor(&proj, &m2);
676+
assert_eq!(env["summary"]["applied"], 1, "{env}");
677+
let vendored = snapshot(&proj);
678+
assert_eq!(
679+
text(&vendored["a/pom.xml"]),
680+
IMPLICIT_A_POM.replace(
681+
"<version>1.10.0</version>",
682+
&format!("<version>{SV}</version>")
683+
),
684+
"the discovered module's declaration is suffixed"
685+
);
686+
assert_eq!(text(&vendored["b/pom.xml"]), IMPLICIT_B_POM);
687+
assert!(
688+
text(&vendored["pom.xml"]).contains(&format!("<version>{SV}</version>")),
689+
"the root is pinned"
690+
);
691+
692+
// The developer's own tree: `~/.m2` still holds Central's 1.10.0.
693+
let out = mvn_classpath(&mvn, &proj, &m2, &settings, &["-o"]);
694+
assert!(ok(&out), "warm offline reactor build:\n{}", dump(&out));
695+
assert_vendored_on_classpath(&proj.join("a"), &patched, "warm build, module a");
696+
assert_vendored_on_classpath(&proj.join("b"), &patched, "warm build, module b");
697+
for dir in ["target", "a/target", "b/target", ".mvn/target"] {
698+
let _ = std::fs::remove_dir_all(proj.join(dir));
699+
}
700+
701+
let (code, doc, stderr) = socket_in(
702+
&proj,
703+
&m2,
704+
None,
705+
&[
706+
"vex",
707+
"--cwd",
708+
proj.to_str().unwrap(),
709+
"--product",
710+
"pkg:maven/com.example/root@1.0.0",
711+
],
712+
);
713+
assert_eq!(code, Some(0), "vex: {doc}\n{stderr}");
714+
assert_eq!(doc["statements"][0]["status"], "not_affected", "{doc}");
715+
let (code, env, stderr) = socket_in(
716+
&proj,
717+
&m2,
718+
None,
719+
&[
720+
"vendor",
721+
"--check",
722+
"--json",
723+
"--offline",
724+
"--cwd",
725+
proj.to_str().unwrap(),
726+
],
727+
);
728+
assert_eq!(code, Some(0), "vendor --check: {env}\n{stderr}");
729+
730+
let fresh = root.join("fresh");
731+
fresh_checkout_all(&proj, &fresh);
732+
purge(&m2);
733+
let out = mvn_classpath(&mvn, &fresh, &m2, &settings, &["-o"]);
734+
assert!(ok(&out), "fresh offline reactor build:\n{}", dump(&out));
735+
assert_vendored_on_classpath(&fresh.join("a"), &patched, "fresh build, module a");
736+
assert_vendored_on_classpath(&fresh.join("b"), &patched, "fresh build, module b");
737+
738+
revert(&proj, &m2);
739+
assert_restored(&proj, &before);
740+
assert!(!proj.join(".mvn").exists(), ".mvn residue");
741+
}
742+
578743
/// #655: `a` declares the library through `${ct.group}:${ct.artifact}`,
579744
/// properties the local corp parent defines. Maven interpolates them before
580745
/// matching, so vendor must rewrite `a`'s literal version too, or `a` builds

‎crates/socket-patch-core/src/vendor/jvm/apply.rs‎

Lines changed: 53 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ use super::super::state::{VendorEntry, WiringAction, WiringRecord};
3030
use super::super::{RevertOpts, RevertOutcome, VendorWarning};
3131
use super::{
3232
coursier_tree, gradle, layout, maven_reactor, op_of, op_str, sbt, scala_cli, sha256_hex,
33-
Coords, JvmPlan, JvmUnplan, ReadFn, Shape, CONFIG_LINE_KIND, COURSIER_INDEX_KIND,
33+
Coords, JvmPlan, JvmUnplan, ListFn, ReadFn, Shape, CONFIG_LINE_KIND, COURSIER_INDEX_KIND,
3434
CREATED_DIR_KIND, DERIVED_METADATA_KIND, KINDS, OWNED_FILE_KIND, POM_FRAGMENT_KIND,
3535
SBT_FRAGMENT_KIND, SETTINGS_FRAGMENT_KIND, TREE_KIND, VERIFICATION_FRAGMENT_KIND,
3636
};
@@ -198,15 +198,31 @@ impl ProjectReader {
198198
/// The children of the project directory `rel` (`""` = the root) for
199199
/// the script graph: names, directories ending in `/`. Resolved like
200200
/// [`Self::read`]; anything unsafe or unreadable lists as empty.
201+
///
202+
/// A directory reached through a symlink that leaves the checkout is
203+
/// only peeked at: its files are listed, its subdirectories are not,
204+
/// and no escape is recorded. A probe (Maven 4 subproject discovery
205+
/// asking whether a linked `target/` holds a `pom.xml`) then refuses
206+
/// nothing for a link it never reads through, while reading any file
207+
/// under the link still records the escape the caller refuses.
201208
pub fn list(&self, rel: &str) -> Vec<String> {
202-
let path = if rel.is_empty() {
209+
let (path, outside) = if rel.is_empty() {
203210
match &self.canonical {
204-
Some(c) => c.clone(),
211+
Some(c) => (c.clone(), false),
205212
None => return Vec::new(),
206213
}
207214
} else {
208-
match self.resolve(rel.trim_end_matches('/')) {
209-
Ok(path) => path,
215+
let rel = rel.trim_end_matches('/');
216+
let before = self.escaped.borrow().clone();
217+
match self.resolve(rel) {
218+
Ok(path) => (path, false),
219+
Err(e) if e.contains(LEAVES_PROJECT) => {
220+
*self.escaped.borrow_mut() = before;
221+
match self.outside_target(rel) {
222+
Some(target) => (target, true),
223+
None => return Vec::new(),
224+
}
225+
}
210226
Err(_) => return Vec::new(),
211227
}
212228
};
@@ -219,13 +235,28 @@ impl ProjectReader {
219235
// Followed like `read` (in-checkout links only).
220236
let dir = e.metadata().ok()?.is_dir()
221237
|| (e.file_type().ok()?.is_symlink() && e.path().is_dir());
222-
Some(if dir { format!("{name}/") } else { name })
238+
match (dir, outside) {
239+
(true, true) => None,
240+
(true, false) => Some(format!("{name}/")),
241+
(false, _) => Some(name),
242+
}
223243
})
224244
.collect();
225245
out.sort();
226246
out
227247
}
228248

249+
/// The directory a project-relative `rel` names once its symlinks are
250+
/// followed wherever they lead (for [`Self::list`]'s peek). `None` for
251+
/// an unsafe path or one that does not resolve to a directory.
252+
fn outside_target(&self, rel: &str) -> Option<PathBuf> {
253+
if !safe_rel(rel) {
254+
return None;
255+
}
256+
let target = std::fs::canonicalize(self.canonical.as_ref()?.join(rel)).ok()?;
257+
target.is_dir().then_some(target)
258+
}
259+
229260
/// The first path that resolved outside the checkout.
230261
pub fn escaped(&self) -> Option<String> {
231262
self.escaped.borrow().clone()
@@ -269,7 +300,7 @@ impl ProjectReader {
269300
let at = segments[..=i].join("/");
270301
self.escaped.borrow_mut().get_or_insert(at.clone());
271302
return Err(format!(
272-
"{at} is a symlink that leaves the project (or dangles)"
303+
"{at} is a symlink that {LEAVES_PROJECT} (or dangles)"
273304
));
274305
};
275306
cur = target;
@@ -281,6 +312,9 @@ impl ProjectReader {
281312
}
282313
}
283314

315+
/// How [`ProjectReader`] words a symlink resolving outside the checkout.
316+
const LEAVES_PROJECT: &str = "leaves the project";
317+
284318
/// A path that does not exist, including one that runs through a regular
285319
/// file (`pom.xml/pom.xml`, probed when a `relativePath` or `<module>`
286320
/// names a pom file rather than its directory, #534).
@@ -593,6 +627,7 @@ pub async fn revert(root: &Path, entry: &VendorEntry, opts: RevertOpts) -> Rever
593627
}
594628
let reader = ProjectReader::new(root);
595629
let read = |rel: &str| reader.read(rel);
630+
let list = |dir: &str| reader.list(dir);
596631
let peers = match super::super::state::load_state(root).await {
597632
Ok(state) => state,
598633
Err(e) => return RevertOutcome::failed(format!("vendor_state_unreadable: {e}")),
@@ -625,7 +660,10 @@ pub async fn revert(root: &Path, entry: &VendorEntry, opts: RevertOpts) -> Rever
625660
unplan = merge_unplans(unplan, gradle::unplan(&read, &c, &records));
626661
}
627662
if maven {
628-
unplan = merge_unplans(unplan, maven_reactor::unplan(&read, &c, &entry.wiring));
663+
unplan = merge_unplans(
664+
unplan,
665+
maven_reactor::unplan(&read, &list, &c, &entry.wiring),
666+
);
629667
}
630668
unplan
631669
};
@@ -837,13 +875,14 @@ pub fn entry_references(root: &Path, entry: &VendorEntry) -> bool {
837875
};
838876
let reader = ProjectReader::new(root);
839877
let read = |rel: &str| reader.read(rel);
878+
let list = |dir: &str| reader.list(dir);
840879
let referenced = if sbt::owns(&entry.wiring) {
841880
sbt::wired_checked(&read, &c).unwrap_or(true)
842881
} else if scala_cli::owns(&entry.wiring) {
843882
scala_cli::wired_checked(&read, &c).unwrap_or(true)
844883
} else {
845884
let (maven, gradle) = sides(&entry.wiring);
846-
(maven && maven_reactor::wired_checked(&read, &c).unwrap_or(true))
885+
(maven && maven_reactor::wired_checked(&read, &list, &c).unwrap_or(true))
847886
|| (gradle && gradle::references_checked(&read, &c).unwrap_or(true))
848887
};
849888
// `read` answers `None` for an unreadable file as for a missing one;
@@ -878,7 +917,7 @@ pub fn entry_wired_checked(root: &Path, entry: &VendorEntry) -> Result<bool, Str
878917
wired = gradle::wired_checked(&read, &list, &c).map_err(|e| e.detail);
879918
}
880919
if maven && wired == Ok(true) {
881-
wired = maven_reactor::attestable_checked(&read, &c).map_err(|e| e.detail);
920+
wired = maven_reactor::attestable_checked(&read, &list, &c).map_err(|e| e.detail);
882921
}
883922
wired
884923
};
@@ -892,12 +931,13 @@ pub fn entry_wired_checked(root: &Path, entry: &VendorEntry) -> Result<bool, Str
892931
/// repository `local_repo` alone (`None` entries: not there).
893932
fn local_external_poms(
894933
read: ReadFn<'_>,
934+
list: ListFn<'_>,
895935
patch: &super::JvmPatch<'_>,
896936
local_repo: Option<&Path>,
897937
) -> maven_reactor::ExternalPoms {
898938
let mut known = maven_reactor::ExternalPoms::new();
899939
loop {
900-
let need = maven_reactor::external_poms_needed(read, patch, &known);
940+
let need = maven_reactor::external_poms_needed(read, list, patch, &known);
901941
if need.is_empty() {
902942
return known;
903943
}
@@ -1000,7 +1040,7 @@ pub fn check_entry(
10001040
// sync: the pin it wrote when it could read it, or none.
10011041
let shape = shape_of(&entry.wiring);
10021042
let external = if matches!(shape, Shape::MavenReactor | Shape::Mixed) {
1003-
Some(local_external_poms(&read, &patch, local_repo))
1043+
Some(local_external_poms(&read, &list, &patch, local_repo))
10041044
} else {
10051045
None
10061046
};
@@ -1020,7 +1060,7 @@ pub fn check_entry(
10201060
let referenced = if sbt_entry || scala_cli_entry {
10211061
entry_wired_checked(root, entry)?
10221062
} else {
1023-
(!maven || maven_reactor::wired_checked(&read, &c).map_err(|e| e.detail)?)
1063+
(!maven || maven_reactor::wired_checked(&read, &list, &c).map_err(|e| e.detail)?)
10241064
&& (!gradle || gradle::references(&read, &c))
10251065
};
10261066
if !referenced {

‎crates/socket-patch-core/src/vendor/jvm/gradle.rs‎

Lines changed: 1 addition & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2443,6 +2443,7 @@ fn line_start(text: &str, at: usize) -> usize {
24432443
mod tests {
24442444
use std::collections::BTreeMap;
24452445

2446+
use super::super::testing::list_of;
24462447
use super::super::{apply, detect, Shape};
24472448
use super::*;
24482449

@@ -2495,29 +2496,6 @@ mod tests {
24952496
plan(&read, &|dir: &str| list_of(files, dir), p)
24962497
}
24972498

2498-
/// The children of `dir` in an in-memory tree (the planner's ListFn).
2499-
fn list_of(files: &BTreeMap<String, Vec<u8>>, dir: &str) -> Vec<String> {
2500-
let prefix = if dir.is_empty() {
2501-
String::new()
2502-
} else {
2503-
format!("{}/", dir.trim_end_matches('/'))
2504-
};
2505-
let mut out: Vec<String> = Vec::new();
2506-
for key in files.keys() {
2507-
let Some(rest) = key.strip_prefix(&prefix) else {
2508-
continue;
2509-
};
2510-
let child = match rest.find('/') {
2511-
Some(i) => format!("{}/", &rest[..i]),
2512-
None => rest.to_string(),
2513-
};
2514-
if !out.contains(&child) {
2515-
out.push(child);
2516-
}
2517-
}
2518-
out
2519-
}
2520-
25212499
fn vendored_line(kotlin: bool, prefix: &str) -> String {
25222500
apply_line(&WiringTarget::vendored(), kotlin, prefix)
25232501
}

0 commit comments

Comments
 (0)