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 cask install receipts #17554

Merged
merged 11 commits into from
Jul 13, 2024
Merged

Add cask install receipts #17554

merged 11 commits into from
Jul 13, 2024

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Jun 23, 2024

  • 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?

This PR is my first go at adding a cast install receipt (which I've called Tab to align for formulae) as per #17013.

Here is a sample INSTALL_RECEIPT.json that is generated when I run brew install --cask warp:

{
  "homebrew_version": "4.3.6-71-gd665c42-dirty",
  "loaded_from_api": true,
  "installed_as_dependency": false,
  "installed_on_request": true,
  "time": 1719106768,
  "dependencies": {},
  "arch": "arm64",
  "source": {
    "path": "",
    "tap": "homebrew/cask",
    "tap_git_head": "f755fc333ebe7647fa3988e360d47a82120db032",
    "version": "0.2024.06.18.08.02.stable_03"
  },
  "installed_on": {
    "os": "Macintosh",
    "os_version": "macOS 14",
    "cpu_family": "arm_firestorm_icestorm",
    "xcode": "15.4",
    "clt": "15.3.0.0.1.1708646388",
    "preferred_perl": "5.34"
  },
  "artifacts": [
    {
      "app": [
        "Warp.app"
      ]
    },
    {
      "zap": [
        {
          "trash": [
            "~/Library/Application Support/dev.warp.Warp-Stable",
            "~/Library/Logs/warp.log",
            "~/Library/Preferences/dev.warp.Warp-Stable.plist",
            "~/Library/Saved Application State/dev.warp.Warp-Stable.savedState"
          ]
        }
      ]
    }
  ]
}

It doesn't quite work yet (there are some test failures and some other things still) but I wanted to push this up to start getting feedback on the strategy.

@request-info request-info bot added the needs response Needs a response from the issue/PR author label Jun 23, 2024
@Rylan12 Rylan12 removed the needs response Needs a response from the issue/PR author label Jun 23, 2024
@Homebrew Homebrew deleted a comment from request-info bot Jun 23, 2024
@Rylan12 Rylan12 requested a review from a team June 23, 2024 01:41
@Rylan12 Rylan12 added the cask Homebrew Cask label Jun 23, 2024
Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

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

I think in general the approach looks good. It'd be nice to be able to share logic between the formula and cask tab classes though it's possible that there isn't enough overlap to make that worth it.

There are few use cases for this that I've seen recently and I think they all seem to be covered here.

  1. Storing the tap that a cask was installed from.
  2. Storing the cask version.

Library/Homebrew/cask/tab.rb Outdated Show resolved Hide resolved
Library/Homebrew/cask/tab.rb Outdated Show resolved Hide resolved
@Bo98
Copy link
Member

Bo98 commented Jun 24, 2024

Do you have an example what the tab of a cask with an uninstall DSL looks like?

Idea being we avoid reading the Ruby file entirely when uninstalling, except for flight blocks which the tab should have a boolean or something that indicates those are used.

@Rylan12
Copy link
Member Author

Rylan12 commented Jun 24, 2024

Do you have an example what the tab of a cask with an uninstall DSL looks like?

Here is the tab for slack:

{
  "homebrew_version": "4.3.6-71-ge30aea5-dirty",
  "loaded_from_api": true,
  "installed_as_dependency": false,
  "installed_on_request": true,
  "time": 1719197835,
  "dependencies": {
    "macos": {
      ">=": [
        "10.15"
      ]
    }
  },
  "arch": "arm64",
  "source": {
    "path": "",
    "tap": "homebrew/cask",
    "tap_git_head": "f755fc333ebe7647fa3988e360d47a82120db032",
    "version": "4.39.88"
  },
  "installed_on": {
    "os": "Macintosh",
    "os_version": "macOS 14",
    "cpu_family": "arm_firestorm_icestorm",
    "xcode": "15.4",
    "clt": "15.3.0.0.1.1708646388",
    "preferred_perl": "5.34"
  },
  "artifacts": [
    {
      "uninstall": [
        {
          "quit": "com.tinyspeck.slackmacgap"
        }
      ]
    },
    {
      "app": [
        "Slack.app"
      ]
    },
    {
      "zap": [
        {
          "trash": [
            "~/Library/Application Scripts/com.tinyspeck.slackmacgap",
            "~/Library/Application Support/com.apple.sharedfilelist/com.apple.LSSharedFileList.ApplicationRecentDocuments/com.tinyspeck.slackmacgap.sfl*",
            "~/Library/Application Support/Slack",
            "~/Library/Caches/com.tinyspeck.slackmacgap*",
            "~/Library/Containers/com.tinyspeck.slackmacgap*",
            "~/Library/Cookies/com.tinyspeck.slackmacgap.binarycookies",
            "~/Library/Group Containers/*.com.tinyspeck.slackmacgap",
            "~/Library/Group Containers/*.slack",
            "~/Library/HTTPStorages/com.tinyspeck.slackmacgap*",
            "~/Library/Logs/Slack",
            "~/Library/Preferences/ByHost/com.tinyspeck.slackmacgap.ShipIt.*.plist",
            "~/Library/Preferences/com.tinyspeck.slackmacgap*",
            "~/Library/Saved Application State/com.tinyspeck.slackmacgap.savedState",
            "~/Library/WebKit/com.tinyspeck.slackmacgap"
          ]
        }
      ]
    }
  ]
}

Idea being we avoid reading the Ruby file entirely when uninstalling, except for flight blocks which the tab should have a boolean or something that indicates those are used.

Right now the format I'm saving the artifacts in is the same that is used in the API, so that should be possible (that's what I anticipated happening). Good note on the boolean to indicate about flight blocks, I'll add that.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looking good so far, great work @Rylan12!

Library/Homebrew/cask/info.rb Outdated Show resolved Hide resolved
Library/Homebrew/cask/tab.rb Outdated Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member

  • Storing the tap that a cask was installed from.
  • Storing the cask version.

In both of these cases: we should store the same for formulae and in the same format for both (to make parsing either easier).

Idea being we avoid reading the Ruby file entirely when uninstalling, except for flight blocks which the tab should have a boolean or something that indicates those are used.

My understanding is also we hope to be able to eventually deprecate these flight blocks so that only the tab is needed for uninstall.

@Bo98
Copy link
Member

Bo98 commented Jun 24, 2024

My understanding is also we hope to be able to eventually deprecate these flight blocks so that only the tab is needed for uninstall.

Yeah, though key word there is "eventually" and no one's working on it yet so still should have something in the meantime to improve the majority of cases while we work on improving the final few cases that use flight blocks.

Flight block phasing out will likely require multiple distinct enhancements to the uninstall DSL.

@MikeMcQuaid
Copy link
Member

Yeah, though key word there is "eventually" and no one's working on it yet so still should have something in the meantime to improve the majority of cases while we work on improving the final few cases that use flight blocks.

Agreed: just think it's worth considering with the design that we will hopefully not keep this around indefinitely.

@Rylan12 Rylan12 force-pushed the cask-install-receipt branch 2 times, most recently from 336fd06 to a4ea7d8 Compare June 25, 2024 18:05
@Rylan12 Rylan12 marked this pull request as ready for review June 25, 2024 18:06
@Rylan12
Copy link
Member Author

Rylan12 commented Jun 25, 2024

I've pushed changes to have casks use the same Tab class as formulae, just with some different methods occasionally. When creating a new tab via an ambiguous method (i.e. when reading from a file and not calling for_formula, for_keg, for_cask, etc), you should specify whether this is a formula or cask tab (formula will be chosen by default if no selection was made). Then, methods like #to_json will intelligently include the correct fields.

I also updated the info and tab commands to support new features here (like marking casks as installed on request or not). Maybe in the future, brew autoremove can be extended to include casks.

Library/Homebrew/cask/cask.rb Outdated Show resolved Hide resolved
Library/Homebrew/cask/cask_loader.rb Outdated Show resolved Hide resolved
Library/Homebrew/tab.rb Show resolved Hide resolved
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Great work so far, thanks @Rylan12!

Library/Homebrew/cask/cask.rb Outdated Show resolved Hide resolved
Library/Homebrew/cask/cask_loader.rb Outdated Show resolved Hide resolved
Library/Homebrew/tab.rb Outdated Show resolved Hide resolved
Library/Homebrew/tab.rb Show resolved Hide resolved
Library/Homebrew/tab.rb Show resolved Hide resolved
Library/Homebrew/tab.rb Outdated Show resolved Hide resolved
Library/Homebrew/tab.rb Show resolved Hide resolved
@miccal miccal closed this Jun 26, 2024
@miccal miccal deleted the cask-install-receipt branch June 26, 2024 11:21
@miccal miccal restored the cask-install-receipt branch June 26, 2024 11:22
@miccal miccal reopened this Jun 26, 2024
@miccal
Copy link
Member

miccal commented Jun 26, 2024

