pantsbuild / pants

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

Native options discrepancy warning #21091

Open bolmstedt opened 2 weeks ago

bolmstedt commented 2 weeks ago

Describe the bug When extending a list, items are added twice in the native value, but not in the legacy value.

This can be reproduced by creating the following files:

# pants.toml
[GLOBAL]
pants_version = "2.23.0.dev1"
backend_packages = ["pants.backend.docker"]
# .pants.rc
[GLOBAL]
backend_packages = "+['pants.backend.docker.lint.hadolint']"

And then running pants version to produce:

15:31:12.47 [WARN] Value mismatch for the option `backend_packages` in scope []:
Legacy value: ['pants.backend.docker', 'pants.backend.docker.lint.hadolint'] of type <class 'list'>, from source CONFIG
Native value: ['pants.backend.docker', 'pants.backend.docker.lint.hadolint', 'pants.backend.docker.lint.hadolint'] of type <class 'list'>, from source CONFIG
...

Pants version 2.23.0.dev1

OS MacOS

lilatomic commented 1 week ago

confirming reproduction. Digging into it, we process all items in .pants.rc twice. Need to do more digging to find out why. (with replace actions (everything except for add or remove) this doesn't have an effect.)

lilatomic commented 1 week ago

In OptionParser.new we skip config discovery if we've been passed some discovered configs https://github.com/pantsbuild/pants/blob/dc0aa0a6444618435c60f8807ab89997885f5910/src/rust/engine/options/src/lib.rs#L326-L328

But we don't skip reading from the .pants.rc file https://github.com/pantsbuild/pants/blob/dc0aa0a6444618435c60f8807ab89997885f5910/src/rust/engine/options/src/lib.rs#L382

When starting up initially, we don't pass any discovered configs. But on the Python side, when we're starting up the rust OptionParser to compare, we do pass in the already discovered configs https://github.com/pantsbuild/pants/blob/a3eaf7007fcb2d878edd9f08d922799289567c25/src/python/pants/option/options.py#L166

I tried just jamming that to None to get the rust parser to discover configs. It didn't explode, but I'm not sure if that's the right decision lol

benjyw commented 1 week ago

Hmm yes, the Rust parser does its own config discovery, so we should pass it no configs I think!

lilatomic commented 4 days ago

Ah, seems passing in the configs is necessary for integration tests. They do some stuff to set the OptionsBootstrapper. That makes sense. I think then that the solution is to not load the rc if we've been passed configs, and then only pass configs for the OptionsBootstrapper (so tests work). I imagine that we can simplify that once we're using the rust parser only.