Skip to content

cmake: fix fish install directory detection via pkg-config #17147

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

Closed
wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

Follow-up to c8b0f0c #16833

@vszakats vszakats changed the title cmake: fix fish install directory detection via pkg-config cmake: fix fish install directory detection via pkg-config Apr 23, 2025
@github-actions github-actions bot added the build label Apr 23, 2025
@testclutch
Copy link

Analysis of PR #17147 at b05fe9ae:

Test 1452 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test 504 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Generated by Testclutch

@vszakats
Copy link
Member Author

Analysis of PR #17147 at b05fe9ae:

Test 1452 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test 504 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Generated by Testclutch

@dfandrich Dan, here's the first live example (I've seen) for a failed test going green after a retry: https://github.com/curl/curl/actions/runs/14614053753/job/40998187449#check-step-16

@vszakats vszakats closed this in 73b6f8d Apr 23, 2025
@vszakats vszakats deleted the cm-fish-sysdir-detection-fix branch April 23, 2025 09:35
@vszakats
Copy link
Member Author

vszakats commented Apr 23, 2025

Merged this, but realized that installing directly in a system directory may be unexpected.
Also on macOS this installs the completion script to:

-- Installing: /opt/homebrew/Cellar/fish/4.0.2/share/fish/vendor_completions.d/curl.fish

Ref: https://github.com/curl/curl/actions/runs/14614053753/job/40998191292?pr=17147#step:13:521

Instead of /opt/homebrew/share/fish/vendor_completions.d (as in __fish_build_paths.fish)

It means completions are lost after each fish update via Homebrew.

(This replicates the logic found in autotools.)

vszakats added a commit that referenced this pull request Apr 24, 2025
Also:
- omit auto-detecting `CURL_COMPLETION_FISH_DIR` via `pkg-config`
  for cross-builds and when `CMAKE_INSTALL_PREFIX` is set.
- flatten nested `if`s.

Note:
On macOS with Homebrew, `pkg-config --variable completionsdir fish`
returns the version-specific Cellar path instead of the permanent path
`/opt/homebrew/share/fish/vendor_completions.d/`. This mimics what
autotools does, but may need further fixing, possibly upstream.
https://github.com/Homebrew/homebrew-core/blob/9c13e62b009b8e814fda180e0fcc5096318daf31/Formula/f/fish.rb
https://github.com/fish-shell/fish-shell/blob/ce631fd2fb1f5b63f5f0f1b4041a30dfad823d22/cmake/Install.cmake#L15-L21

Ref: #17147
Ref: 51170b5 #17159
Bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1103938

Closes #17145
nbaws pushed a commit to nbaws/curl that referenced this pull request Apr 26, 2025
nbaws pushed a commit to nbaws/curl that referenced this pull request Apr 26, 2025
Also:
- omit auto-detecting `CURL_COMPLETION_FISH_DIR` via `pkg-config`
  for cross-builds and when `CMAKE_INSTALL_PREFIX` is set.
- flatten nested `if`s.

Note:
On macOS with Homebrew, `pkg-config --variable completionsdir fish`
returns the version-specific Cellar path instead of the permanent path
`/opt/homebrew/share/fish/vendor_completions.d/`. This mimics what
autotools does, but may need further fixing, possibly upstream.
https://github.com/Homebrew/homebrew-core/blob/9c13e62b009b8e814fda180e0fcc5096318daf31/Formula/f/fish.rb
https://github.com/fish-shell/fish-shell/blob/ce631fd2fb1f5b63f5f0f1b4041a30dfad823d22/cmake/Install.cmake#L15-L21

Ref: curl#17147
Ref: 51170b5 curl#17159
Bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1103938

Closes curl#17145
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants