Fix incompatibility in brew shellenv with older version of Fish shell

[smsearcy]

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

---

Fish didn't support $(...) substitution until v3.4.0, but using (...) didn't produce expected output so I rewrote the Fish command for MANPATH (and updated INFOPATH so that it doesn't add duplicate entries).

I didn't have any issues until a recent brew update, I think the issue was introduced here.

This is the error that was happening on Fish 3.3.1 (Ubuntu 22.04 LTS):

❯ eval (/home/linuxbrew/.linuxbrew/bin/brew shellenv)
fish: $(...) is not supported. In fish, please use '(string)'.
set -gx HOMEBREW_PREFIX "/home/linuxbrew/.linuxbrew"; set -gx HOMEBREW_CELLAR "/home/linuxbrew/.linuxbrew/Cellar"; set -gx HOMEBREW_REPOSITORY "/home/linuxbrew/.linuxbrew/Homebrew"; fish_add_path -gP "/home/linuxbrew/.linuxbrew/bin" "/home/linuxbrew/.linuxbrew/sbin"; set -q MANPATH; and set MANPATH[1] ":$(string trim --left --chars=":" $MANPATH[1])"; ! set -q INFOPATH; and set INFOPATH ''; set -gx INFOPATH "/home/linuxbrew/.linuxbrew/share/info" $INFOPATH;
                                                                                                                                                                                                                                                                                                                 ^

[smsearcy]

Two tests failed when running brew tests locally, but I don't think either had to do with my change:

Failures:

  1) Caveats#caveats when f.keg_only is not nil when joining different caveat types together adds the correct amount of new lines to the output
     Failure/Error: expect(caveats.count("\n")).to eq(9)

       expected: 9
            got: 11

       (compared using ==)
     # ./test/caveats_spec.rb:234:in `block (5 levels) in <top (required)>'
..........
.......................................................

Failures:

  1) Hardware::CPU::family returns the current CPU's family name as a symbol, or :dunno if it cannot be detected
     Failure/Error: expect(cpu_families).to include described_class.family

       expected [:amd_k7, :amd_k8, :amd_k8_k10_hybrid, :amd_k10, :amd_k12, :arm, :arm_blizzard_avalanche, :arm_firestorm_icestorm, :arm_hurricane_zephyr, :arm_ibiza, :arm_lightning_thunder, :arm_lobos, :arm_monsoon_mistral, :arm_palma, :arm_twister, :arm_typhoon, :arm_vortex_tempest, :atom, :bobcat, :broadwell, :bulldozer, :cannonlake, :cometlake, :core, :core2, :dothan, :haswell, :icelake, :ivybridge, :jaguar, :kabylake, :merom, :nehalem, :penryn, :ppc, :prescott, :presler, :sandybridge, :skylake, :westmere, :zen, :zen3, :dunno] to include :unknown_0x6_0xb7
       Diff:
       @@ -1,43 +1,85 @@
       -[:unknown_0x6_0xb7]
       +[:amd_k7,
       + :amd_k8,
       + :amd_k8_k10_hybrid,
       + :amd_k10,
       + :amd_k12,
       + :arm,
       + :arm_blizzard_avalanche,
       + :arm_firestorm_icestorm,
       + :arm_hurricane_zephyr,
       + :arm_ibiza,
       + :arm_lightning_thunder,
       + :arm_lobos,
       + :arm_monsoon_mistral,
       + :arm_palma,
       + :arm_twister,
       + :arm_typhoon,
       + :arm_vortex_tempest,
       + :atom,
       + :bobcat,
       + :broadwell,
       + :bulldozer,
       + :cannonlake,
       + :cometlake,
       + :core,
       + :core2,
       + :dothan,
       + :haswell,
       + :icelake,
       + :ivybridge,
       + :jaguar,
       + :kabylake,
       + :merom,
       + :nehalem,
       + :penryn,
       + :ppc,
       + :prescott,
       + :presler,
       + :sandybridge,
       + :skylake,
       + :westmere,
       + :zen,
       + :zen3,
       + :dunno]

     # ./test/hardware/cpu_spec.rb:71:in `block (3 levels) in <top (required)>'



Took 10 seconds
Tests Failed

[smsearcy]

Oops, this isn't ready to merge. While running eval (./bin/brew shellenv) didn't cause an error, I don't think the data is actually correct:

❯ eval (./bin/brew shellenv)

❯ echo $MANPATH
 (string trim --left --chars=  ) (string trim --left --chars=  ) (string trim --left --chars=  /home/linuxbrew/.linuxbrew/share/man)

[smsearcy]

The Fish script for updating the MANPATH was not correct (see previous comment), so I updated it (and INFOPATH) to use the contains test to only prepend the path if not already present.

Example output running locally:

brew on  fix-fish-shellenv 
❯ set -e MANPATH

brew on  fix-fish-shellenv [!] 
❯ echo $MANPATH


brew on  fix-fish-shellenv [!] 
❯ eval (./bin/brew shellenv)

brew on  fix-fish-shellenv [!] 
❯ echo $MANPATH
/home/smsearcy/Projects/brew/share/man

brew on  fix-fish-shellenv [!] 
❯ eval (./bin/brew shellenv)

brew on  fix-fish-shellenv [!] 
❯ echo $MANPATH
/home/smsearcy/Projects/brew/share/man

I didn't see any CI tests for brew shellenv, I'm assuming it would be possible to setup something with GitHub Actions but have not looked into that.

[gromgit]

@smsearcy, your fix breaks #17633. Please put the initial colon back into MANPATH, and remove Homebrew's man dir.

% fish
Welcome to fish, the friendly interactive shell
Type help for instructions on how to use fish

/o/homebrew (fix-fish-shellenv)> eval (/opt/homebrew/bin/brew shellenv)

/o/homebrew (fix-fish-shellenv)> env | grep MANPATH
MANPATH=/opt/homebrew/share/man:/Library/Developer/CommandLineTools/usr/share/man:/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/share/man

[smsearcy]

@gromgit, thank you for the feedback. I think I came up with a working solution, but have a question. If MANPATH is not defined or an empty string, does it need to be modified?

Edit: I noticed a comment on #17633 that speculated Fish shell wasn't affected, in which case I would make different changes.

[smsearcy]

@gromgit, I pushed some new changes for further testing. This prepends and empty string when MANPATH has content, triggering a leading colon in env. My MANPATH is empty, and after running mandb I can view man pages installed via Homebrew (so I don't think a colon is necessary when MANPATH is an empty string).

brew on  fix-fish-shellenv 
❯ set -gx MANPATH 'testing'

brew on  fix-fish-shellenv 
❯ env | grep MANPATH
MANPATH=testing

brew on  fix-fish-shellenv 
❯ eval (./bin/brew shellenv)

brew on  fix-fish-shellenv 
❯ env | grep MANPATH
MANPATH=:testing

brew on  fix-fish-shellenv 
❯ eval (./bin/brew shellenv)

brew on  fix-fish-shellenv 
❯ env | grep MANPATH
MANPATH=:testing

[gromgit]

Yes, the correct MANPATH logic is as follows:

• if it's empty, do nothing
• if it's not empty, ensure it has a leading colon

[MikeMcQuaid]

@smsearcy thanks for the PR!

@gromgit can you confirm you're ✅ on the current version? Want to ensure you are before it's approved.

[gromgit]

I'm not a regular fish user, but it passes all the obvious manual tests (including running shellenv multiple times--it's idempotent w.r.t MANPATH), so I'm ✅ on the current PR.

Thanks much, @smsearcy!

[MikeMcQuaid]

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @smsearcy!