Skip to content

Don't assume brew is available in the system #442

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

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

Conversation

davidmh
Copy link

@davidmh davidmh commented Feb 23, 2025

I'm working on a project that uses nix as a package manager through a flake file. I can add tfenv through a custom derivation, which can include the required gnugrep dependency in the definition.

But the project assumes that the GNU version would be installed with Homebrew, which might not be installed at all.

Since all we care about is that GNU grep is available, we should use grep --version instead.

Zordrak
Zordrak previously approved these changes Jul 4, 2025
@Zordrak Zordrak force-pushed the fix/package-agnostic-dep-check branch 2 times, most recently from 965bf03 to bd086f0 Compare July 4, 2025 13:16
@Zordrak
Copy link
Collaborator

Zordrak commented Jul 4, 2025

Tests are failing. Might get a colleague that has a Mac to look into why if I can - but test failure needs resolving before PR can be merged.

All we care about is that GNU grep is available, which could've been
installed in a number of ways, not just via Homebrew.
@Zordrak Zordrak force-pushed the fix/package-agnostic-dep-check branch from bd086f0 to 4695cb7 Compare July 4, 2025 14:46
@davidmh
Copy link
Author

davidmh commented Jul 4, 2025

Hey @Zordrak, I think I've fixed the issue, see the commit message for the details.

@davidmh davidmh force-pushed the fix/package-agnostic-dep-check branch from 760670e to 8b13235 Compare July 4, 2025 21:20
When we find a brew-installed grep, we create an alias from ggrep to
grep, but an alias can't be defined and used in the same parsing unit,
so we exit early instead.

See: https://www.shellcheck.net/wiki/SC2262
@davidmh davidmh force-pushed the fix/package-agnostic-dep-check branch from 8b13235 to d57fdd5 Compare July 14, 2025 19:01
@davidmh
Copy link
Author

davidmh commented Jul 14, 2025

@Zordrak I mistakenly used an exit instead of a return to do the early exit in the check_dependencies function. I've verified the return does work in my fork: https://github.com/davidmh/tfenv/actions/runs/16275337735

You can see the diff here: https://github.com/tfutils/tfenv/compare/8b13235509c7521d9984c4a66f0b53262ea394de..d57fdd50b49a1c5852ce51f6f2ca1f3b15c83a60

Sorry for the thrash

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