Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `./pants generate-pants-ini` for first time users to generate pants.ini with sensible defaults #7448

Merged
merged 21 commits into from Mar 29, 2019

Conversation

Projects
None yet
5 participants
@Eric-Arellano
Copy link
Contributor

commented Mar 28, 2019

Problem

Our first-time setup instructions are harder than need be. Currently, before being able to do anything with Pants, you must:

  1. curl pants
  2. chmod +x pants
  3. touch pants.ini
  4. Copy and paste [GLOBAL] into the file.
  5. Run ./pants -V or decide you want to pin a different value.
  6. Copy and paste that value into a new line pants_version.

Instead, we could simplify the instructions to:

  1. curl pants
  2. chmod +x pants
  3. touch pants.ini
  4. ./pants generate-pants-ini

Once we add instructions on pants_runtime_python_version, this would get even more complex.

Simplifying the onboarding process should make it easier for us to retain first time users. There are a lot of new concepts they already have to learn, e.g. BUILD files, targets, and goals. The less overwhelming we can make this process, the better.

Refer to https://pantsbuild.slack.com/archives/CBNMV1LRH/p1552506655047900 for the original discussion around this idea and pantsbuild/setup#45 for the original implementation until we realized this should be a goal.

Solution

Add new ./pants generate-pants-ini to setup the initial value with defaults as follows:

  • pants_version == version used during the run
  • pants_runtime_python_version == version used during the run

We do not provide options to override these defaults, because this is meant for first time users who would not yet be familiar with options or discovering them via --help. Further, they can change the defaults by rewriting pants.ini.

We also do not allow this command to be ran on pre-existing pants.ini, because Python's configparser would strip all comments and mess up the indentation. Instead, users should simply modify their file.

Result

If there is no content in pants.ini, we will print this and update pants.ini accordingly:

Adding sensible defaults to /Users/eric/DocsLocal/code/projects/pants/pants.ini:
* Pinning `pants_version` to `1.15.0rc0`.
* Pinning `pants_runtime_python_version` to `3.6`.

You may modify these values directly in the file at any time. The ./pants script will detect any changes the next time you run it.

You are now ready to use Pants!

If there is already content, we will fail the task and explain how to update, including the sensible defaults as a suggested edit.

TODO: allow missing pants.ini

Currently, Pants fails to execute if there is no pants.ini in the buildroot. This does not seem intentional, as an empty pants.ini will fix the issue.

As a followup, we should allow a missing pants.ini and then use this command to generate the file. It will allow us to avoid having to instruct touch pants.ini.

TODO: Updating docs

We cannot update the docs to reflect this new option until 1.15.0 is released. The setup repo defaults to the most stable version, and this goal will not be available in 1.14.0 so would confuse people.

Eric-Arellano added some commits Mar 28, 2019

Fail if pants.ini already exists
Rewriting it means we lose all comments and the indentation gets messed up. Instead, we should not allow this.
Rename generate-config to generate-pants-ini
It's more explicit, and generate-config suggests this function is more flexible than we really want it to be.
No longer allow options
Since we don't allow running the command on pre-existing pants.ini, it's less relevant to allow options. First-time users are not expected to know these exist or to use them. Instead, we want to teach them to update the values directly in pants.ini.

@Eric-Arellano Eric-Arellano marked this pull request as ready for review Mar 28, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Reviewers, the biggest thing I'd appreciate feedback on is the logs. This is the very first command new users will ever run, so it's important we get the UX right.

Thanks!

Eric-Arellano added some commits Mar 28, 2019

Round 1 of responding to John's suggestions
* Simply write directly to the file, rather than using configparser. Using configparser is a vestige of when this task originally allowed running even if pants.ini was populated, so needed to keep the original values. Now that we only generate new pants.ini, this is not necessary.
* Import pants.version.VERSION as pants_version.
* Use self.context.log. I'll look into making this a ConsoleTask. For now, this is an improvement.
@Eric-Arellano
Copy link
Contributor Author

left a comment

Thanks @jsirois! This is much better now.

For making this a ConsoleTask, I was hesitant to do so because of this docstring: ConsoleTasks are not intended to modify build state.. This task seems to violate that assumption, because it's an impure task that does modify pants.ini. I'll defer to your judgment if you still think I should change it to ConsoleTask, but it smells wrong to me.

@blorente
Copy link
Contributor

left a comment

I really like these changes, thanks!

If it were me I think I'd leaned towards a bash script that runs at the same time as we bootstrap native code, but a console goal seems even better!

However, could it be worth inserting this into the ./pants script itself, like we do to bootstrap code?

Show resolved Hide resolved src/python/pants/core_tasks/generate_pants_ini.py Outdated
Show resolved Hide resolved src/python/pants/core_tasks/generate_pants_ini.py Outdated
Borja's suggestions
* Remove unnecessary .format().
* Clarify we will only apply changes next time you run the script.
@Eric-Arellano
Copy link
Contributor Author

left a comment

Thanks @blorente!

We originally had this change as part of the setup script, and then decided in pantsbuild/setup#45 (comment) to instead make this a Pants goal to make the bash script less magical and keep it smaller, and so that running ./pants generate-pants-ini can be sort of like a tutorial to how goals work.

Show resolved Hide resolved src/python/pants/core_tasks/generate_pants_ini.py Outdated
Show resolved Hide resolved src/python/pants/core_tasks/generate_pants_ini.py Outdated

@Eric-Arellano Eric-Arellano added this to the 1.15.x milestone Mar 28, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Reviewers, question. We provide multiple global options to use a different config file than pants.ini:

register('--pants-config-files', advanced=True, type=list, daemon=False,
default=[get_default_pants_config_file()], help='Paths to Pants config files.')
# TODO: Deprecate the --pantsrc/--pantsrc-files options? This would require being able
# to set extra config file locations in an initial bootstrap config file.
register('--pantsrc', advanced=True, type=bool, default=True,
help='Use pantsrc files.')
register('--pantsrc-files', advanced=True, type=list, metavar='<path>', daemon=False,
default=['/etc/pantsrc', '~/.pants.rc'],
help='Override config with values from these files. '
'Later files override earlier ones.')

Should we hook these options up to this goal?

I strongly am biased towards no. This goal is exclusively meant for first time users, and we are not expecting them to have any compelling reason to want to use a different config file name nor do we want to introduce the cognitive load to consider this as an option. Integrating into the options would also mean we have to rename generate-pants-ini to something more general like generate-config, which makes the function more general and less explicit than I'd like.

Make goal robust to being ran from different directories than buildroot
Use pants.base.build_environment.get_default_pants_config_file() to make the script more robust.

Eric-Arellano added some commits Mar 28, 2019

Print suggested pants.ini content when user already has pants.ini wit…
…h entries

This makes the error message more useful.
Refactor test to use TaskTestBase
Doing this required moving the definition of PANTS_INI into the function execute(), rather than being a class constant, so that it gets evaluated dynamically instead of at improt time.
Show resolved Hide resolved src/python/pants/core_tasks/generate_pants_ini.py Outdated

Eric-Arellano added some commits Mar 28, 2019

Convert to ConsoleTask
John and Danny were 100% right this is better as a console task. It makes the logging much more natural, makes the task quiet (good thing here), and leads to simpler code.
Refactor tests to not use a setUp function
Now that we can use self.execute_task(), the only value we were pinning is temp_pants_ini_path. Once we change Pants to allow running without a pants.ini file created, we won't want this always created for the test case of a missing pants.ini. So, this is sort of pre-work + it leads to less code.
Show resolved Hide resolved tests/python/pants_test/core_tasks/BUILD Outdated
Show resolved Hide resolved src/python/pants/core_tasks/BUILD Outdated

Eric-Arellano added a commit to pantsbuild/setup that referenced this pull request Mar 29, 2019

Allow for pants.ini to be missing (#47)
### Problem
With us introducing the command `./pants generate-pants-ini` to simplify first time setup in pantsbuild/pants#7448, we'll be able to in a followup to that PR make an additional simplification by having `./pants generate-pants-ini` not only populate `pants.ini` but to also create it.

So, when a user first curls the bootstrap script, we will no longer have them `touch pants.ini`.

However, without `pants.ini`, our parsing function will result in a sed error that the file cannot be found.

### Solution
Only try to parse `pants.ini` if it exists.

@Eric-Arellano Eric-Arellano merged commit a009067 into pantsbuild:master Mar 29, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:generate-config branch Mar 29, 2019

Eric-Arellano added a commit that referenced this pull request Mar 29, 2019

Fix crash if pants.ini is missing in the buildroot (#7452)
### Problem
If `pants.ini` is not present, then Pants will crash with this error:

```
timestamp: 2019-03-28T18:16:34.356612
Exception caught: (exceptions.IOError)
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/bin/pants", line 10, in <module>
    sys.exit(main())
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/bin/pants_loader.py", line 85, in main
    PantsLoader.run()
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/bin/pants_loader.py", line 81, in run
    cls.load_and_execute(entrypoint)
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/bin/pants_loader.py", line 74, in load_and_execute
    entrypoint_main()
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/bin/pants_exe.py", line 39, in main
    PantsRunner(exiter, start_time=start_time).run()
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/bin/pants_runner.py", line 39, in run
    options_bootstrapper = OptionsBootstrapper.create(env=self._env, args=self._args)
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/option/options_bootstrapper.py", line 133, in create
    config_files_products = [filecontent_for(p) for p in config_file_paths]
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/option/options_bootstrapper.py", line 106, in filecontent_for
    return FileContent(ensure_text(path), read_file(path))
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/util/dirutil.py", line 155, in read_file
    with open(filename, mode) as f:

Exception message: [Errno 2] No such file or directory: '/Users/eric/DocsLocal/code/projects/setup/pants.ini'
```

This does not appear to be by design. Notably, we allow an empty `pants.ini` without any issue. There is no meaningful difference between an empty `pants.ini` from a non-existent `pants.ini`, so this seems to be a bug.

### Solution
First check if `pants.ini` exists before adding it to the list of config files.

This also allows us to refactor `./pants generate-pants-ini` from #7448 to create a new `pants.ini`, rather than requiring the user to make it. This allows us to simplify our setup instructions even more by removing the `touch pants.ini` instruction.

stuhood added a commit that referenced this pull request Mar 29, 2019

Add `./pants generate-pants-ini` for first time users to generate pan…
…ts.ini with sensible defaults (#7448)

### Problem
Our first-time [setup instructions](https://www.pantsbuild.org/install.html#recommended-installation) are harder than need be. Currently, before being able to do anything with Pants, you must:

1) curl `pants`
1) `chmod +x pants`
1) `touch pants.ini`
1) Copy and paste `[GLOBAL]` into the file.
1) Run `./pants -V` or decide you want to pin a different value.
1) Copy and paste that value into a new line `pants_version`.

Instead, we could simplify the instructions to:

1) curl `pants`
1) `chmod +x pants`
1) `touch pants.ini`
1) `./pants generate-pants-ini`

Once we add instructions on `pants_runtime_python_version`, this would get even more complex.

Simplifying the onboarding process should make it easier for us to retain first time users. There are a lot of new concepts they already have to learn, e.g. BUILD files, targets, and goals. The less overwhelming we can make this process, the better.

Refer to https://pantsbuild.slack.com/archives/CBNMV1LRH/p1552506655047900 for the original discussion around this idea and pantsbuild/setup#45 for the original implementation until we realized this should be a goal.

### Solution
Add new `./pants generate-pants-ini` to setup the initial value with defaults as follows:
* `pants_version` == version used during the run
* `pants_runtime_python_version` == version used during the run

We do not provide options to override these defaults, because this is meant for first time users who would not yet be familiar with options or discovering them via `--help`. Further, they can change the defaults by rewriting `pants.ini`.

We also do not allow this command to be ran on pre-existing `pants.ini`, because Python's configparser would strip all comments and mess up the indentation. Instead, users should simply modify their file.

### Result
If there is no content in `pants.ini`, we will print this and update `pants.ini` accordingly:
```
Adding sensible defaults to /Users/eric/DocsLocal/code/projects/pants/pants.ini:
* Pinning `pants_version` to `1.15.0rc0`.
* Pinning `pants_runtime_python_version` to `3.6`.

You may modify these values directly in the file at any time. The ./pants script will detect any changes the next time you run it.

You are now ready to use Pants!
```

If there is already content, we will fail the task and explain how to update, including the sensible defaults as a suggested edit.

### TODO: allow missing `pants.ini`
Currently, Pants fails to execute if there is no `pants.ini` in the buildroot. This does not seem intentional, as an empty `pants.ini` will fix the issue. 

As a followup, we should allow a missing `pants.ini` and then use this command to generate the file. It will allow us to avoid having to instruct `touch pants.ini`.

### TODO: Updating docs
We cannot update the docs to reflect this new option until 1.15.0 is released. The setup repo defaults to the most stable version, and this goal will not be available in 1.14.0 so would confuse people.

stuhood added a commit that referenced this pull request Mar 29, 2019

Fix crash if pants.ini is missing in the buildroot (#7452)
### Problem
If `pants.ini` is not present, then Pants will crash with this error:

```
timestamp: 2019-03-28T18:16:34.356612
Exception caught: (exceptions.IOError)
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/bin/pants", line 10, in <module>
    sys.exit(main())
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/bin/pants_loader.py", line 85, in main
    PantsLoader.run()
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/bin/pants_loader.py", line 81, in run
    cls.load_and_execute(entrypoint)
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/bin/pants_loader.py", line 74, in load_and_execute
    entrypoint_main()
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/bin/pants_exe.py", line 39, in main
    PantsRunner(exiter, start_time=start_time).run()
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/bin/pants_runner.py", line 39, in run
    options_bootstrapper = OptionsBootstrapper.create(env=self._env, args=self._args)
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/option/options_bootstrapper.py", line 133, in create
    config_files_products = [filecontent_for(p) for p in config_file_paths]
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/option/options_bootstrapper.py", line 106, in filecontent_for
    return FileContent(ensure_text(path), read_file(path))
  File "/Users/eric/.cache/pants/setup/bootstrap-Darwin-x86_64/unspecified_py27/lib/python2.7/site-packages/pants/util/dirutil.py", line 155, in read_file
    with open(filename, mode) as f:

Exception message: [Errno 2] No such file or directory: '/Users/eric/DocsLocal/code/projects/setup/pants.ini'
```

This does not appear to be by design. Notably, we allow an empty `pants.ini` without any issue. There is no meaningful difference between an empty `pants.ini` from a non-existent `pants.ini`, so this seems to be a bug.

### Solution
First check if `pants.ini` exists before adding it to the list of config files.

This also allows us to refactor `./pants generate-pants-ini` from #7448 to create a new `pants.ini`, rather than requiring the user to make it. This allows us to simplify our setup instructions even more by removing the `touch pants.ini` instruction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.