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

Prepare for LLVM 18, link to local repo for scripts and patches #27178

Merged
merged 9 commits into from
Mar 3, 2025

Conversation

alan-j-hu
Copy link
Contributor

This references the scripts and patches for my fork of opam-source-archive, just so I can see if things work on CI... If this is accepted, they will need to be changed to point to ocaml/opam-source-archive.

Context: ocaml/opam-source-archives#40

@alan-j-hu
Copy link
Contributor Author

alan-j-hu commented Dec 24, 2024

I'm getting cascading errors because of this:

Error in conf-llvm-static.18: error 46: Package is flagged "conf" but has source, install or remove instructions
Error in conf-llvm-static.18: No package source directory provided.
Error in conf-llvm-shared.18: error 46: Package is flagged "conf" but has source, install or remove instructions
Error in conf-llvm-shared.18: No package source directory provided.

This is despite the fact that this all worked on my computer testing locally, and conf-llvm.17 and prior all have build fields and were accepted. However, today when I run opam lint on conf-llvm.17, I get this error. Is this something new?

@alan-j-hu
Copy link
Contributor Author

Okay, I misunderstood the CI error message.

@alan-j-hu alan-j-hu marked this pull request as draft December 25, 2024 04:07
@alan-j-hu alan-j-hu marked this pull request as ready for review January 17, 2025 19:22
@alan-j-hu
Copy link
Contributor Author

I'm changing this PR from "draft" to "ready to review" because the opam maintainers have not seen my PR over at the opam-source-archives repository. They may have only glanced at this one, and misunderstood its status because of the "draft" state.

I need the opam maintainers to look at this PR. However, as it stands, it points at scripts on my fork of opam-source-archives. Therefore, my fork of opam-source-archives therefore needs to be merged first, so I can change the URLs on this PR.

@mseri
Copy link
Member

mseri commented Jan 17, 2025

You can link the opam-source-archive patches and files now, thanks!

@alan-j-hu
Copy link
Contributor Author

I have changed the links now.

@alan-j-hu
Copy link
Contributor Author

Hold on, don't merge this yet. I think there might be a problem with bytecode executables, which I need to check out.

@alan-j-hu
Copy link
Contributor Author

Bytecode executables were not working. (I'm not sure if they were even working before, as bytecode executables do not compile under the LLVM 14 package for a different reason.)

To fix, please merge ocaml/opam-source-archives#42 first, and then merge this.

@alan-j-hu
Copy link
Contributor Author

Argh, I spoke too soon. The bytecode executable seems to work, but I think I still made a mistake in the patch, which I'm checking out right now.

After the creation of the opam-source-archives repository, it's really, really difficult to support packages that depend on files in the opam-repository, because I need to move between two repositories and make two merge requests, leaving a lot of room for mistakes.

@alan-j-hu
Copy link
Contributor Author

Okay, ocaml/opam-source-archives#42 should be ready to merge, then this can be merged after.

Right now, I have four projects open:

  • opam-repository
  • opam-source-archives
  • llvm-project
  • My OCaml LLVM test project to make sure the LLVM package works

I need to make sure that the llvm-project test suite passes, and that the LLVM bindings also work properly when installed out-of-tree. I realized that bytecode executables didn't work when the -custom flag was passed (as it was), and set out to fix the issue, which is what the commotion was about.

@alan-j-hu
Copy link
Contributor Author

AFAIK, in LLVM 14 (the last version opam to use CMake to build), bytecode compilation wasn't working, because of a different issue that has since been fixed: https://discuss.ocaml.org/t/llvm-symbol-not-found-for-bytecode-compilation/8728. Now, I would like to pay closer attention to this package to make sure all the moving parts work.

@mseri
Copy link
Member

mseri commented Jan 25, 2025

Thanks for the effort!

By the way, you don’t need the opam-source-archive until the patch is ready. It could live on your repo or in a gist for example, giving you fast iteration, and once everything is ready and tested, we can move it to the archive

}
extra-source "AddOCaml.cmake.patch" {
src:
"https://raw.githubusercontent.com/ocaml/opam-source-archives/main/patches/llvm/AddOCaml.cmake.patch.18"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"https://raw.githubusercontent.com/ocaml/opam-source-archives/main/patches/llvm/AddOCaml.cmake.patch.18"
"https://github.com/alan-j-hu/opam-source-archives/raw/e5d49c1c15149d3a239d237634f5fc85d903718d/patches/llvm/AddOCaml.cmake.patch.18"

to iterate on the patch and test it while you are working on it, this or any other remote location would be fine

}
extra-source "AddOCaml.cmake.patch" {
src:
"https://raw.githubusercontent.com/ocaml/opam-source-archives/refs/heads/main/patches/llvm/AddOCaml.cmake.patch.18"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"https://raw.githubusercontent.com/ocaml/opam-source-archives/refs/heads/main/patches/llvm/AddOCaml.cmake.patch.18"
"https://github.com/alan-j-hu/opam-source-archives/raw/e5d49c1c15149d3a239d237634f5fc85d903718d/patches/llvm/AddOCaml.cmake.patch.18"

@Drup
Copy link
Contributor

Drup commented Feb 14, 2025

Just a gentle ping, because I also would like this to work. :)

