search

pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.29k stars 628 forks source link

Fix options parser discrepancy due to repeated evaluation of `.pants.rc` #21118

Closed lilatomic closed 2 months ago

lilatomic commented 3 months ago

Allows config discovery in rust by not passing discovered config to rust-based options parser. (The exception is in bootstrapping, which is necessary to make integration tests work)

fixes #21091

lilatomic commented 2 months ago

For some integration tests, it's not loading from the --pants-config-files that's passed in to the runner. IDK yet if that's because it doesn't use it or because something causes that arg to not be passed.

lilatomic commented 2 months ago

I'm pretty sure this is happening during handle_unknown_flags. In that function, we replace the args (to remove the invalid one, and since we don't care about args when handling the unknown one). https://github.com/pantsbuild/pants/blob/1d1e93edcdf617c651c3eb1d1cbadd29d99172b2/src/python/pants/init/options_initializer.py#L185-L187 So config discovery doesn't have the hint and there isn't a pants.toml in the workdir.

Do we still need to pass the dummy flag to get suggestions?

benjyw commented 2 months ago

The "dummy first arg" is presumably to represent the process name, not a flag, no?

lilatomic commented 2 months ago

The "dummy first arg" is presumably to represent the process name, not a flag, no?

You're correct. I was trying to see if we could, somehow, not remove all the other args, so that the native options parser could still extract the config locations and do discover properly. I found the setting "allow_unknown_options" which seems like it's what we want?

Doing some more digging, this was achieved by some statefulness in the OptionBootstrapper. The configs are populated during the OptionBootstrapper.create function through the invocation of OptionBootstrapper.get_config_file_paths. When we are looking for suggestions for the unknown cli args (above), we only blank the args, and not the configs. This means that in the Python code, we effectively kept the discovered configs. But the Rust options parser is recreated when the Options is recreated. And we don't disable config discovery here and pass in the previously discovered config, so the Rust parser tries to discover configs but without the hints to the correct locations.

I'm not sure what we're planning to do going forward, and whether we'll pushdown the bootstrapping to the Rust parser. We could try to make it work with "allow_unknown_options", or trying to also forward the configs if we're searching for suggestions. Or we could implement something of the statefulness of OptionBootstrapper in the Rust parser.