Apologies for closing @Rylan12, my bad :(

@Rylan12 Rylan12 marked this pull request as draft July 2, 2024 23:08
@Rylan12 Rylan12 marked this pull request as ready for review July 3, 2024 00:48
@apainintheneck
Copy link
Contributor

Tangential: It looks like we don't currently save any info about whether a formula was installed from the API. Would that be useful debugging info to have at times?

The loaded_from_api entry is saved for formulae already. It's set to true when the formula was loaded from the API as opposed to the formula file. Same as is implemented here for casks

Ah, I got confused. My bad.

@Bo98
Copy link
Member

Bo98 commented Jul 3, 2024

The pre/post flight blocks do show up in the artifact lists (just as null), but we could generate it using artifacts_list compact: true to ignore those blocks if we want.

This is fine. We needed something to to determine whether flight blocks are used on uninstall so this works. We'll use this later to only load the Ruby file when needed on uninstall.

caskfile_only also checks non-uninstall flight blocks which wouldn't be useful for our case.

@Rylan12
Copy link
Member Author

Rylan12 commented Jul 4, 2024

caskfile_only also checks non-uninstall flight blocks which wouldn't be useful for our case.

Good point. I updated this in the latest version.

Also, I realized that zap artifacts were being ignored since those use zap_phase and not uninstall_phase, so I fixed that too.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks @Rylan12! Yeh, the AbstractTab approach is making a lot more sense to me.

Library/Homebrew/tab.rb Show resolved Hide resolved
Library/Homebrew/tab.rb Outdated Show resolved Hide resolved
Library/Homebrew/tab.rb Show resolved Hide resolved
Library/Homebrew/cask/tab.rb Outdated Show resolved Hide resolved
@Rylan12
Copy link
Member Author

Rylan12 commented Jul 4, 2024

I pushed changes adding some more shared items to the generic items list, but I left off tabfile, runtime_dependencies, and source.path because they all have different formula/cask calls that are used to generate them. With all of those, I can still add them to the generic method as nil if preferred.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks great, almost here! Just a few tweaks, some DRYing up and a little more test coverage and this is good to 🚢. Nice work @Rylan12!

Library/Homebrew/tab.rb Outdated Show resolved Hide resolved
Library/Homebrew/tab.rb Outdated Show resolved Hide resolved
Library/Homebrew/tab.rb Outdated Show resolved Hide resolved
Library/Homebrew/tab.rb Outdated Show resolved Hide resolved
Library/Homebrew/cask/info.rb Show resolved Hide resolved
Library/Homebrew/cask/tab.rb Outdated Show resolved Hide resolved
Library/Homebrew/cask/tab.rb Show resolved Hide resolved
Library/Homebrew/cask/cask.rb Outdated Show resolved Hide resolved
Library/Homebrew/cask/installer.rb Outdated Show resolved Hide resolved
Library/Homebrew/tab.rb Outdated Show resolved Hide resolved
Library/Homebrew/tab.rb Outdated Show resolved Hide resolved
Library/Homebrew/tab.rb Outdated Show resolved Hide resolved
Library/Homebrew/cask/cask.rb Outdated Show resolved Hide resolved
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looking great @Rylan12! I agree with @apainintheneck's comments and then I think we're good to 🚢 this. Thanks for the patience with all the back and forth; higher threshold than a typical PR here just because we want to avoid writing bad tabs and cleaning them up (as happened with formulae).

Library/Homebrew/tab.rb Outdated Show resolved Hide resolved
@Rylan12
Copy link
Member Author

Rylan12 commented Jul 9, 2024

higher threshold than a typical PR here just because we want to avoid writing bad tabs and cleaning them up (as happened with formulae).

Agreed, thanks!

@Rylan12
Copy link
Member Author

Rylan12 commented Jul 9, 2024

I've just pushed some more changes because I realized that the runtime dependencies in cask tabs did not include any recursive dependencies, only the dependencies that were directly declared. This should be fixed now (with some shared AbstractTab logic) and I've added a test.

With that, the runtime_dependencies entry in a cask tab looks something like this:

  "runtime_dependencies": {
    "cask": [
      {
        "full_name": "foo-cask",
        "version": "1.2.3",
        "declared_directly": true
      },
      {
        "full_name": "bar-cask",
        "version": "4.5.6",
        "declared_directly": false
      }
    ],
    "formula": [
      {
        "full_name": "foo-formula",
        "version": "1.2.3",
        "revision": 0,
        "pkg_version": "1.2.3",
        "declared_directly": true
      },
      {
        "full_name": "bar-formula",
        "version": "4.5.6",
        "revision": 0,
        "pkg_version": "4.5.6",
        "declared_directly": false
      }
    ],
    "macos": {
      ">=": [
        "10.14"
      ]
    },
    "arch": [
      {
        "type": "arm",
        "bits": 64
      }
    ]
  },

Should the macos and arch entries live somewhere else instead of underneath runtime_dependencies? Or, should we even include them at all? We don't include such information in formula tabs (although maybe we should?)

@apainintheneck
Copy link
Contributor

The approach to getting the recursive deps for each cask looks good to me.

I'm with you when you say that the arch and macos info maybe shouldn't be included in the runtime dependencies section. We already have arch at the top-level of the install receipt and os version is already included in the built_on section. It's not obvious to me that either of the arch or os dependencies are needed in the install receipt.

@MikeMcQuaid
Copy link
Member

Looks great, thanks @Rylan12! Let's merge this tomorrow when we're all together in case there's any issues.

@MikeMcQuaid MikeMcQuaid merged commit f39b5c1 into master Jul 13, 2024
25 checks passed
@MikeMcQuaid MikeMcQuaid deleted the cask-install-receipt branch July 13, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cask Homebrew Cask
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants