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

zig 0.1.1 (new formula) #23189

Closed
wants to merge 1 commit into from
Closed

zig 0.1.1 (new formula) #23189

wants to merge 1 commit into from

Conversation

@jfo
Copy link
Contributor

@jfo jfo commented Jan 23, 2018

HI! Thanks for brew.

I'd like to add zig's 0.1.1 release to brew. I do need feedback on a dependency issue, though.

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

~ $ brew audit --strict zig
zig:
  * Dependency 'llvm@5' is an alias; use the canonical name 'llvm'.
Error: 1 problem in 1 formula

See: ziglang/zig#714 (comment)

Please lmkwyt! Thank you.

This PR resolves ziglang/zig#714

@jfo jfo mentioned this pull request Jan 23, 2018
Formula/zig.rb Outdated

def install
Dir.mkdir "build"
Dir.chdir "build" do

This comment has been minimized.

@fxcoudert

fxcoudert Jan 23, 2018
Member

On a single line: mkdir "build" do

Formula/zig.rb Outdated
def install
Dir.mkdir "build"
Dir.chdir "build" do
system "cmake", "..",

This comment has been minimized.

@fxcoudert

fxcoudert Jan 23, 2018
Member

Probably you can simply say: system "cmake", "..", *std_cmake_args which should take care of the paths for you

This comment has been minimized.

@jfo

jfo Jan 23, 2018
Author Contributor

I get Error: Empty installation when I use that.

This comment has been minimized.

@fxcoudert

fxcoudert Jan 23, 2018
Member

std_cmake_args contains:

      -DCMAKE_C_FLAGS_RELEASE=-DNDEBUG
      -DCMAKE_CXX_FLAGS_RELEASE=-DNDEBUG
      -DCMAKE_INSTALL_PREFIX=#{prefix}
      -DCMAKE_BUILD_TYPE=Release
      -DCMAKE_FIND_FRAMEWORK=LAST
      -DCMAKE_VERBOSE_MAKEFILE=ON
      -Wno-dev

But I do not understand what your CMAKE_PREFIX_PATH is pointing to. #{prefix} will expand to zig's own prefix (e.g. /usr/local/Cellar/zig/0.1.1), so why are you appending llvm@5 to that?

This comment has been minimized.

@jfo

jfo Jan 23, 2018
Author Contributor

Actually maybe I'm calling mkdir strangely, let me test a bit.

edit: I was. Was trying to call Dir.mkdir instead of mkdir.

This comment has been minimized.

@jfo

jfo Jan 23, 2018
Author Contributor

I was naively following zig's build instructions to see how everything worked. std_cmake_args works great though!

Formula/zig.rb Outdated
end

test do
src = <<-HEREDOC

This comment has been minimized.

@fxcoudert

fxcoudert Jan 23, 2018
Member

We prefer the following syntax: (testpath/"hello.zip").write <<~EOS

Formula/zig.rb Outdated
HEREDOC
File.write("hello.zig", src)
system "#{bin}/zig", "build-exe", "hello.zig"
system "./hello"

This comment has been minimized.

@fxcoudert

fxcoudert Jan 23, 2018
Member

Can you check that the output from hello is as expected?
assert_equal "Hello, world!\n", shell_output("./hello")

This comment has been minimized.

@jfo

jfo Jan 23, 2018
Author Contributor

Yes, that's much better thanks.

@jfo jfo force-pushed the jfo:master branch Jan 23, 2018
Formula/zig.rb Outdated
end

test do
(testpath/"hello.zig").write <<~HEREDOC

This comment has been minimized.

@fxcoudert

fxcoudert Jan 23, 2018
Member

We'll prefer EOS to HEREDOC, for consistency with other formulas.
And can the empty lines in hello.zig be removed?

This comment has been minimized.

@jfo

jfo Jan 23, 2018
Author Contributor

Sure

@fxcoudert
Copy link
Member

@fxcoudert fxcoudert commented Jan 23, 2018

Regarding llvm@5, our policy is to depend on llvm: when version 6 is published, hopefully there will be a new zig version to match it. Otherwise, if there is incompatibility, we will then modify the formula to depend on llvm@5.

@jfo jfo force-pushed the jfo:master branch Jan 23, 2018
@jfo
Copy link
Contributor Author

@jfo jfo commented Jan 23, 2018

Thanks for the reviews.

I am curious as to whether llvm is a build time dependency only. I'm waiting on an answer to that just so I'm sure. If it's necessary to have it during runtime, is it as simple as removing the :build symbol?

@jfo
Copy link
Contributor Author

@jfo jfo commented Jan 23, 2018

also the test is still failing with just llvm, despite passing brew audit --strict zig

Failing with same error as brew audit --new-formula zig does locally:

zig:
  * Dependency 'llvm' may be unnecessary as it is provided by macOS; try to build this formula without it.
Error: 1 problem in 1 formula
@fxcoudert
Copy link
Member

@fxcoudert fxcoudert commented Jan 23, 2018

That remaining audit failure we'll ignore, as it has a valid reason to use Homebrew LLVM.

Formula/zig.rb Outdated
sha256 "a160d14aecebead2f8c574b0dd687cdfecc53d149d521a17b89b629d8b558c94"

depends_on "cmake" => :build
depends_on "llvm" => :build

This comment has been minimized.

@fxcoudert

fxcoudert Jan 23, 2018
Member

zig links to LLVM's libc++ (/usr/local/opt/llvm/lib/libc++.1.dylib). So this dependency is necessary at runtime too. Remove the => :build.

This comment has been minimized.

@jfo

jfo Jan 23, 2018
Author Contributor

Done, thanks for sussing that out!

@jfo jfo force-pushed the jfo:master branch Jan 23, 2018
@jfo
Copy link
Contributor Author

@jfo jfo commented Jan 23, 2018

@fxcoudert thanks a lot for a painless contributor experience and for the prompt reviews! I'll look forward to seeing this land.

I'll do some research around what fleshing out this formula might look like (see associated man issue) and whether or not that is necessary at all. At the very least we'll keep it bumped on new releases.

@jfo jfo closed this Jan 23, 2018
@jfo jfo force-pushed the jfo:master branch to c4c3fde Jan 23, 2018
@fxcoudert
Copy link
Member

@fxcoudert fxcoudert commented Jan 23, 2018

hi @jfo why did you close this?

@jfo
Copy link
Contributor Author

@jfo jfo commented Jan 23, 2018

@fxcoudert sorry, I was pushing jfo@35218e9 from a local branch and typed git push myremote master instead of git push myremote mybranch:master

@jfo
Copy link
Contributor Author

@jfo jfo commented Jan 23, 2018

I pushed my changes back to my fork's master but github is confused now.

edit: looks like reopening it triggered a recheck, sorry for the noise!! :)

@jfo jfo reopened this Jan 23, 2018

def install
mkdir "build" do
system "cmake", "..", *std_cmake_args

This comment has been minimized.

@ilovezfs

ilovezfs Jan 23, 2018
Contributor

let's make this an in-tree build

This comment has been minimized.

@jfo

jfo Jan 23, 2018
Author Contributor

do you mean

diff --git a/Formula/zig.rb b/Formula/zig.rb
index 42ce4f54..758d9617 100644
--- a/Formula/zig.rb
+++ b/Formula/zig.rb
@@ -8,10 +8,8 @@ class Zig < Formula
   depends_on "llvm"
 
   def install
-    mkdir "build" do
-      system "cmake", "..", *std_cmake_args
-      system "make", "install"
-    end
+    system "cmake", ".", *std_cmake_args
+    system "make", "install"
   end
 
   test do

This comment has been minimized.

@ilovezfs

ilovezfs Jan 23, 2018
Contributor

ya

@fxcoudert
Copy link
Member

@fxcoudert fxcoudert commented Jan 23, 2018

@jfo thanks for your contribution to Homebrew!

@fxcoudert
Copy link
Member

@fxcoudert fxcoudert commented Jan 23, 2018

@BrewTestBot test this please

@fxcoudert
Copy link
Member

@fxcoudert fxcoudert commented Jan 23, 2018

@jfo sorry, somehow the <<-EOS.undent in one of the commits is making our build bot fail. Could you squash the 3 commits together into a single commit please? Then I can merge this.

@jfo jfo force-pushed the jfo:master branch to 62d62c0 Jan 23, 2018
@fxcoudert
Copy link
Member

@fxcoudert fxcoudert commented Jan 23, 2018

And… finally, it's in! Thanks again @jfo

@fxcoudert fxcoudert closed this in c33c622 Jan 23, 2018
@jfo
Copy link
Contributor Author

@jfo jfo commented Jan 23, 2018

🙌 thank you!

Super great and much appreciated.

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants