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

nginx: have head depends on openssl@1.1 #4519

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@JLHwung
Copy link
Contributor

JLHwung commented Sep 4, 2016

Add build with openssl@1.1 support, there will be an option parsed as --with-openssl@1.1 on brew info nginx. It looks a little bit creepy but I don't know how to customize the parsed option.

Now it can only be built on --HEAD, so one can looking forward to build on 1.11.4.

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same formula update/change?
  • Have you built your formula locally prior to submission with brew install <formula> (where <formula> is the name of the formula you're submitting)?
  • Does your submission pass brew audit --new-formula <formula> (after doing brew install <formula>)?

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Sep 4, 2016

If I had my druthers, we would:

  • remove the option to build with libressl
  • have devel and stable depend on openssl 1.0
  • have head depend on openssl 1.1

and get rid of all of the options entirely.

@DomT4 probably has opinions about that. :)

@JLHwung

This comment has been minimized.

Copy link
Contributor

JLHwung commented Sep 5, 2016

I totally agree with @tdsmith. I will work on it if @DomT4 also votes up.

@tdsmith Would devel depend on openssl 1.1 once a new devel cut from head?

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Sep 5, 2016

@tdsmith Would devel depend on openssl 1.1 once a new devel cut from head?

Yes, as soon as it's supported.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Sep 5, 2016

Since nginx isn't used by anything else, seems fine to start migrating it across. I don't want to make a complete habit of this, but I'm fine with dropping libressl here because it seems the majority who care about LibreSSL with nginx are using the nginx-full formula in the nginx tap.

have devel and stable depend on openssl 1.0

This remains just depends_on "openssl" for the benefit of OP. I considered renaming it to openssl-lts but I thought that'd probably annoy the heck out of people.

have head depend on openssl 1.1

Seems reasonable, yeah.

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Sep 5, 2016

Cool! Thanks Dom :)

JLHwung added some commits Sep 4, 2016

@JLHwung JLHwung force-pushed the JLHwung:patch-2 branch from bcaa3f7 to 85feaad Sep 5, 2016

@JLHwung

This comment has been minimized.

Copy link
Contributor

JLHwung commented Sep 5, 2016

I have reworked on the branch, marking openssl as required because I haven't found any option like --without-openssl. I also removes building with libressl.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Sep 5, 2016

That isn't quite the way to go about the change. You'll want to do something like:

diff --git a/Formula/nginx.rb b/Formula/nginx.rb
index 93fc50c..298e204 100644
--- a/Formula/nginx.rb
+++ b/Formula/nginx.rb
@@ -1,9 +1,13 @@
 class Nginx < Formula
   desc "HTTP(S) server and reverse proxy, and IMAP/POP3 proxy server"
   homepage "https://nginx.org/"
-  url "https://nginx.org/download/nginx-1.10.1.tar.gz"
-  sha256 "1fd35846566485e03c0e318989561c135c598323ff349c503a6c14826487a801"
-  head "http://hg.nginx.org/nginx/", :using => :hg
+
+  stable do
+    url "https://nginx.org/download/nginx-1.10.1.tar.gz"
+    sha256 "1fd35846566485e03c0e318989561c135c598323ff349c503a6c14826487a801"
+
+    depends_on "openssl"
+  end

   bottle do
     sha256 "2b67c86454cc67bdeb2a637d9872ae471a810d8a2dae40b6a0c1dad7b253d30d" => :el_capitan
@@ -14,6 +18,14 @@ class Nginx < Formula
   devel do
     url "https://nginx.org/download/nginx-1.11.3.tar.gz"
     sha256 "4a667f40f9f3917069db1dea1f2d5baa612f1fa19378aadf71502e846a424610"
+
+    depends_on "openssl"
+  end
+
+  head do
+    url "http://hg.nginx.org/nginx/", :using => :hg
+
+    depends_on "openssl@1.1"
   end

   # Before submitting more options to this formula please check they aren't
@@ -29,8 +41,6 @@ class Nginx < Formula

   depends_on "pcre"
   depends_on "passenger" => :optional
-  depends_on "openssl" => :recommended
-  depends_on "libressl" => :optional

   def install
     # Changes default port to 8080
@@ -40,16 +50,14 @@ class Nginx < Formula
     end

     pcre = Formula["pcre"]
-    openssl = Formula["openssl"]
-    libressl = Formula["libressl"]

-    if build.with? "libressl"
-      cc_opt = "-I#{pcre.include} -I#{libressl.include}"
-      ld_opt = "-L#{pcre.lib} -L#{libressl.lib}"
+    if build.head?
+      openssl = Formula["openssl@1.1"]
     else
-      cc_opt = "-I#{pcre.include} -I#{openssl.include}"
-      ld_opt = "-L#{pcre.lib} -L#{openssl.lib}"
+      openssl = Formula["openssl"]
     end
+    cc_opt = "-I#{pcre.include} -I#{openssl.include}"
+    ld_opt = "-L#{pcre.lib} -L#{openssl.lib}"

     args = %W[
       --prefix=#{prefix}

@JLHwung JLHwung force-pushed the JLHwung:patch-2 branch from 25e2e4b to 1cf01ef Sep 5, 2016

@JLHwung

This comment has been minimized.

Copy link
Contributor

JLHwung commented Sep 5, 2016

Thanks @DomT4. Reworked according to the diff.

cc_opt = "-I#{pcre.include} -I#{libressl.include}"
ld_opt = "-L#{pcre.lib} -L#{libressl.lib}"
if build.head?
openssl = Formula["openssl@1.1"]

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 5, 2016

Member

Could simplify all this to

openssl = build.head? ? Formula["openssl@1.1"] : Formula["openssl"]

This comment has been minimized.

@JLHwung

JLHwung Sep 5, 2016

Contributor

I am good with if-else, as there is no other ternary operators in this file.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 5, 2016

Member

I prefer the ternary but have no strong preference. If you prefer if/else then:

openssl = if build.head?
  Formula["openssl@1.1"]
else
  Formula["openssl"]
end

This comment has been minimized.

@DomT4

DomT4 Sep 5, 2016

Contributor

@MikeMcQuaid I thought you hated ternary operators so I didn't suggest it 😄, but if you're happy I'm happy for OP to use that.

This comment has been minimized.

@JLHwung

JLHwung Sep 5, 2016

Contributor

That even looks worse than ternary operator, I will go with your previous suggestion.

@JLHwung JLHwung changed the title nginx: add build with openssl@1.1 support nginx: have head depends on openssl@1.1 Sep 5, 2016

@JLHwung JLHwung force-pushed the JLHwung:patch-2 branch from 1cf01ef to 88ac279 Sep 5, 2016

@JLHwung JLHwung force-pushed the JLHwung:patch-2 branch from 88ac279 to d00c7cc Sep 5, 2016

@DomT4 DomT4 closed this in 3b58fe0 Sep 7, 2016

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Sep 7, 2016

Merged in 3b58fe0. Thank you for another contribution to Homebrew @JLHwung; we appreciate it! 😺

@JLHwung

This comment has been minimized.

Copy link
Contributor

JLHwung commented Sep 7, 2016

@DomT4 Thank you very much for your example. I should have read more about the cookbook. And thank @MikeMcQuaid for code style suggestion.

@JLHwung JLHwung deleted the JLHwung:patch-2 branch Sep 7, 2016

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Sep 7, 2016

More than happy to help. It's a big part of what we're here for 😃.

@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.