@alan-j-hu
Copy link
Contributor Author

alan-j-hu commented Mar 1, 2025

Trying this again. It turns out that when I transferred the files from my https://github.com/alan-j-hu/llvm-ocaml-ci repo to the opam-source-archives repo, extra newlines got added at the end, so the checksums are different from the checksums listed in the opam files in the CI repo. From what I can tell, the functionality shouldn't change...

@alan-j-hu
Copy link
Contributor Author

https://github.com/alan-j-hu/llvm-ocaml-ci I updated my CI repo to match the hashes, and can confirm that the build still works.

@mseri
Copy link
Member

mseri commented Mar 1, 2025

Every now and then it fails with

# [  0%] Building OCaml library llvm
# ld: error: unable to find library -lzstd
# cc: error: linker command failed with exit code 1 (use -v to see invocation)

but somehow inconsistently. I see the failure on the conf package, and later on the package itself (using this conf) builds fine. Do you know why?

I am rerunning some of the builds to double check

@alan-j-hu
Copy link
Contributor Author

alan-j-hu commented Mar 1, 2025

Ugh, first this is a problem with llvm-config. It's just hardcoded to output -lzstd, but it doesn't add any flags to look for zstd in the right paths. I searched online and see similar issues at ziglang/zig#19617 and rust-lang/rust#135503.

Furthermore, Homebrew started installing libraries under /'opt/homebrew instead of /usr/local under Silicon Macs, breaking stuff for a lot of people. It looks like the Mac build works on Amd64, but not on Arm64 (i.e. Silicon Macs). I had assumed my patch would handle both cases, because I use the HOMEBREW_PREFIX environment variable. However, I wonder if HOMEBREW_PREFIX is not being set to /opt/homebrew in the Arm64 build.

I'm not sure of a good way to fix these issues.

I'm not familiar with FreeBSD, or its package manager/repositories. I wonder if FreeBSD requires some zstd depext?

@alan-j-hu
Copy link
Contributor Author

I wonder if the LLVM.18-shared package will build on Silicon Macs so long as the user properly sets the HOMEBREW_PREFIX environment variable prior to installing.

@alan-j-hu
Copy link
Contributor Author

I tried adding "zstd" as a depext to conf-llvm, but the FreeBSD CI still failed, so looks like that wasn't the issue.

@mseri
Copy link
Member

mseri commented Mar 2, 2025

Let's leave it as is for now and merge. This way homebrew and freebsd users will have a chance to try it out and see if they can help us with a fix. What do you think?

@alan-j-hu
Copy link
Contributor Author

Ok, sounds good to me.

@mseri mseri merged commit ec92f53 into ocaml:master Mar 3, 2025
0 of 2 checks passed
@mseri
Copy link
Member

mseri commented Mar 3, 2025

Thanks for such an amount of wark and your patience with all my requests 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants