Resolve @fromfile paths relative to the buildroot. (#20820)

This is how the Python options parser works, so
now the Rust parser follows suit.

src/rust/engine/options/src/args_tests.rs

@@ -17,7 +17,7 @@ where
 {
     ArgsReader::new(
         Args::new(args.into_iter().map(|x| x.to_string())),
-        FromfileExpander::new(),
+        FromfileExpander::relative_to_cwd(),
     )
 }
 

src/rust/engine/options/src/build_root.rs

@@ -6,13 +6,19 @@ use std::ops::Deref;
 use std::path::{Path, PathBuf};
 
 use log::debug;
+use std::os::unix::ffi::OsStrExt;
 
-#[derive(Debug)]
+#[derive(Clone, Debug)]
 pub struct BuildRoot(PathBuf);
 
 impl BuildRoot {
     const SENTINEL_FILES: &'static [&'static str] = &["pants.toml", "BUILDROOT", "BUILD_ROOT"];
 
+    // Useful in tests.
+    pub fn for_path(path: PathBuf) -> Self {
+        Self(path)
+    }
+
     pub fn find() -> Result<BuildRoot, String> {
         match env::var_os("PANTS_BUILDROOT_OVERRIDE") {
             Some(buildroot) if !buildroot.is_empty() => Ok(BuildRoot(PathBuf::from(buildroot))),
@@ -59,6 +65,16 @@ impl BuildRoot {
             ))?;
         }
     }
+
+    pub fn convert_to_string(&self) -> Result<String, String> {
+        String::from_utf8(self.0.as_os_str().as_bytes().to_vec()).map_err(|e| {
+            format!(
+                "Failed to decode build root path {}: {}",
+                self.0.display(),
+                e
+            )
+        })
+    }
 }
 
 impl Deref for BuildRoot {

src/rust/engine/options/src/config_tests.rs

@@ -32,7 +32,7 @@ fn maybe_config(file_content: &str) -> Result<ConfigReader, String> {
             ("seed2".to_string(), "seed2val".to_string()),
         ]),
     )
-    .map(|config| ConfigReader::new(config, FromfileExpander::new()))
+    .map(|config| ConfigReader::new(config, FromfileExpander::relative_to_cwd()))
 }
 
 fn config(file_content: &str) -> ConfigReader {

src/rust/engine/options/src/env_tests.rs

@@ -18,7 +18,7 @@ fn env<'a, I: IntoIterator<Item = (&'a str, &'a str)>>(vars: I) -> EnvReader {
                 .map(|(k, v)| (k.to_owned(), v.to_owned()))
                 .collect::<HashMap<_, _>>(),
         ),
-        FromfileExpander::new(),
+        FromfileExpander::relative_to_cwd(),
     )
 }
 

src/rust/engine/options/src/fromfile.rs

@@ -1,7 +1,7 @@
 // Copyright 2024 Pants project contributors (see CONTRIBUTORS.md).
 // Licensed under the Apache License, Version 2.0 (see LICENSE).
 
-use super::{DictEdit, DictEditAction, ListEdit, ListEditAction};
+use super::{BuildRoot, DictEdit, DictEditAction, ListEdit, ListEditAction};
 
 use crate::parse::{mk_parse_err, parse_dict, ParseError, Parseable};
 use log::warn;
@@ -51,11 +51,26 @@ fn try_deserialize<'a, DE: Deserialize<'a>>(
     }
 }
 
-pub struct FromfileExpander {}
+#[derive(Clone, Debug)]
+pub struct FromfileExpander {
+    build_root: BuildRoot,
+}
 
 impl FromfileExpander {
-    pub fn new() -> Self {
-        Self {}
+    // Creates a FromfileExpander that treats relpaths as relative to the build root.
+    pub fn relative_to(build_root: BuildRoot) -> Self {
+        Self {
+            build_root: build_root,
+        }
+    }
+
+    // Creates a FromfileExpander that treats relpaths as relative to the CWD.
+    // Useful in tests.
+    #[cfg(test)]
+    pub(crate) fn relative_to_cwd() -> Self {
+        Self {
+            build_root: BuildRoot::for_path(PathBuf::from("")),
+        }
     }
 
     fn maybe_expand(&self, value: String) -> Result<ExpandedValue, ParseError> {
@@ -67,7 +82,7 @@ impl FromfileExpander {
                 match suffix.strip_prefix('?') {
                     Some(subsuffix) => {
                         // @? means the path is allowed to not exist.
-                        let path = PathBuf::from(subsuffix);
+                        let path = self.build_root.join(subsuffix);
                         match fs::read_to_string(&path) {
                             Ok(content) => Ok((Some(path), Some(content))),
                             Err(err) if err.kind() == io::ErrorKind::NotFound => {
@@ -78,7 +93,7 @@ impl FromfileExpander {
                         }
                     }
                     _ => {
-                        let path = PathBuf::from(suffix);
+                        let path = self.build_root.join(suffix);
                         let content =
                             fs::read_to_string(&path).map_err(|e| mk_parse_err(e, &path))?;
                         Ok((Some(path), Some(content)))

src/rust/engine/options/src/fromfile_tests.rs

@@ -4,7 +4,7 @@
 use crate::fromfile::test_util::write_fromfile;
 use crate::fromfile::*;
 use crate::parse::{ParseError, Parseable};
-use crate::{DictEdit, DictEditAction, ListEdit, ListEditAction, Val};
+use crate::{BuildRoot, DictEdit, DictEditAction, ListEdit, ListEditAction, Val};
 use maplit::hashmap;
 use std::collections::HashMap;
 use std::fmt::Debug;
@@ -22,15 +22,15 @@ macro_rules! check_err {
 }
 
 fn expand(value: String) -> Result<Option<String>, ParseError> {
-    FromfileExpander::new().expand(value)
+    FromfileExpander::relative_to_cwd().expand(value)
 }
 
 fn expand_to_list<T: Parseable>(value: String) -> Result<Option<Vec<ListEdit<T>>>, ParseError> {
-    FromfileExpander::new().expand_to_list(value)
+    FromfileExpander::relative_to_cwd().expand_to_list(value)
 }
 
 fn expand_to_dict(value: String) -> Result<Option<Vec<DictEdit>>, ParseError> {
-    FromfileExpander::new().expand_to_dict(value)
+    FromfileExpander::relative_to_cwd().expand_to_dict(value)
 }
 
 #[test]
@@ -52,6 +52,19 @@ fn test_expand_fromfile() {
         .starts_with("Problem reading /does/not/exist for XXX: No such file or directory"))
 }
 
+#[test]
+fn test_fromfile_relative_to_buildroot() {
+    let (_tmpdir, fromfile_pathbuf) = write_fromfile("fromfile.txt", "FOO");
+    let fromfile_relpath_pathbuf = fromfile_pathbuf.strip_prefix(&_tmpdir).unwrap();
+    assert!(fromfile_relpath_pathbuf.is_relative());
+    let fromfile_relpath_str = format!("{}", fromfile_relpath_pathbuf.display());
+    assert_eq!(
+        Ok(Some("FOO".to_string())),
+        FromfileExpander::relative_to(BuildRoot::for_path(_tmpdir.into_path()))
+            .expand(format!("@{}", fromfile_relpath_str))
+    );
+}
+
 #[test]
 fn test_expand_fromfile_to_list() {
     fn expand_fromfile<T: Parseable + Clone + Debug + PartialEq>(

src/rust/engine/options/src/lib.rs

@@ -37,7 +37,6 @@ mod types;
 use std::collections::{BTreeMap, HashMap, HashSet};
 use std::fmt::Debug;
 use std::hash::Hash;
-use std::os::unix::ffi::OsStrExt;
 use std::path::Path;
 use std::rc::Rc;
 
@@ -257,14 +256,8 @@ impl OptionParser {
         buildroot: Option<BuildRoot>,
     ) -> Result<OptionParser, String> {
         let buildroot = buildroot.unwrap_or(BuildRoot::find()?);
-        let buildroot_string = String::from_utf8(buildroot.as_os_str().as_bytes().to_vec())
-            .map_err(|e| {
-                format!(
-                    "Failed to decode build root path {}: {}",
-                    buildroot.display(),
-                    e
-                )
-            })?;
+        let buildroot_string = buildroot.convert_to_string()?;
+        let fromfile_expander = FromfileExpander::relative_to(buildroot);
 
         let mut seed_values = HashMap::from_iter(
             env.env
@@ -275,11 +268,11 @@ impl OptionParser {
         let mut sources: BTreeMap<Source, Rc<dyn OptionsSource>> = BTreeMap::new();
         sources.insert(
             Source::Env,
-            Rc::new(EnvReader::new(env, FromfileExpander::new())),
+            Rc::new(EnvReader::new(env, fromfile_expander.clone())),
         );
         sources.insert(
             Source::Flag,
-            Rc::new(ArgsReader::new(args, FromfileExpander::new())),
+            Rc::new(ArgsReader::new(args, fromfile_expander.clone())),
         );
         let mut parser = OptionParser {
             sources: sources.clone(),
@@ -342,7 +335,7 @@ impl OptionParser {
                     ordinal,
                     path: path_strip(&buildroot_string, path),
                 },
-                Rc::new(ConfigReader::new(config, FromfileExpander::new())),
+                Rc::new(ConfigReader::new(config, fromfile_expander.clone())),
             );
             ordinal += 1;
         }
@@ -371,7 +364,7 @@ impl OptionParser {
                             ordinal,
                             path: rcfile,
                         },
-                        Rc::new(ConfigReader::new(rc_config, FromfileExpander::new())),
+                        Rc::new(ConfigReader::new(rc_config, fromfile_expander.clone())),
                     );
                     ordinal += 1;
                 }