pants.ini [binaries] section no longer applies after move to bootstrap options

[kwlzn]

in #4846 I landed a change to move the binaries options to bootstrap options. this seems to have had the unintended side effect of making the [binaries] section no longer work in pants.ini:

ERROR] Invalid option 'path_by_id' under [binaries] in /Users/kwilson/dev/source/pants.ini
ERROR] Invalid option 'fetch_timeout_secs' under [binaries] in /Users/kwilson/dev/source/pants.ini
ERROR] Invalid option 'baseurls' under [binaries] in /Users/kwilson/dev/source/pants.ini
...

Exception message: Invalid config entries detected. See log for details on which entries to update or remove.
(Specify --no-verify-config to disable this check.)

flattening these as e.g.

[GLOBAL]
binaries_baseurls: ...
binaries_fetch_timeout_secs: ...
binaries_path_by_id: ...

works as expected tho.

it's not clear to me if this is a violation of the deprecation policy, but I'm also not sure how we can more cleanly migrate these options to bootstrap. posting here mainly for posterity but also to raise a discussion on the broader issue.

[stuhood]

One idea might be to add a deprecated_scope option to individual registered options. It's already supported for entire scopes/Optionables, so I don't suspect it would be too hard...?

pants/src/python/pants/option/options.py

Line 81 in b170aeb

if si.deprecated_scope:

[benjyw]

The cmd-line flags (and even env vars) still work, right?

I tried to naively see if binaries can be a deprecated scope for the global scope. But no - that leads to shadowing violations.

We used to have a script that "migrates" the config file. How about resuscitating that, and running it at pants startup during the deprecation cycle? It could error out but give a sensible message on how to fix.

Or - since this is a one-off kind of thing, we could hard-code some special-casing in the config parser...

[stuhood]

@benjyw : Is my suggestion of allowing the deprecated scope to be specified on individual options reasonable?

[kwlzn]

the migration script approach is interesting, but iirc that only provided suggestions and had to be manually run. I'll have to think more about that idea, but it'd be ideal to not have to ship a sidecar solution I think.

I'm currently leaning towards special casing a plural deprecated_options_scope on the GlobalOptionsRegistrar for cases where scopes have been subsumed by global options. (e.g. deprecated_options_scopes = ('binaries', 'pantsd', 'watchman').

still trying to figure out how best to wire that all up - let me know if you have any high level design thoughts.

[benjyw]

While considering this, I tried to add deprecated_options_scope = binaries to the global scope, to see if that would "just work", and then attempted to patch up all the problems as they popped up. I ended up disappearing down a rabbit hole of conflicts:

• If you set deprecated_options_scope on an enclosing scope of the deprecated scope, you end up with double-registration of recursive options, so you have to check for that, which is not on its own particularly complicated.

• However you also end up with shadowing errors on non-recursive options, and those seem harder to detect and handle appropriately.

Whether deprecated_options_scope is singular or plural has no bearing on the issues above.

However @stuhood's suggestion of allowing deprecated scopes on individual options might allow for solving these problems. Certainly it would then not be necessary for deprecated_options_scope to be plural.

[benjyw]

There are other complications. For example, we have to "complete" the set of known scopes before we register any options. Fortunately the 'binaries' scope still exists on the BinaryUtil.Factory subsystem, so it will get pulled in when we register the full options. But not when we register the bootstrap options. Currently those assume that the only scope that matters is the global scope.

[benjyw]

Yep, after trying to get this to work for a bit, my sad conclusion is that supporting non-global scopes during options bootstrapping is going to be quite disruptive, and I'm not sure it's worth it.

I honestly think that, since cmd-line flags and env vars will work, so the only problem is config files, that we special case this somehow (e.g., in the options parser) and not worry about the general case. Moving existing options into bootstrap options should hopefully be a very rare thing.

How about a wrapper around the config reader that does the transform implicitly?

[kwlzn]

sgtm

[kwlzn]

reviewable: #5007