Use `pants.toml` internally #9090

Eric-Arellano · 2020-02-07T21:56:53Z

This allows us to dogfood TOML support.

We make several changes to land this:

Fix when an option overrides a DEFAULT value.
Fix migrate_to_toml_config.py to work with :, e.g. src/python:target.
Add config.TomlSerializer to convert a parsed Python dictionary of option-scopes -> options into a TOML representation.
Update all tests to use TOML.

We do not change any public-facing documentation or user-messages - those still refer to INI.

… string

Eric-Arellano

Reviewers: each commit is distinct and they're generally ordered logically (although it looks like GitHub swapped the order of the last two commits?)

Eric-Arellano · 2020-02-11T00:17:30Z

build-support/migration-support/migrate_to_toml_config.py

@@ -83,7 +83,7 @@ def generate_new_config(config: Path) -> List[str]:
  for i, line in enumerate(original_text_lines):
    option_regex = r"(?P<option>[a-zA-Z0-9_]+)"
    before_value_regex = rf"\s*{option_regex}\s*[:=]\s*"
-    valid_value_characters = r"a-zA-Z0-9_.@!%\=\>\<\-\(\)\/"
+    valid_value_characters = r"a-zA-Z0-9_.@!:%\=\>\<\-\(\)\/"


Without this, we weren't properly converting options with : in the value, such as target: src/python:example.

Eric-Arellano · 2020-02-11T00:19:48Z

pants.remote.toml

 #     --remote-oauth-bearer-token-path=<(gcloud auth application-default print-access-token | perl -p -e 'chomp if eof')
 #     --no-v1 --v2 test tests/python/pants_test/util:strutil
 #
 # Remoting does not work for every goal, so you should not permanently point to this ini file, e.g.
 # via an env var; only point to it when you want to remote a specific invocation.

-[DEFAULT]


I changed this to GLOBAL. It works to be DEFAULT but is not very idiomatic. DEFAULT is useful when you want to use string interpolation or apply an option to multiple sections.

Eric-Arellano · 2020-02-11T00:21:13Z

src/python/pants/option/config.py

-    if option in self.defaults:
-      return self._stringify_val(self.defaults[option])
    if option not in section_values:
-      raise configparser.NoOptionError(option, section)
+      if option not in self.defaults:
+        raise configparser.NoOptionError(option, section)
+      return self._stringify_val(self.defaults[option])


These changes were necessary to get it working when an option overrides a DEFAULT option with the same name.

Eric-Arellano · 2020-02-11T00:21:57Z

src/python/pants/option/config.py

+
+
+@dataclass(frozen=True)
+class TomlSerializer:


Currently, we only use this for PantsRunIntegrationTest, but I imagine we might have some use down the road so I put it in config.py.

# Conflicts: # pants.ini

codealchemy

✔️

cosmicexplorer · 2020-02-11T18:30:47Z

This allows us to dogfood TOML support.

I'm not sure this description covers the change to also using e.g. pants.toml instead of pants.ini in all documentation, and deleting pants.ini. Is the plan to remove ini support entirely? Where is that documented?

Eric-Arellano · 2020-02-11T18:39:34Z

Is the plan to remove ini support entirely?

I was planning in a followup to propose deprecating INI so that we can consolidate around one sane config file. I first mentioned my intention to propose doing this in the original TOML PR: #9052. It's also why it was so important to me that we have migrate_to_toml_config.py, so that the deprecation doesn't take more than 3-10 minutes for a Pants admin to address.

Good point about not updating any public-facing deprecation warnings, though! You're right that it doesn't belong here and I was being lazy including here.

….toml. Stick with pants.ini We do still update all tests to refer to TOML, though, as this PR makes that change.

cosmicexplorer

Awesome!

Eric-Arellano · 2020-02-11T21:27:19Z

I had canceled the CI run on this last commit because it only made 3 small changes to docstring / user-messages. The prior commit was green.

Thanks for the reviews!

stuhood

Thanks!

stuhood · 2020-02-11T21:23:14Z

pants.toml

