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

Release patched LLVM 15 #24093

Closed
wants to merge 7 commits into from
Closed

Release patched LLVM 15 #24093

wants to merge 7 commits into from

Conversation

alan-j-hu
Copy link
Contributor

@alan-j-hu alan-j-hu commented Jul 11, 2023

I talked with @jberdine and we think that getting the LLVM 15 fork with the OCaml 5 support released on OPAM is more important than details of specific build systems. We think that LLVM 15 should just be released now in the same manner that LLVM 14 was.

My personal feeling after looking into Dune is that:

  • Integrating Dune into LLVM upstream will make life harder for the predominantly C++ programmers maintaining LLVM. Therefore, I don't see a path going forward that would transition away from a separate Dune LLVM repo.
  • Having the option of dynamic versus static linking to LLVM is much hackier with Dune than with OCamlfind. In general, I think Dune is opinionated in a way that is not good for multi-language projects. Perhaps this should be feedback for the Dune developers.

The ball is in the court of @kit-ty-kate to decide whether to accept this PR. Please let me know if you have any disagreement or concerns.

Copy link
Contributor

@jberdine jberdine left a comment

Choose a reason for hiding this comment

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

I seem to recall that the conf-llvm package only needs to have the right major version, and the rest are superfluous. I don't remember where in the code I got that impression from though. It might be tidier to release conf-llvm.15 instead.

packages/conf-llvm/conf-llvm.15.0.7/files/configure.sh Outdated Show resolved Hide resolved
["llvm15-dev"] {os-distribution = "alpine"}
["llvm"] {os-family = "arch"}
["llvm15-devel"] {os-family = "suse"}
["llvm15-devel"] {os-distribution = "fedora"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@alan-j-hu alan-j-hu Jul 12, 2023

Choose a reason for hiding this comment

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

I do not know, as I am not a Fedora user. I am just copying over the naming convention from conf-llvm.14. Does the naming on that webpage, llvm-devel-15.0.0-1.fc37, imply that I would type dnf install llvm-devel-15 to install the package on Fedora?

I also found https://packages.fedoraproject.org/pkgs/llvm15/llvm15-devel/index.html.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not familiar with the fedora naming scheme either, but am just guessing from the log of a failing CI job that says Error: Unable to find a match: llvm15-devel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CI seems to use Fedora 36. According to https://pkgs.org/download/llvm-devel, Fedora 26 has llvm-devel-14. On the other hand, https://pkgs.org/download/llvm15-devel says that llvm15-devel is available for Fedora 38 but not Fedora 36.

"conf-bash" {build}
]
depexts: [
["llvm@15"] {os-distribution = "homebrew" & os = "macos"}
Copy link
Contributor

Choose a reason for hiding this comment

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

The macos-homebrew-*-arm64 CI jobs fail with ld: library not found for -lzstd but the llvm@15 homebrew package does require zstd, so the failure is odd. I don't know if there is something nonstandard about the CI worker setup.

Accept @jberdine's fix

Co-authored-by: Josh Berdine <josh@berdine.net>
@alan-j-hu
Copy link
Contributor Author

The relevant change being backported is https://reviews.llvm.org/D136400. My fork of LLVM also fixes various functions that were removed between LLVM 15 and LLVM 16 (and therefore not included in the linked patch) to work with OCaml 5. In addition, my fork of LLVM 15 adds deprecation annotations to various functions that were replaced between LLVM 15 and LLVM 16.

Compared to LLVM 14, the opam package file in this PR removes the conflicts with nnp and changes the tarball URL and hash.

@Kakadu
Copy link
Contributor

Kakadu commented Jul 17, 2023

@alan-j-hu Could you remind me, if there any easy ways to test this package without disabling the whole default opam repository? I have some trouble with https://discuss.ocaml.org/t/dynlinking-plugin-which-references-llvm-stubs/12596 and it would be great, if I could test LLVM 15.

@alan-j-hu
Copy link
Contributor Author

alan-j-hu commented Jul 18, 2023

@Kakadu The LLVM OCaml bindings README gives some instructions on how to install the bindings while linking to the already-installed LLVM: https://github.com/llvm/llvm-project/tree/main/llvm/bindings/ocaml

However, I'm not too experienced with this. I just tested the bindings by running the LLVM tests. IIRC CMake actually installed some of the files in the wrong place in the .opam folder and I had to manually move them, but I don't remember the details.

You can also try adding this branch of my fork as another opam repository, as seen with the example here: https://github.com/janestreet/opam-repository. However, I've never done this before.

@Kakadu
Copy link
Contributor

Kakadu commented Jul 20, 2023

You can also try adding this branch of my fork as another opam repository, as seen with the example here: https://github.com/janestreet/opam-repository. However, I've never done this before.

Build is currently failing:

#=== ERROR while compiling conf-llvm.15.0.7 ===================================#
Bad hash for   - /home/opam/.opam/repo/default/packages/conf-llvm/conf-llvm.15.0.7/files/configure.sh

@Kakadu
Copy link
Contributor

Kakadu commented Jul 22, 2023

It seem that "%{conf-llvm:config}%" is being expanded into empty string on my machine. I don't understand why this variable should have non-empty value. Is new release of llvm depends on minimal version of OPAM or something? I'm using 2.1.2

➜  asp opam install llvm.15.0.7 --verbose -y                                                        llvm15
The following actions will be performed:
∗ install llvm 15.0.7

<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
⬇ retrieved llvm.15.0.7  (cached)
[llvm: patch] applying fix-shared.patch
[llvm: patch] applying fix-macos.patch
[llvm: patch] applying fix-rhel.patch
+ /mnt/mand/.opam/opam-init/hooks/sandbox.sh "build" "bash" "-ex" "install.sh" "" "/mnt/mand/.opam/llvm15/lib" "cmake" "make" "build" (CWD=/mnt/mand/.opam/llvm15/.opam-switch/build/llvm.15.0.7)
- + llvm_config=
- + libdir=/mnt/mand/.opam/llvm15/lib
- + cmake=cmake
- + make=make
- + action=build
- + case "$action" in
- + '' --link-static --libs
- install.sh: line 55: : command not found
- + '' --link-static --libs
- install.sh: line 59: : command not found
- + '' --link-shared --libs
- install.sh: line 62: : command not found
- + echo 'Link mode not recognized'
- Link mode not recognized
- + exit 1
[ERROR] The compilation of llvm.15.0.7 failed at "bash -ex install.sh  /mnt/mand/.opam/llvm15/lib cmake
make build".

@kit-ty-kate
Copy link
Member

Superseded by #24268

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.

4 participants