Conversation
bamcgill
left a comment
There was a problem hiding this comment.
This change is to set the latest downloaded version and link it to the latest reference in Caskroom/sqlcl/latest/sqlcl/bin so the user does not have to redo the environment variable for PATH each time we update.
|
Hi @bamcgill my feedback on a similar change from December remains here; I understand that the clash with sql makes this unable to be installed automatically to due to the conflict. But looking at the other package managers that have this available the binary is almost always linked as sqlcl, I think that is going to be our best course of action here, as postflight blocks are not tested, and are therefore more likely to break. If necessary we could add a caveat making it clear that the binary is renamed. |
bamcgill
left a comment
There was a problem hiding this comment.
We've reworked the stanzas and the major change here is to link the version to a latest link so that the PATH does not need changed every time
354a018 to
706f82c
Compare
|
The change I'm submitting is just for the versions of sqlcl that are coming in. as it comes in with a version. I wanted the latest link to show where this would be so the the user only ever has to fix the path once and every autoupdate would fix the latest link to point to the install. |
|
@bamcgill Why do you continue to ignore the feedback I provided in December and here a couple of weeks ago? Other package managers are linking this package as |
|
@bevanjkay I dont think i am ignoring it. It’s still going to be sqlcl. The change is about the version in the cask when it’s installed. it’s now just adding a link called latest pointing at the version just installed. Help me if im missing something really obvious |
| stage_only true | ||
|
|
||
| zap trash: "~/.sqlcl" | ||
| postflight do | ||
| cask_dir = Pathname("#{HOMEBREW_PREFIX}/Caskroom/sqlcl") | ||
| version_dir = cask_dir/version | ||
| latest_dir = cask_dir/"latest" | ||
|
|
||
| caveats do | ||
| depends_on_java "11+" | ||
| path_environment_variable "#{staged_path}/sqlcl/bin" | ||
| latest_dir.delete if latest_dir.symlink? | ||
|
|
||
| latest_dir.make_symlink(version_dir) | ||
| end |
There was a problem hiding this comment.
| stage_only true | |
| zap trash: "~/.sqlcl" | |
| postflight do | |
| cask_dir = Pathname("#{HOMEBREW_PREFIX}/Caskroom/sqlcl") | |
| version_dir = cask_dir/version | |
| latest_dir = cask_dir/"latest" | |
| caveats do | |
| depends_on_java "11+" | |
| path_environment_variable "#{staged_path}/sqlcl/bin" | |
| latest_dir.delete if latest_dir.symlink? | |
| latest_dir.make_symlink(version_dir) | |
| end | |
| binary "sqlcl/bin/sql", target: "sqlcl" |
This should allow us to avoid the entire postflight block, and the need to add the binary to path at all.
There was a problem hiding this comment.
That would be good but unfortunately, thats not how we ship the code everywhere else. It’s known everywhere as a sql prompt. I’m open to other suggestions. Ultimately, even this way, we;d be adding a soft link to link it to sql or telling customers to add a link to link sqlcl to sql in their environment
There was a problem hiding this comment.
Maybe when it is shipped directly from the developer, but the current cask file results in a bad user experience because it is not compatible with Homebrew API. We really shouldn't be shipping custom logic like this in homebrew-cask.
Regarding how the code is shipped "everywhere else".
It's shipped as sqlcl by nixpkg - https://github.com/NixOS/nixpkgs/blob/nixos-unstable/pkgs/by-name/sq/sqlcl/package.nix
It's shipped as both sql and sqlcl by AUR - https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=sqlcl
It's shipped as sqlcl by slackbuilds - https://slackbuilds.org/slackbuilds/15.0/development/sqlcl/sqlcl.SlackBuild
All this to say that installing sqlcl as sqlcl is a common way to install the binary with a Package Manager.
|
@bamcgill I have included the exact code suggestions above, I hope this helps with understanding what I'm talking about here. |
Ok, I can accept that. Thanks @bevanjkay Co-authored-by: Bevan Kay <bevanjkay@gmail.com>
Accepting this as well @bevanjkay Co-authored-by: Bevan Kay <bevanjkay@gmail.com>
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Important: Do not tick a checkbox if you haven’t performed its action. Honesty is indispensable for a smooth review process.
Updating sqlcl so that we always reference the latest install so a user does not have to set an environment variable each time.
In the following questions
<cask>is the token of the cask you're submitting.After making any changes to a cask, existing or new, verify:
brew audit --cask --online <cask>is error-free.brew style --fix <cask>reports no offenses.Additionally, if adding a new cask:
brew audit --cask --new <cask>worked successfully.HOMEBREW_NO_INSTALL_FROM_API=1 brew install --cask <cask>worked successfully.brew uninstall --cask <cask>worked successfully.If AI was used to generate or assist with generating the PR:
zapstanza paths.