+  "%(buildroot)s/contrib/thrifty/src/python",
+  "%(buildroot)s/pants-plugins/src/python",
+]
+backend_packages.add = [


Can toml keys contain any useful special characters? If so, it might actually be desirable to have the ".add"/".remove" syntaxes jump out even more than they do as magical... strawman:

backend_packages.++ = [ .. ]

(related: @illicitonion recently pointed out a convention of using # keys in json to add comments, which made me think about this.)

Oh good idea! Unfortunately, with the current spec, you'd have to wrap it in quotes:

backend_packages."++" = [ "..", ]

But they're considering loosening the spec to allow any unicode symbol for a key name (particularly helpful for other languages). toml-lang/toml#687

blorente

I didn't submit the review the first time :(

blorente · 2020-02-11T14:50:05Z

contrib/python/src/python/pants/contrib/python/checks/tasks/python_eval.py

-                   "\n\nTo prepare, set `skip: True` in your `pants.ini` under the section "
-                   "`python-eval`. If you still need to use this tool, set `skip: False`. In "
-                   "Pants 1.27.0.dev0, the default will change from `skip: False` to `skip: True`, "
+                   "\n\nTo prepare, set `skip = true` in your `pants.toml` under the section "


I'm sure there's cases of this old ini syntax still hanging around in the codebase, even if the comments don't specifically refer to pants.ini. It's probably out of the scope of this change, but would it make sense to open a follow-up issue to search and change those?

Eric-Arellano force-pushed the use-toml branch 4 times, most recently from df446ce to d0433e1 Compare February 10, 2020 21:41

Eric-Arellano changed the title ~~WIP: Use pants.toml internally~~ Use pants.toml internally Feb 11, 2020

Eric-Arellano marked this pull request as ready for review February 11, 2020 00:15

Eric-Arellano requested review from blorente, codealchemy, illicitonion and stuhood February 11, 2020 00:16

Eric-Arellano added 5 commits February 10, 2020 17:25

Fix TOML handling of overridden default values

842baa0

Fix migrate_to_toml_config.py to work with the : character

91aa17b

Replace config with TOML

520339e

Add config.TomlSerializer to from parsed Python values back to a TOML…

2982e38

… string

Update code, including tests, to refer to pants.toml

fc9003c

Eric-Arellano force-pushed the use-toml branch from d2c1370 to fc9003c Compare February 11, 2020 00:25

Eric-Arellano commented Feb 11, 2020

View reviewed changes

Eric-Arellano requested review from cosmicexplorer and gshuflin February 11, 2020 01:58

Merge branch 'master' of github.com:pantsbuild/pants into use-toml

780914d

# Conflicts: # pants.ini

codealchemy approved these changes Feb 11, 2020

View reviewed changes

Eric-Arellano added 2 commits February 11, 2020 11:46

Stop changing any user-facing documentation and messages to use pants…

f762e36

….toml. Stick with pants.ini We do still update all tests to refer to TOML, though, as this PR makes that change.

Remove 3 last references to pants.toml in production code

875ef4c

gshuflin approved these changes Feb 11, 2020

View reviewed changes

cosmicexplorer approved these changes Feb 11, 2020

View reviewed changes

Eric-Arellano merged commit e13ae3f into pantsbuild:master Feb 11, 2020

Eric-Arellano deleted the use-toml branch February 11, 2020 21:27

stuhood approved these changes Feb 11, 2020

View reviewed changes

blorente reviewed Feb 12, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `pants.toml` internally #9090

Use `pants.toml` internally #9090

Eric-Arellano commented Feb 7, 2020 •

edited

Loading

Eric-Arellano left a comment

Eric-Arellano Feb 11, 2020

Eric-Arellano Feb 11, 2020

Eric-Arellano Feb 11, 2020

Eric-Arellano Feb 11, 2020

codealchemy left a comment

cosmicexplorer commented Feb 11, 2020

Eric-Arellano commented Feb 11, 2020

cosmicexplorer left a comment

Eric-Arellano commented Feb 11, 2020

stuhood left a comment

stuhood Feb 11, 2020

Eric-Arellano Feb 11, 2020

blorente left a comment

blorente Feb 11, 2020



		@dataclass(frozen=True)
		class TomlSerializer:

Use pants.toml internally #9090

Use pants.toml internally #9090

Conversation

Eric-Arellano commented Feb 7, 2020 • edited Loading

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano Feb 11, 2020

Choose a reason for hiding this comment

Eric-Arellano Feb 11, 2020

Choose a reason for hiding this comment

Eric-Arellano Feb 11, 2020

Choose a reason for hiding this comment

Eric-Arellano Feb 11, 2020

Choose a reason for hiding this comment

codealchemy left a comment

Choose a reason for hiding this comment

cosmicexplorer commented Feb 11, 2020

Eric-Arellano commented Feb 11, 2020

cosmicexplorer left a comment

Choose a reason for hiding this comment

Eric-Arellano commented Feb 11, 2020

stuhood left a comment

Choose a reason for hiding this comment

stuhood Feb 11, 2020

Choose a reason for hiding this comment

Eric-Arellano Feb 11, 2020

Choose a reason for hiding this comment

blorente left a comment

Choose a reason for hiding this comment

blorente Feb 11, 2020

Choose a reason for hiding this comment

Use `pants.toml` internally #9090

Use `pants.toml` internally #9090

Eric-Arellano commented Feb 7, 2020 •

edited

Loading