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

Replace pkg-config with pkgconf when the distribution is brew #26891

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

mseri
Copy link
Member

@mseri mseri commented Nov 14, 2024

Import #26878 with only the latest pkg-config-file updated (in case there are users still using older copies of homebrew)

emjin and others added 4 commits November 13, 2024 16:25
installs pkgconf. Accordingly, this PR adjusts the expectations
of `conf-pkg-config` to depend on `pkgconf`.
@mseri
Copy link
Member Author

mseri commented Nov 14, 2024

This was already tested

@hannesm
Copy link
Member

hannesm commented Nov 14, 2024

First of all, thanks for your work on the opam-repository maintenance :)

Hmm, I'd have expected to bump conf-pkg-config in the version number... now it is unclear when someone reports a failure in conf-pkg-config.3 what their brew depext candidate is/was.

Furthermore, from my brief understanding, a recompilation for everybody using conf-pkg-config could have been avoided by bumping the version number (conf-pkg-config.3.1) and mark that as available: ... = os = "macos" & os-distribution = "homebrew"}.

But I may be wrong, I'm a bit unhappy about unversioned conf-* package upgrades in general (adding distributions is usually not an issue, but modifying existing filters etc. is dangerous - I remember some breakage in the past), but if this is considered to be a good idea, please go ahead.

@mseri
Copy link
Member Author

mseri commented Nov 14, 2024 via email

@mseri
Copy link
Member Author

mseri commented Nov 14, 2024

Furthermore, from my brief understanding, a recompilation for everybody using conf-pkg-config could have been avoided by bumping the version number (conf-pkg-config.3.1) and mark that as available: ... = os = "macos" & os-distribution = "homebrew"}.

I thought with recent opams this should not happen unless you are on the system that was affected by the change. But I am absolutely not sure about it 😅

@SkySkimmer
Copy link
Contributor

This seems to have broken our CI and the fix I found looks a bit strange https://github.com/coq/coq/pull/19836/files
Are we doing something wrong?

@hhugo
Copy link
Contributor

hhugo commented Nov 19, 2024

I'm seeing failure that look related to this. https://github.com/ocsigen/js_of_ocaml/actions/runs/11914383047/job/33202182023
@mseri, any help to fix this would be welcome

@mseri
Copy link
Member Author

mseri commented Nov 19, 2024

The github runners have pkg-config, the old homebrew package, preinstalled and it conflicts with the new one. What's worse, is that unlinking the old one allows the new one to be installed but the symlink that they create to pkg-config seems to not work on the runners.

There are two options that I have seen working so far:

I am going to suggest an updated conf- package that should work with just brew unlink pkg-config, but there is no guarantee that that will work out of the box on the GitHub runners until they fix this (in homebrew or in the runners)

EDIT: updated with @hhugo workaround

@mseri
Copy link
Member Author

mseri commented Nov 19, 2024

I suggested a new release, but it will not completely solve the problem. The runners are using an outdated package that conflicts with the new one and it seems that they have an issue with the symlink created by the new package (it is not found by opam)

@hhugo
Copy link
Contributor

hhugo commented Nov 20, 2024

I've fixed my issue with ocsigen/js_of_ocaml#1737

@hhugo
Copy link
Contributor

hhugo commented Nov 25, 2024

I've fixed my issue with ocsigen/js_of_ocaml#1737

The jsoo CI error-ed out again.
Fixed with

          brew update
          brew upgrade
          brew unlink [email protected]
          brew install pkgconf

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.

5 participants