hashlink 1.11 (new formula) #53790
hashlink 1.11 (new formula) #53790
Conversation
Formula/hashlink.rb
Outdated
| desc "Virtual machine for Haxe" | ||
| homepage "https://hashlink.haxe.org/" | ||
| url "https://github.com/HaxeFoundation/hashlink.git", | ||
| :tag =>"1.11" |
There was a problem hiding this comment.
This needs a revision as well.
There was a problem hiding this comment.
I can change this, but it's strange that :tag is permitted at all in that case, since it could conflict with :revision.
There was a problem hiding this comment.
It wouldn't, tags can be recreated. The revision makes sure that doesn't happen without us knowing about it.
Formula/hashlink.rb
Outdated
| end | ||
|
|
||
| test do | ||
| system "hl", "--version" |
There was a problem hiding this comment.
We need a test that exercises the some of the functionality of the app. Version checks or usage checks (foo --version or foo --help) are not sufficient, as explained in the formula cookbook.
In most cases, a good test would involve running a simple test case: run #{bin}/foo input.txt.
- Then you can check that the output is as expected (with assert_equal or assert_match on the output of shell_output)
- You can also check that an output file was created, if that is expected: assert_predicate testpath/"output.txt", :exist?
Some advice for specific cases:
- If the formula is a library, compile and run some simple code that links against it. It could be taken from upstream's documentation / source examples.
- If the formula is for a GUI program, try to find some function that runs as command-line only, like a format conversion, reading or displaying a config file, etc.
- If the software cannot function without credentials, a test could be to try to connect with invalid credentials (or without credentials) and confirm that it fails as expected.
- Same if the software requires a virtual machine, docker instance, etc. to be running.
There was a problem hiding this comment.
HL is a bytecode runtime, so none of the suggestions apply. I'm also not seeing helpful examples in any of the more popular runtimes. What would be a good test for a bytecode interpreter? I tried embedding bytecode in the ruby file with no luck.
There was a problem hiding this comment.
E.g., I could create a small "hello world" example, but it would need to be in a separate file, and I'm not seeing anything analogous to this approach in homebrew-core.
There was a problem hiding this comment.
You could download an example from somewhere using a resource block
|
Latest update adds a simple "hello world" test |
Formula/hashlink.rb
Outdated
| homepage "https://hashlink.haxe.org/" | ||
| version "1.11" | ||
| url "https://github.com/HaxeFoundation/hashlink.git", | ||
| :revision=>"cd269136628a1d445b2253ded9fd10ab9fe2f9be" # v 1.11 |
There was a problem hiding this comment.
It needs to have the tag and the revision.
|
You have style issues now. |
|
Updated once more. |
|
I'm seeing a CI/test failure: Does brew not recognize hdll files as libraries? |
|
I wouldn't think so. The macOS standard for those is dyld. |
|
That seems well intentioned, but overly restrictive, no? Homebrew/brew#2997 In any event, I'll see about trying to change it. |
|
After looking into the library extension issue, I'm seeing that the "hdll" is an extension pattern that Hashlink uses pretty extensively. Removing it/renaming it is non-trivial, in fact I think forcing the change is a deal breaker. Hashlink is a cross platform virtual machine, and is a successor to "neko" (that uses a similar pattern of library naming): https://github.com/Homebrew/homebrew-core/blob/2d2aa9cf0d0ba73314a2d04821542667e5712c84/Formula/neko.rb see neko .ndll library file usage here: https://github.com/HaxeFoundation/neko/blob/ff67a696a718cba0a6bc0ec583e355272b4a5923/CMakeLists.txt My point is, there's an awful lot of cross-platform best practice learnings incorporated in these projects and their library naming patterns. Is this something that homebrew can adapt to? Perhaps a whitelist of some sort? |
Formula/hashlink.rb
Outdated
| :revision => "cd269136628a1d445b2253ded9fd10ab9fe2f9be", | ||
| :tag => "1.11" | ||
|
|
||
| depends_on "cmake" |
There was a problem hiding this comment.
Is this a runtime dependency?
There was a problem hiding this comment.
The short answer is no. These are (mostly) all lazy loaded for the purposes of FFI.
|
@jdonaldson Here is revised formula without external testing dependencies: class Hashlink < Formula
desc "Virtual machine for Haxe"
homepage "https://hashlink.haxe.org/"
url "https://github.com/HaxeFoundation/hashlink/archive/1.11.tar.gz"
sha256 "b087ded7b93c7077f5b093b999f279a37aa1e31df829d882fa965389b5ad1aea"
depends_on "haxe" => :test
depends_on "jpeg-turbo"
depends_on "libogg"
depends_on "libpng"
depends_on "libuv"
depends_on "libvorbis"
depends_on "mbedtls"
depends_on "openal-soft"
depends_on "sdl2"
def install
system "make"
system "make", "install", "INSTALL_DIR=#{prefix}"
end
test do
haxebin = Formula["haxe"].bin
(testpath/"HelloWorld.hx").write <<~EOS
class HelloWorld {
static function main() trace("Hello world!");
}
EOS
system "#{haxebin}/haxe", "-hl", "out.hl", "-main", "HelloWorld"
assert_equal "HelloWorld.hx:2: Hello world!\n", shell_output("#{bin}/hl out.hl")
system "#{haxebin}/haxelib", "newrepo"
system "#{haxebin}/haxelib", "install", "hashlink"
system "#{haxebin}/haxe", "-hl", "src/main.c", "-main", "HelloWorld"
system "cc", "-O3", "-o", "hello-world",
"-std=c11", "-I", "src", "src/main.c",
"-L#{lib}", "-lhl"
assert_equal "HelloWorld.hx:2: Hello world!\n", `./hello-world`
end
endNote that you don't really need |
|
Great tip, I'll go with that. |
|
Thanks again @vladimyr, this is very elegant and I learned a couple of things. Let me know if there's something else I can do from my end. |
Then it is a double win 👍 and I'm glad I could help. I believe this is best you can get from this one but we'll see what @SMillerDev thinks about it 🤞 |
|
Thanks again for the changes, I've merged them in. Is there anything else that is required here? |
|
I think this is in a good shape and ready for merge 👍 |
|
Thanks, I merged the suggestion. |
|
Can you squash all your commits into one |
|
squashed/rebased on master |
|
amended the name |
Actually I was wrong. The real requirement here is that it is located inside OFC that means it will end up symlinked inside
Honestly, I still haven't seen any example of plain old Makefile having good
First of all, it didn't sound like that! Anyway, the former is independent of HB's install location so it shouldn't break on Linux and later - well it isn't cross-platform by definition 🙃 But I knew I could do better so I did... Before I proceed let me just thank both of you @MikeMcQuaid and @jdonaldson for your kind words! It means a lot to me! Ok, back to the track. Instead of tweaking rpath after compilation (which is platform-specific), I decided to use cross-platform/toolchain linker directive:
Here is how I addressed them:
That resolves the issue with libraries because now we:
There is also an issue with placing Finally, it results with following directory layout: Global lib and include folders aren't cluttered and the only thing that still gets symlinked at the root level is Full installation procedure looks like this: def install
inreplace "Makefile", /^LFLAGS\s*=\s*/, "LFLAGS = -Wl,-rpath,#{lib}/hashlink "
system "make"
bin.install "hl"
(lib/"hashlink").install Dir["*.dylib"], Dir["*.hdll"]
(include/"hashlink").install "src/hl.h", "src/hlc.h", "src/hlc_main.c"
lib.install_symlink Dir[lib/"hashlink/*.dylib"]
endYou may ask why I'm doing so many But then I noticed that def install
inreplace "Makefile", /^LFLAGS\s*=\s*/, "LFLAGS = -Wl,-rpath,#{lib}/hashlink "
system "make"
if build.head?
system "make", "install",
"INSTALL_DIR=#{prefix}",
"INSTALL_BIN_DIR=#{bin}",
"INSTALL_LIB_DIR=#{lib}/hashlink",
"INSTALL_INCLUDE_DIR=#{include}/hashlink"
else
bin.install "hl"
(lib/"hashlink").install Dir["*.dylib"], Dir["*.hdll"]
(include/"hashlink").install "src/hl.h", "src/hlc.h", "src/hlc_main.c"
end
lib.install_symlink Dir[lib/"hashlink/*.dylib"]
endPresumably, the next version will have those make vars and I've shown you global side of things here is how it looks from cellar perspective: Let's look inside Yup, no global paths, strictly cellar ones! And global libs are loaded from Let me now finish this journey by presenting you with the updated formula: https://gist.github.com/vladimyr/ba6f3f8396910a7399450f8f5fd1c099#file-hashlink-rb 🎉 PS I've deleted previous review suggestions because I no longer recommend libexec and binary relative rpath solution due to reasons mentioned above and all those test suite updates can be c/p from linked gist. PSS @jdonaldson
Both MachO and ELF binaries support (multiple) |
If you set the prefix to
Would you be able to submit the Great work @vladimyr 👏 |
You got me thinking there for a while but I finally figured out what is happening. First, I compiled That makes sense because Makefile says the following: where the dot is build directory and So where does It turns out I've reverted my def install
system "make"
system "make", "install", "INSTALL_DIR=#{libexec}"
bin.install_symlink libexec/"bin/hl"
endand re-ran installation. Here is a trimmed (debug) log output: I've changed it once again to use def install
system "make"
system "make", "install", "INSTALL_DIR=#{prefix}"
endand got this: So HB itself alters install names inside binaries (and libraries) thus inserting phantom def install
system "make"
system "make", "install", "INSTALL_DIR=#{libexec}"
lib.install_symlink libexec/"lib/libhl.dylib"
bin.install_symlink libexec/"bin/hl"
endThis produces the correct result: |
Again: good catch. I've opened Homebrew/brew#7679 which should mean that this is unnecessary. |
What do you mean by this? HB's magic works only if dynamic libraries are known upfront i.e. if their load paths are baked inside binary. In hashlink's case, only |
It should mean the |
Yup, thanks for the clarification! 👍 |
|
As per your other point:
I've slightly changed it to mimic existence of additional def install
inreplace "Makefile", /\$\{LFLAGS\}/, "${LFLAGS} ${EXTRA_LFLAGS}"
system "make", "EXTRA_LFLAGS=-Wl,-rpath,#{lib}/hashlink"
# ...
endThe next thing I'm gonna do is to create upstream PR that adds this new variable. If it gets accepted |
Sounds great, thanks! |
|
HaxeFoundation/hashlink#387 got merged and @MikeMcQuaid asked to change lib location from
|
|
@vladimyr Thank you for sticking with this PR and troubleshooting this issue, Dario! 🏆 Note that if you're relying on a new feature of Homebrew/brew that is required to install the bottle/formula, this PR must wait until a new release of Homebrew/brew is tagged and released. |
alebcay
added
do not merge
and removed
upstream issue
|
I updated the formula again with @vladimyr's suggestions. I feel like he deserves all the credit at this point, but I'd really like to hang on to this thread since I'm getting such a positive vibe off of it (And that's in short supply these days!) |
|
Thank you for your kind words and at risk of sounding cheap, this really was and still is teamwork. Everyone did their part, from the one who created PR in the first place, folks asking questions and providing constructive criticism, the one who altered Github labels, the one who did research, the one who altered HB itself to support it, Hashlink author theyself for adjusting Makefile to our needs... Many roles listed, yet without any of those, we wouldn't be anywhere near the end of this journey. Today it was me, tomorrow will be someone else... |
|
Lovely words @vladimyr. I've had a shitty 24 hours and this has made it much better ❤️ |
hashlink : use :revision instead of :tag hashlink: add hello world test using resource address style issues, add tag back hashlink : use the testing configuration from vladimyr Update Formula/hashlink.rb Co-authored-by: Dario Vladović <[email protected]> Update Formula/hashlink.rb Co-authored-by: Dario Vladović <[email protected]> Update Formula/hashlink.rb Co-authored-by: Dario Vladović <[email protected]> Update Formula/hashlink.rb Co-authored-by: Sean Molenaar <[email protected]>
|
|
|
Thanks so much for your contribution and patience! Without people like you submitting PRs we couldn't run this project. You rock, @jdonaldson! Also thanks to @vladimyr, @SMillerDev and others for all the help here ❤️ |
brew install --build-from-source <formula>, where<formula>is the name of the formula you're submitting?brew test <formula>, where<formula>is the name of the formula you're submitting?brew audit --strict <formula>(after doingbrew install <formula>)?