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

Fix conf-openblas on macOS arm64 hardware #25076

Merged
merged 2 commits into from
Jan 24, 2024
Merged

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Jan 15, 2024

Possible fix for #25075 - working correctly on my M2 and didn't break a run in a Debian 12 opam container.

pkg-config requires additional configuration after installing the openblas package - it's detailed after installing the package that PKG_CONFIG_PATH must be updated for this to work.

The first commit fixes the test command to do exactly that; the second commit also adds the amendment to the output of opam env. Note that opam ignores FOO += "" in a setenv. The second commit is optional, but having installed openblas, it seems logical for opam to ensure that pkg-config then actually works.

@dra27 dra27 linked an issue Jan 15, 2024 that may be closed by this pull request
@dra27 dra27 changed the title Mac os conf openblas Fix conf-openblas on macOS arm64 hardware Jan 15, 2024
Copy link
Contributor

@tmcgilchrist tmcgilchrist left a comment

Choose a reason for hiding this comment

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

Tested with conf-pkg-config fix locally on both x86 and Arm MacOS.

pkg-config requires additional configuration after installing the
openblas package.
@mseri mseri merged commit 0a15710 into ocaml:master Jan 24, 2024
1 check failed
@pklehre
Copy link

pklehre commented Feb 7, 2024

I suspect this commit could be causing problems on my Debian machine. See this issue: #25194

@mseri
Copy link
Member

mseri commented Feb 7, 2024

That is surprising though, I wonder why we don’t see issues on debian in the ci

@pklehre
Copy link

pklehre commented Feb 7, 2024

Could it be that a critical environment variable is set to the empty string? The specific error message seems only to be triggered in line 216 of https://github.com/ocaml/opam/blob/master/src/state/opamEnv.ml, perhaps because the variable var is bound to an empty string?

@hannesm
Copy link
Member

hannesm commented Feb 7, 2024

I wonder why such a major semantic change (introducing some setenv business on all platforms) didn't impose a version bump of the conf- package? Is this in line with the best current practises of opam-repository (opam lock, append-only, ...)?

@mseri
Copy link
Member

mseri commented Feb 7, 2024

You are right, we should have made it with a -1 and never assume “it is safe”. If one of you can send a PR to revert this, I can make a new one where we require opam > 2.1.5 for this ariant package

@mseri
Copy link
Member

mseri commented Feb 7, 2024

I am not at my laptop until later in the afternoon, the best I can do is a fast merge and then a new PR later

@hannesm
Copy link
Member

hannesm commented Feb 7, 2024

@mseri revert at #25195 -- thanks for investigations and quick responses (to both @mseri and @pklehre)

@hannesm
Copy link
Member

hannesm commented Feb 7, 2024

-1 -- well, or since it is a conf-package, just bump it to 0.2.2 (or 0.3 or 1.0)? (read: I don't know anything about conf- package versioning)

@mseri
Copy link
Member

mseri commented Feb 7, 2024

That is something that at some point we should figure out. So far is mostly just .x, adding 1 to the version for large changes and appending -1 for smaller (whatever this means), but different packages have different versioning for different (sometimes historical but other times not) reasons 😳

@hannesm
Copy link
Member

hannesm commented Feb 7, 2024

Fine with me. My intuition is: adding "os" "distributions" to an existing conf- package doesn't deserve a version bump, but everything else does.

Is it annoying (i.e. recompiles on every client) if a depexts changes? I do not know (hopefully someone else does).

I remember #10531 still being open, but eventually there should be some concrete policies being written down (plus CI-checked).

@dra27
Copy link
Member Author

dra27 commented Feb 7, 2024

Oh dear, apologies for this.

I wonder why such a major semantic change (introducing some setenv business on all platforms) didn't impose a version bump of the conf- package? Is this in line with the best current practises of opam-repository (opam lock, append-only, ...)?

In limited defence, the point here is that it was not believed to be a semantic change affecting all packages. I had not registered that the second commit here relied on opam 2.0.11+ or 2.1.5+ to work correctly, as noted by my unfortunately incomplete comment "Note that opam ignores FOO += "" in a setenv".

I don't know the full history on conf version bumping either - in this PR, I incorrectly believed that the semantic change only fixed things. I'm not sure what the best thing here in terms of testing would be - if this had been put in a new version, we'd still have had all the breakage, it would have "just" been with a different version number.

I'll prepare a replacement PR in a sec.

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.

Installing conf-openblas 0.2.1 needs PKG_CONFIG_PATH extended.
5 participants