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

Auto-generate pants.ini with pinned pants_version if file is missing #45

Closed
wants to merge 12 commits into from

Conversation

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

commented Mar 21, 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. ./pants
  4. Inspect pants.ini to make sure you're okay with the defaults.

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.

Concern about creating pants.ini being a tutorial

While there is some value in creating pants.ini so that users see how the config works, we can still get this benefit via logging when we create it and then instructing them on the website to inspect the file and change the values if desired.

Solution

  • Add logic to check if pants.ini exists and create it if not.
  • Modify pants_ini_config_value to work if pants.ini is missing.

Testing

Add a test file specifically dedicated to this feature that temporarily deletes our pants.ini, runs ./pants, and inspects the results to confirm they're as expected.

This is a separate file, rather than being with ci.py, because they are testing different things. This solely is meant to test our auto-generation works, whereas ci.py is meant to test that we 1) bootstrap properly, 2) properly parse pants_version and pants_runtime_python_version, and 3) ./pants -V and ./pants list :: work with both pantsd on and off.

Result

If you already have pants.ini, everything behaves the same.

If you don't, we'll create it and print this:

Config file `pants.ini` not detected in the repository root. Creating with sensible defaults:

* Pinning `pants_version` to the most recent stable release: 1.14.0.

`pants.ini` created. You may now run `./pants` normally.
@@ -90,8 +91,9 @@ matrix:
--skip-pantsd-tests
- ./build-support/ci.py
--pants-versions config
--python-versions unspecified 2.7 3.6
--python-versions unspecified 2.7 3.6 3.7

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 21, 2019

Author Contributor

This was accidentally removed in #42.

log "Config file \`pants.ini\` not detected in the repository root. Creating with sensible defaults.\n"
touch "pants.ini"
pants_version="$(./pants -V)"
log "* Pinning \`pants_version\` to the most recent stable release: ${pants_version}."

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 21, 2019

Author Contributor

After 1.15.0 is released, we'll also automatically pin pants_runtime_python_version!

* Pinning `pants_runtime_python_version` to Python 2.7. Ensure you have this interpreter available everywhere you plan to use Pants!

Will probably try to default to Python 3.6, then 3.7, then 2.7, so that dropping Py27 after 1.17.0 is a bit less surprising. TBD..

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 21, 2019

Would it be even less surprising if we were to also mention in the logging to the user here that in 1.15.0 we will automatically pin pants_runtime_python_version?

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 21, 2019

Author Contributor

I think that might confuse people here, because there is nothing actionable for them to be able to do.

We're only about 1-3 weeks away from 1.15.0, depending on how many RCs we have, so this will be fixed soon!

@cosmicexplorer cosmicexplorer self-requested a review Mar 21, 2019

@cosmicexplorer
Copy link

left a comment

I am a huge fan of this change! I personally always felt annoyed that I had to go look up how to set up pants every single time I wanted to make a new repo and I think being explicit about the changes we're doing automatically (but still doing them automatically) is a great precedent to set.

Show resolved Hide resolved build-support/common.py Outdated
Show resolved Hide resolved build-support/test_pants_ini_autogen.py Outdated
log "Config file \`pants.ini\` not detected in the repository root. Creating with sensible defaults.\n"
touch "pants.ini"
pants_version="$(./pants -V)"
log "* Pinning \`pants_version\` to the most recent stable release: ${pants_version}."

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 21, 2019

Would it be even less surprising if we were to also mention in the logging to the user here that in 1.15.0 we will automatically pin pants_runtime_python_version?

Show resolved Hide resolved build-support/test_pants_ini_autogen.py Outdated
return 0
fi
log "Config file \`pants.ini\` not detected in the repository root. Creating with sensible defaults.\n"
touch "pants.ini"

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 21, 2019

I think you can remove this touch, just add more log "* ..." lines for any other sensible defaults we may add in the future, and still write it all at once in a heredoc.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 21, 2019

Author Contributor

We have to keep the touch because of the recursive call to ./pants -V. Otherwise the script will attempt to continually generate the file.

This comment has been minimized.

Copy link
@cosmicexplorer
Show resolved Hide resolved pants Outdated
[GLOBAL]
pants_version: ${pants_version}
EOF
green "\n\`pants.ini\` created. You may now run \`./pants\`."

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 21, 2019

Within single quotes, nothing is substituted (which is why I try to use them where possible, to make it more clear when things are getting substituted into a string), so you can do things a little more fearlessly:

Suggested change
green "\n\`pants.ini\` created. You may now run \`./pants\`."
green '\n`pants.ini` created. You may now run `./pants`.'

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 21, 2019

Author Contributor

I tried this suggestion, but then shellcheck.py complains that it thinks we're intending to do command substitution and it explains we would then need double quotes. So I kept as is.

Show resolved Hide resolved pants Outdated

@Eric-Arellano Eric-Arellano requested review from stuhood, jsirois and benjyw Mar 21, 2019

@jsirois

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Did you think about instead making this a goal so the code / complication could live in the pants repo?. The instructions might just be:

  1. curl
  2. chmod
  3. ./pants version

Where version might print out the version like --version when configured and otherwise prompt to pick the version and edit pants.ini if unset.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Did you think about instead making this a goal so the code / complication could live in the pants repo?. The instructions might just be:

No, I did not think much about making it a Pants goal.

My main concern would be that after 1.15.0, we will also wan to auto-pin pants_runtime_python_version. That wouldn't make as much sense to live in the Pants repo, as Pants cannot / should not modify the Python interpreter it runs with, since that is the responsibility of the calling ./pants script.

Also, this feature is solely for first time users who do not already have pants.ini, so it seems less right for this to belong in Pants repo than in a setup script.

@stuhood

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

I agree with @jsirois that it is worth considering at least. If you still wanted it to be automagic, you could replace his third step with something like ./pants pin-pants-version, which would write out the file or prompt you if it already exists.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

I agree with @jsirois that it is worth considering at least. If you still wanted it to be automagic, you could replace his third step with something like ./pants pin-pants-version, which would write out the file or prompt you if it already exists.

This would still require the user to still create pants.ini, or ./pants pin-pants-version will fail to work.

Steps would be:

  1. curl pants
  2. chmod +x pants
  3. touch pants.ini
  4. Copy and paste [GLOBAL] into the file.
  5. ./pants pin-pants-version.

Maybe we could skip step 4 and have ./pants pin-pants-version add [GLOBAL] if missing, in which case it would be:

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

--

Once we add pinning pants_runtime_python_version, steps would be:

  1. curl pants
  2. chmod +x pants
  3. touch pants.ini
  4. ./pants pin-pants-version.
  5. ./pants pin-pants-runtime-python-version.

As opposed to:

  1. curl pants
  2. chmod +x pants
  3. ./pants, which would log everything being pinned.

--

If we go with the goals approach, would we allow optional arguments?

$ ./pants pin-pants-version 1.15.0.dev4
$ ./pants pin-pants-runtime-python-version 3.6

If left off, default to current version being used and current interpreter being used?

--

Any risk for slippery slope, e.g. why do we not have pin-python-setup-interpreter-constraints or pin-x etc?

--

Both options are improvements on what we have, so I can take the Pants goals approach if you all think it's the right approach. Let me know.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Okay I talked with Danny and agree with you both now! We came up with this proposal:

./pants generate-config

Defaults to using the Pants Version in the file VERSION and the Python interpreter being used.

--

Also would allow optional args:

./pants generate-config --new-pants-version 1.15.0.dev4 --new-runtime-python-version 3.6

If pants.ini already exists, it will modify it with those new values and then exit, so they get picked up the next Pants invocation.

--

This has the benefit of being a tutorial to running Pants and how goals work.

Also addresses my main concern of a proliferation of pin-x goals.

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:auto-pants-ini branch Mar 21, 2019

Eric-Arellano added a commit to pantsbuild/pants 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 to pantsbuild/pants 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.
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.