I want to make a small addition to the OCaml bindings for LLVM (adding a function that was added to the C API, but seemingly overlooked in OCaml).
I cloned LLVM, but my computer does not have much hard drive space, and attempting to build everything caused my computer to run out of space. I grepped the generated Makefile for “ocaml” and tried to build the ocaml_all and check-llvm-bindings-ocaml targets, but I don’t think those two targets are what I need, because when I purposely introduced a typo into my code, those targets did nothing to signal an error. What do I need to do to test my change (preferably without building stuff I don’t need and running out of space on my computer)?
Also, even though I only touched the OCaml-related files, when I ran git status, a bunch of other files were seemingly changed in the build process, and git diff crashed.
Now, I need to ping a reviewer from CODE_OWNERS.txt. I didn’t see anyone listed as an owner of the OCaml bindings, but Chris Lattner is listed as the code owner of anything not covered by someone else. I don’t think a noob like me should be pinging Chris Lattner to look at my first-time patch…
I figured out what I was doing wrong. I had nuked my OCaml installation, with all my packages, to make room for LLVM. I reinstalled the OCaml compiler and ocamlfind, but had neglected to install ctypes. As a result, when CMake made the makefile, it saw that I did not have ctypes and consequently did not give any logic for the OCaml targets. After installing ctypes and regenerating the makefile, a test failed… because I did not have ctypes.foreign installed! Now I’ve installed that and am rebuilding. The fact that the test failed lets me know that I am on the right track, as I am now sure that the build isn’t blindly accepting whatever code I have without running the tests I expect it to.
Update: Now, I intentionally made a typo in the name of the C function I’m binding from OCaml, and the test suite reported an error. So, I have greater confidence that my (non-typo’d) code is correct.
I had some fiddling around with build errors having to do with the Llvm_debuginfo module, but now I’ve updated my patch with tests!
According to Code Reviews with Phabricator — LLVM 16.0.0git documentation, In addition to pinging the code owner, I should find people who recently touched the part of the code I was working on to be code reviewers. Since I’m not sure who the code owner is, I’m going to do the latter…
I don’t think a noob like me should be pinging Chris Lattner to look at my first-time patch…
FWIW I don’t think he’d mind and maybe we could do with someone to own the OCaml stuff anyway.
LLVM has not historically had a complete set of code owners so adding recent authors in the area is totally fine.
I reinstalled the OCaml compiler and ocamlfind , but had neglected to install ctypes . As a result, when CMake made the makefile, it saw that I did not have ctypes and consequently did not give any logic for the OCaml targets. After installing ctypes and regenerating the makefile, a test failed… because I did not have ctypes.foreign installed!
Good work figuring this out. I have been stung by this in lldb where things are implicitly disabled.
For example for python bindings we have Auto, OFF, ON. Where ON gives an error if python is not found. Helps a lot flushing out these issues.
Would something like that have helped you here? Perhaps we can improve the options or the OCaml build documentation (if such documentation exists?). At least a list of dependencies somewhere would help.
Maybe after this patch is done, I can write about my experiences as a first-time contributor and the hiccups I faced.
One confusion I have right now is how to use Phabricator. I’m used to the Git workflow of making commits on a feature branch. I wasn’t too sure how to “update” a patch on Phabricator (or a “differential,” as Phabricator seemed to call it). Making a new commit, and then pushing it to Phabricator with arc diff --update would discard the +'s and -'s of the old diff and only add the +'s and -'s of the new commit, being made relative to the old one, which was very confusing. I figured out that I could update the patch by combining the two commits into one (e.g. by making a new branch, then squash merging into it). However, when reviewers left comments on specific lines of my diffs, those comments would always appear on the most updated version of the diff, and not the original version they were made on. This is confusing if I fix a mistake they point out, then update the diff, as their comments pointing out a problem then appear on the correct code.
Once a patch has been reviewed and approved on Phabricator it can then be committed to trunk. If you do not have commit access, someone has to commit the change for you (with attribution). It is sufficient to add a comment to the approved review indicating you cannot commit the patch yourself.
I just left a comment on the patch stating that I don’t have permission to commit the change to the main branch. I assume someone is going to come along to merge this?
Also, will this change appear in the next minor or patch release of LLVM 15, which adds opaque pointers, or will I have to wait for the next major version of LLVM?
Normally one of your reviewers will commit it in that case. Feel free to ping them if they don’t get round to it in a timely manner.
Not automatically, but you could ask your reviewer about getting it backported to the 15.x branch (which is a mostly automated process these days that takes very little effort). Given the nature of the change there shouldn’t be any risk of breaking things so that shouldn’t be controversial.
