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

feat(just): add brew-command-not-found setup #1874

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tulilirockz
Copy link
Collaborator

@tulilirockz tulilirockz commented Nov 3, 2024

This should setup command-not-found alongside brew-setup. The only thing missing is fish support that I couldnt set up yet (idk why it isnt working). Otherwise this seems good? Just needs a bit more testing. - Fixes #1327

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. brew Issues related to brew.sh integration size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Nov 3, 2024
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Nov 5, 2024
@m2Giles
Copy link
Member

m2Giles commented Nov 10, 2024

I think this might be a bit too much. Especially if this is going to install something from brew automatically. While brew is present, it is very much in an opt-in configuration.

@tulilirockz
Copy link
Collaborator Author

This probably should just be a justfile, right? Ill convert it over to that!

@tulilirockz tulilirockz force-pushed the brew-command-not-found branch 2 times, most recently from 7e2cf61 to 7b8c124 Compare November 10, 2024 17:24
@tulilirockz tulilirockz changed the title feat(brew): add command-not-found by default after setup feat(just): add brew-command-not-found setup Nov 10, 2024
@tulilirockz
Copy link
Collaborator Author

There you go! It seems to work fine on my machine

Copy link

@lem4s lem4s left a comment

Choose a reason for hiding this comment

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

As it is often quite easy to misunderstand the intention of the review comments let me make it clear here: There are absolutely no rhetorical questions in my comments and all questions are well intended and out of curiosity of understanding your intentions and learning a few things ;)

BREW_BINARY=/home/linuxbrew/.linuxbrew/bin/brew
BREW_ROOT=${HOMEBREW_REPOSITORY:-$($BREW_BINARY --repository)}
if ! $BREW_BINARY -h > /dev/null; then
echo "Make sure Homebrew is first. Check journalctl -e -u brew-setup.service"
Copy link

Choose a reason for hiding this comment

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

Make sure Homebrew is installed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, placed it wrong LOL

Copy link

Choose a reason for hiding this comment

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

In this comment I actually meant that the first sentence in the echo is wrong, isn't it? ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OH! I didnt see that typo till now! Great catch!

just/bluefin-tools.just Outdated Show resolved Hide resolved
pkexec tee /etc/profile.d/brew-command-not-found.sh > /dev/null <<EOF
# Check for interactive bash or zsh and that we haven't already been sourced
if [[ -d /home/linuxbrew/.linuxbrew && \$- == *i* && BREW_COMMAND_NOT_FOUND != 1 ]] ; then
HB_CNF_HANDLER="${BREW_ROOT}/Library/Taps/homebrew/homebrew-command-not-found/handler.sh"
Copy link

@lem4s lem4s Nov 12, 2024

Choose a reason for hiding this comment

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

Maybe there is some magic going on here I don't understand but shouldn't you use the same logic as above: BREW_ROOT=${HOMEBREW_REPOSITORY:-$($BREW_BINARY --repository)} . Otherwise, how do you guarantee that this will work? At least on ghcr.io/ublue-os/bluefin:41-20241110 it seems unset.

Maybe this is the reason why fish is not working for you?
It would be awesome if we can get fish to work as that`s my preferred shell ❤️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the variable does not have a backslash before it it gets interpreted by the shell, in that code it just expands BREW_ROOT then writes the file, not the other way around

Copy link

Choose a reason for hiding this comment

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

Ah you are correct, it was quite late yesterday for me ;)
Then I think there could be some issue with hard-coding it here. Suppose the following:

  1. Somebody customized their homebrew installation and BREW_ROOT is set on their system.
  2. He/she then calls this just command. The path gets hard-coded to the customizations. Everything works fine.
  3. Later on the person decides to remove the customizations an remove the brew root under the custom path. This world break brew-command-not-found.

just/bluefin-tools.just Show resolved Hide resolved
just/bluefin-tools.just Outdated Show resolved Hide resolved
just/bluefin-tools.just Show resolved Hide resolved
just/bluefin-tools.just Outdated Show resolved Hide resolved
@tulilirockz
Copy link
Collaborator Author

As it is often quite easy to misunderstand the intention of the review comments let me make it clear here: There are absolutely no rhetorical questions in my comments and all questions are well intended and out of curiosity of understanding your intentions and learning a few things ;)

Most of the answers to your questions are: I just wrote bad code! Ill probably rewrite the just recipe with your review :p Thanks for reviewing!

@tulilirockz
Copy link
Collaborator Author

Im on my phone right now so I cant fix the recipe rn, but ill get on my computer, rewrite it and add some comments to clear up the logic

@lem4s
Copy link

lem4s commented Nov 13, 2024

Just added two comments on the review. Please check them. I can't seem to unresolve the issues or I am too blind to find the button... ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
brew Issues related to brew.sh integration size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use brew's command-not-found
3 participants