Inline multiple config files into options sources. #20549

benjyw · 2024-02-14T23:15:13Z

Previously the options sources list could have only one source
per type (flag/env/config/default). Since we support multiple
config files, we had the concept of a "merged config" that
encompassed multiple "config sources". This led to
duplication of the logic that traverses and merges sources.

This change gets rid of the concept of merged config.
Instead, the list of options sources we consult can now
include multiple config files. This is achieved by enhancing
the Source::Config enum to include an ordinal (to ensure
that configs are applied in the right order) and the path to
the config file (purely informational).

The natural #[derive(Ord)] will order these augmented
configs appropriately within all the sources. To make this
easier to achieve we change the order of the enum to be
from lowest to highest priority, so that we can add config
files naturally in the order in which we encounter them.

This also allows us to simplify some types. In particular,
we no longer need to traffic in Vec<DictEdit> - a single
source can only return a single DictEdit.

More importantly, this supports derivation history that
includes the specific config file each value comes from.

benjyw · 2024-02-14T23:16:09Z

src/rust/engine/options/src/config.rs

@@ -216,133 +215,16 @@ fn toml_table_to_dict(table: &Value) -> HashMap<String, Val> {
    }
 }

-#[derive(Clone)]
-struct ConfigSource {


We get rid of ConfigSource entirely, and fold its functionality directly into Config below, since they are now one-to-one.

benjyw · 2024-02-14T23:17:04Z

src/rust/engine/options/src/config.rs

        })
    }

+    fn option_name(id: &OptionId) -> String {


Moved from ConfigSource

benjyw · 2024-02-14T23:17:32Z

src/rust/engine/options/src/config.rs

-        let mut edits: Vec<ListEdit<T>> = vec![];
-        for source in self.sources.iter() {
-            edits.append(&mut source.get_list(id, parse_list)?);
+        let mut list_edits = vec![];


Moved from ConfigSource

benjyw · 2024-02-14T23:20:50Z

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

@@ -55,49 +48,6 @@ fn test_display() {
    );
 }

-#[test]
-fn test_multiple_sources() {


Multiple configs is no longer a thing to be tested separately within config_tests.rs, it is amply tested along with multiple sources in general, in tests.rs.

benjyw · 2024-02-14T23:21:22Z

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

@@ -185,16 +137,25 @@ fn test_interpolate_config() {
            .unwrap()
    );

-    // TODO: Uncomment when we implement get_dict.


Should have done this before. It needed a little massaging after uncommenting.

benjyw · 2024-02-14T23:22:18Z

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

 }

-#[derive(Copy, Clone, Debug, Ord, PartialOrd, Eq, PartialEq)]
+#[derive(Clone, Debug, Ord, PartialOrd, Eq, PartialEq)]


Ord will do the right thing here: order by increasing declaration order, and within Config, lexicographically by field, hence ordinal.

I didn't notice a test for the ord behaviour; have I missed it and/or is it worth adding one?

It falls out of the documentation for the generated Ord, but can't hurt to add one, done.

I was thinking the test would be catching if someone jumps in and changes the enum (eg sorts the variants alphabetically) they may not realise that the variant and field order is specifically meaningful, rather than any order being fine (and just being necessary to allow use in BTreeMap or similar).

Yep, a test is a good idea, added one.

benjyw · 2024-02-14T23:23:41Z

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

+            }
+        }
+
+        fn path_strip(prefix: &str, path: &str) -> String {


For paths under the buildroot, it seemed more useful to put the relative path to the buildroot in the derivation, rather than the full path. It also makes the tests simpler.

Note that we implement our own simply path manipulation here because we have Strings in hand, not OsString or Path.

One can construct paths from strs cheaply: https://doc.rust-lang.org/stable/std/path/struct.Path.html#method.new

Did you consider that as an option?

The problem is going back to a String from the underlying OsString, but for now I've done it with an aggressive unwrap(), and in the future the calling code should really be using OsString or Path anyway.

benjyw · 2024-02-14T23:24:55Z

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

                if let Some(val) = getter(source, id)? {
-                    derivations.push((*source_type, val));
+                    derivations.push((source_type.clone(), val));


Right now the derivations will contain many hundreds of copies of the string pants.toml or whatever. In a followup I might change this to a &str with a lifetime tied to the OptionsParser, so we can re-use references.

If the lifetime is too annoying/constraining, an Rc<String> or Rc<str> (or Arc) might be options to consider too.

Would have to be Arc because these get passed into Python code, but yeah, not a bad solution.

benjyw · 2024-02-14T23:26:04Z

It's a non-trivial number of diff lines, but a big chunk of that is moving code from ConfigSource directly into Config, so it's less intimidating than it looks.

huonw

Cool. Seems to be a nice conceptual simplification!

huonw · 2024-02-15T18:13:17Z

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

+
+        fn path_strip(prefix: &str, path: &str) -> String {
+            match path.strip_prefix(prefix) {
+                Some(suffix) => match suffix.strip_prefix(path::MAIN_SEPARATOR) {


Just confirming that this separator stripping should only apply if the prefix was removed? (That is, if the prefix remains, a trailing / should too?)

Correct, to ensure that the stripped dir is a relpath.

huonw · 2024-02-15T18:15:06Z

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

+        }
+
+        fn path_strip(prefix: &str, path: &str) -> String {
+            match path.strip_prefix(prefix) {


It looks like this would strip off a prefix in the middle of a directory, like path = /abcdef, prefix = /abc => def. Is that desirable?

Hmm, yeah, that is not good.

I think I will switch to using Path utils.

Switched to using Path.

huonw · 2024-02-15T18:15:40Z

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

+            }
+        }
+
+        fn path_strip(prefix: &str, path: &str) -> String {


One can construct paths from strs cheaply: https://doc.rust-lang.org/stable/std/path/struct.Path.html#method.new

Did you consider that as an option?

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

huonw · 2024-02-15T18:21:35Z

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

                if let Some(val) = getter(source, id)? {
-                    derivations.push((*source_type, val));
+                    derivations.push((source_type.clone(), val));


If the lifetime is too annoying/constraining, an Rc<String> or Rc<str> (or Arc) might be options to consider too.

huonw · 2024-02-15T18:24:13Z

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

 }

-#[derive(Copy, Clone, Debug, Ord, PartialOrd, Eq, PartialEq)]
+#[derive(Clone, Debug, Ord, PartialOrd, Eq, PartialEq)]


I didn't notice a test for the ord behaviour; have I missed it and/or is it worth adding one?

huonw

Great

Inline multiple config files into options sources.

e2cf754

benjyw added the category:internal CI, fixes for not-yet-released features, etc. label Feb 14, 2024

benjyw requested review from tgolsson and huonw February 14, 2024 23:15

benjyw commented Feb 14, 2024

View reviewed changes

src/rust/engine/options/src/config.rs

})

}

fn option_name(id: &OptionId) -> String {

Copy link

Contributor Author

benjyw Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved from ConfigSource

benjyw commented Feb 14, 2024

View reviewed changes

More simplification

02bd9ed

benjyw commented Feb 14, 2024

View reviewed changes

benjyw added 2 commits February 14, 2024 15:26

Merge branch 'main' into option_sources

7c4af56

Clippy

8683a37

huonw reviewed Feb 15, 2024

View reviewed changes

benjyw added 3 commits February 15, 2024 15:08

Merge branch 'main' into option_sources

02adf44

Code review feedback

9d3bebb

Fix unwrap

5b6e3d3

benjyw requested a review from huonw February 16, 2024 02:21

huonw approved these changes Feb 16, 2024

View reviewed changes

benjyw merged commit 3c940a9 into pantsbuild:main Feb 17, 2024
24 checks passed

benjyw deleted the option_sources branch February 17, 2024 06:54

purajit mentioned this pull request Jun 26, 2024

Bump default terraform version to 1.9.0, add all known terraform versions inbetween #21110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline multiple config files into options sources. #20549

Inline multiple config files into options sources. #20549

benjyw commented Feb 14, 2024 •

edited

Loading

benjyw Feb 14, 2024

benjyw Feb 14, 2024

benjyw Feb 14, 2024

benjyw Feb 14, 2024

benjyw Feb 14, 2024

benjyw Feb 14, 2024

huonw Feb 15, 2024

benjyw Feb 16, 2024

huonw Feb 16, 2024

benjyw Feb 16, 2024

benjyw Feb 14, 2024

huonw Feb 15, 2024

benjyw Feb 16, 2024

benjyw Feb 14, 2024

huonw Feb 15, 2024

benjyw Feb 16, 2024

benjyw commented Feb 14, 2024

huonw left a comment

huonw Feb 15, 2024

benjyw Feb 16, 2024

huonw Feb 15, 2024 •

edited

Loading

benjyw Feb 16, 2024

benjyw Feb 16, 2024

huonw Feb 15, 2024

huonw Feb 15, 2024

huonw Feb 15, 2024

huonw left a comment

Inline multiple config files into options sources. #20549

Inline multiple config files into options sources. #20549

Conversation

benjyw commented Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw commented Feb 14, 2024

huonw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw left a comment

Choose a reason for hiding this comment

benjyw commented Feb 14, 2024 •

edited

Loading

huonw Feb 15, 2024 •

edited

Loading