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

cleaner: remove RECORD and modify INSTALLER #16866

Merged
merged 1 commit into from
Mar 10, 2024

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Mar 10, 2024

EDIT: Updated contents to match latest commit

According to Python specification, we should remove RECORD file
to prevent changes to installed formula files via other tools, e.g. pip.
This also improves chances of generating an all bottle as it avoids
diff due to checksums of HOMEBREW_PREFIX present files. Also modify
INSTALLER file to indicate that brew is managing the Python package.

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

brew bottle --json --only-json-tab asciidoc
==> Determining asciidoc bottle rebuild...
==> Bottling asciidoc--10.2.0.arm64_sonoma.bottle.7.tar.gz...
./asciidoc--10.2.0.arm64_sonoma.bottle.7.tar.gz
  bottle do
    rebuild 7
    sha256 cellar: :any_skip_relocation, arm64_sonoma: "397fc8b36e3079d3f387c91a9df61d0314f34c26f98c4dcebf44b13a0498d648"
  endbrew fetch -q asciidoc
SHA256: 215cdd544ce3046f58f868347c476c8fa83b3391b2a700e6ddd1f9b65b0e9c4fdiffoscope $(brew --cache -q asciidoc) asciidoc--10.2.0.arm64_sonoma.bottle.7.tar.gz
--- /Users/cho-m/Library/Caches/Homebrew/downloads/0dc87f8a307fb752c8a06de20d603793dcd2d4a2aab76dcbf6085012327a2507--asciidoc--10.2.0.arm64_sonoma.bottle.6.tar.gz
+++ asciidoc--10.2.0.arm64_sonoma.bottle.7.tar.gz
│   --- 0dc87f8a307fb752c8a06de20d603793dcd2d4a2aab76dcbf6085012327a2507--asciidoc--10.2.0.arm64_sonoma.bottle.6.tar
├── +++ asciidoc--10.2.0.arm64_sonoma.bottle.7.tar
│ ├── file list
│ │ @@ -142,15 +142,14 @@
│ │  -rw-r--r--   0        0        0    11241 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc/resources/themes/flask/flask.css
│ │  drwxr-xr-x   0        0        0        0 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc/resources/themes/volnitsky/
│ │  -rw-r--r--   0        0        0     8043 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc/resources/themes/volnitsky/volnitsky.css
│ │  -rw-r--r--   0        0        0     1954 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc/resources/xhtml11-quirks.conf
│ │  -rw-r--r--   0        0        0    23017 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc/resources/xhtml11.conf
│ │  -rw-r--r--   0        0        0     5810 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc/utils.py
│ │  drwxr-xr-x   0        0        0        0 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc-10.2.0.dist-info/
│ │ --rw-r--r--   0        0        0        4 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc-10.2.0.dist-info/INSTALLER
│ │ +-rw-r--r--   0        0        0        5 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc-10.2.0.dist-info/INSTALLER
│ │  -rw-r--r--   0        0        0     3103 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc-10.2.0.dist-info/METADATA
│ │ --rw-r--r--   0        0        0    11135 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc-10.2.0.dist-info/RECORD
│ │  -rw-r--r--   0        0        0        0 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc-10.2.0.dist-info/REQUESTED
│ │  -rw-r--r--   0        0        0       92 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc-10.2.0.dist-info/WHEEL
│ │  -rw-r--r--   0        0        0       74 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc-10.2.0.dist-info/entry_points.txt
│ │  -rw-r--r--   0        0        0        9 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc-10.2.0.dist-info/top_level.txt
│ │  -rw-r--r--   0        0        0      361 2022-05-22 17:07:31.000000 asciidoc/10.2.0/libexec/pyvenv.cfg
│ ├── asciidoc/10.2.0/libexec/lib/python3.12/site-packages/asciidoc-10.2.0.dist-info/INSTALLER
│ │ @@ -1 +1 @@
│ │ -pip
│ │ +brew

@cho-m cho-m force-pushed the venv-all-bottle branch from c4cac48 to 5f550a6 Compare March 10, 2024 03:01
@Bo98
Copy link
Member

Bo98 commented Mar 10, 2024

To minimize potential impact, we specifically restrict the replacement to match relative path of common directory structure, e.g. ../../../bin/<script>, and the expected CSV entries. The regex can be modified/expanded if RECORD format changes or if we find additional files that need similar modification.

I guess the concern is relocation can happen in literally any text file, including any .py file. So there's not really any filename regex that covers that.

There are most drastic measures: https://salsa.debian.org/python-team/tools/dh-python/-/commit/b369c028b398f8893b7fd94f2a0cb67e34e26ad0. In the end, interfacing Homebrew-provided Python packages with pip isn't really supported - you should manage them via brew instead. Looking into it more actually, it's probably actually what we want:

If the RECORD file is missing, tools that rely on .dist-info must not attempt to uninstall or upgrade the package. (This restriction does not apply to tools that rely on other sources of information, such as system package managers in Linux distros.)

And perhaps replace INSTALLER:

For example, if a tool is asked to uninstall a project but finds no RECORD file, it may suggest that the tool named in INSTALLER may be able to do the uninstallation.

See https://packaging.python.org/en/latest/specifications/recording-installed-packages/#intentionally-preventing-changes-to-installed-packages

@cho-m
Copy link
Member Author

cho-m commented Mar 10, 2024

I'll go ahead and just delete RECORD given there is official documentation allowing it.

Changing INSTALLER does make message more useful:

ERROR: Cannot uninstall numpy 1.26.4, RECORD file not found. You might be able to recover from this via: 'pip install --force-reinstall --no-deps numpy==1.26.4'.
ERROR: Cannot uninstall numpy 1.26.4, RECORD file not found. Hint: The package was installed by brew.

@cho-m cho-m force-pushed the venv-all-bottle branch from 5f550a6 to ae5cb28 Compare March 10, 2024 13:36
@cho-m cho-m changed the title cleaner: modify RECORD for bin scripts cleaner: remove RECORD and modify INSTALLER Mar 10, 2024
@cho-m cho-m force-pushed the venv-all-bottle branch from ae5cb28 to 4b8d2fe Compare March 10, 2024 13:42
Library/Homebrew/cleaner.rb Outdated Show resolved Hide resolved
According to [Python specification][1], we should remove `RECORD` file
to prevent changes to installed formula files via other tools, e.g. pip.
This also improves chances of generating an `all` bottle as it avoids
diff due to checksums of HOMEBREW_PREFIX present files. Also modify
`INSTALLER` file to indicate that brew is managing the Python package.

[1]: https://packaging.python.org/en/latest/specifications/recording-installed-packages/#intentionally-preventing-changes-to-installed-packages

Signed-off-by: Michael Cho <michael@michaelcho.dev>
@cho-m cho-m force-pushed the venv-all-bottle branch from 4b8d2fe to 74f3105 Compare March 10, 2024 14:23
Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

Thanks!

@cho-m cho-m merged commit ae0ceee into Homebrew:master Mar 10, 2024
26 checks passed
@cho-m cho-m deleted the venv-all-bottle branch March 10, 2024 23:55
@MikeMcQuaid
Copy link
Member

Thanks again @cho-m!

@github-actions github-actions bot added the outdated PR was locked due to age label Apr 11, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants