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 command to check for vX.Y.Z tag vs pybind11/_version.py consistency. #4757

Merged
merged 6 commits into from
Jul 23, 2023

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Jul 17, 2023

Description

Slightly expand instructions based on lesson just learned (see #4756).

Piggy-backing hints for converting changelog to release message.

Suggested changelog entry:

…cy. Piggy-backing hints for converting changelog to release message.
@rwgk rwgk requested a review from henryiii July 17, 2023 19:55
@rwgk
Copy link
Collaborator Author

rwgk commented Jul 20, 2023

Hi @henryiii did you get a chance to look at this tiny update?

I just double-checked the git diff command, with a fresh eye, practicing a mock release. It does work as intended:

  • If the branch tag corresponding to what's in pybind11/_version.py does not exist:
$ git diff "$(grep ^__version__ pybind11/_version.py | cut -d'"' -f2 | sed 's/^/v/')"
fatal: ambiguous argument 'v2.11.9': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
  • if the branch tag exists but the tagged commit is not identical to the currently checked-out git working copy it shows the diff.

I think that's exactly what we need as a last line of defense, right before the git push --tags in the instructions.

docs/release.rst Outdated
@@ -47,6 +47,9 @@ If you don't have nox, you should either use ``pipx run nox`` instead, or use
- Update tags (optional; if you skip this, the GitHub release makes a
non-annotated tag for you)
- ``git tag -a vX.Y.Z -m 'vX.Y.Z release'``.
- To ensure the new tag is consistent with the version number in the sources, run this command:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the current tag number shown? __version__ and pybind11/_version.py are already forced to be in sync by nox -s tests_packaging above, if that's all this is doing.

Copy link
Collaborator Author

@rwgk rwgk Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nox command only ensures that common.h and pybind11/_version.py are fully self-consistent.

Which is something I take for granted.

The git-diff-grep command would have shown a diff because it would have pulled v2.11.0 from pybind11/_version.py, but at that step in the instructions my working copy had the v2.11.1 changes, just not the version number update.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm failing to understand why this would catch anything that git status or a plain git diff would not show (or, in my terminal, the current git condition is shown as a colored bar). It's not using or comparing the tag itself at all.

Could it be replaced with "make sure all changes are committed"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simply forgot to update the version number, and everybody else overlooked it, too.

git status would not have shown anything.

git diff also would not have shown anything.

git diff v2.11.0 would have shown that the code I had was not what was tagged as v2.11.0.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh, oooookay. That seems like an interesting way to so it. Can't you just print out the current version and make sure it's the same as the tag? grep ^__version__ pybind11/_version.py | cut -d'"' -f2 | sed 's/^/v/'" should show the current tag.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And grep ^__version__ pybind11/_version.py would be enough for a human user to verify the code states the version they just tagged.

docs/release.rst Outdated
Comment on lines 51 to 52
- ``git diff "$(grep ^__version__ pybind11/_version.py | cut -d'"' -f2 | sed 's/^/v/')"``
- There should be no diff.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- ``git diff "$(grep ^__version__ pybind11/_version.py | cut -d'"' -f2 | sed 's/^/v/')"``
- There should be no diff.
- ``grep ^__version__ pybind11/_version.py``
- This should print the version you just tagged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a slightly different wording before seeing this comment.
Give me a moment to try again.
Simpler is better I agree.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus it's not great for the "working" path to produce no feedback that it worked. Your wording is fine.

@rwgk
Copy link
Collaborator Author

rwgk commented Jul 20, 2023

@henryiii I removed a few line breaks to make it look better in the web view.

https://github.com/pybind/pybind11/blob/2b54c6539951cbd819b00b2140182bcf9a100ed7/docs/release.rst

Do you know if we can have the line breaks in the .rst file without making it look weird in the web view?

It's still not perfect, but good enough?

Lesson learned:

This is NOT GOOD:

```
- Bullet nesting level 1.
    - Bullet nesting level 2.
```

This is BETTER:

```
- Bullet nesting level 1.
  - Bullet nesting level 2.
```

Also consistently adding empty lines between bullet points, to make the .rst
file easier to read.

Also piggy-backing a few very minor enhancements.
@rwgk
Copy link
Collaborator Author

rwgk commented Jul 23, 2023

It's still not perfect, but good enough?

With a lot of trial and error I finally figured out then main problem:

This is NOT GOOD:

- Bullet nesting level 1.
    - Bullet nesting level 2.

This is BETTER:

- Bullet nesting level 1.
  - Bullet nesting level 2.

With that and some minor miscellaneous polishing the rendered page on the web looks nice now. I'll go ahead and merge.

@rwgk rwgk merged commit f3e0602 into pybind:master Jul 23, 2023
@rwgk rwgk deleted the release_rst_tweaks branch July 23, 2023 18:10
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 23, 2023
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Jul 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants