[PATCH 0/3] configure: respect --without-<package> flags

[PATCH 0/3] configure: respect --without-<package> flags

[Đoàn Trần Công Danh]

Current git's configure script allow --without-<package>,
it rightfully set NO_<PACKAGE> in the beginning of the configure script,
but overwrite them with autodetect later.

Respect them now.

P/S: I'm not sure about the change to NO_OPENSSL

Đoàn Trần Công Danh (3):
  configure: respect --without-curl flags
  configure: respect --without-expat
  configure: respect --without-openssl

 configure.ac | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

-- 
2.26.0.rc2.234.g969ad452ca

[PATCH 1/3] configure: respect --without-curl flags

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 configure.ac | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 66aedb9288..f4742878c0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -592,6 +592,9 @@ fi
 # git-http-push are not built, and you cannot use http:// and https://
 # transports.
 
+# Respect --without-curl
+if test "x$NO_CURL" != "xYesPlease"; then
+
 GIT_STASH_FLAGS($CURLDIR)
 
 AC_CHECK_LIB([curl], [curl_global_init],
@@ -600,8 +603,6 @@ AC_CHECK_LIB([curl], [curl_global_init],
 
 GIT_UNSTASH_FLAGS($CURLDIR)
 
-GIT_CONF_SUBST([NO_CURL])
-
 if test -z "$NO_CURL"; then
 
 AC_CHECK_PROG([CURL_CONFIG], [curl-config],
@@ -622,6 +623,9 @@ fi
 
 fi
 
+fi
+
+GIT_CONF_SUBST([NO_CURL])
 
 #
 # Define NO_EXPAT if you do not have expat installed.  git-http-push is
-- 
2.26.0.rc2.234.g969ad452ca

Re: [PATCH 1/3] configure: respect --without-curl flags

[Eric Sunshine]

On Wed, Mar 18, 2020 at 3:38 AM Đoàn Trần Công Danh
<congdanhqx@gmail.com> wrote:
> diff --git a/configure.ac b/configure.ac
> @@ -592,6 +592,9 @@ fi
>  # Define NO_CURL if you do not have libcurl installed.  git-http-pull and
>  # git-http-push are not built, and you cannot use http:// and https://
>  # transports.
>
> +# Respect --without-curl
> +if test "x$NO_CURL" != "xYesPlease"; then
> ...
>  if test -z "$NO_CURL"; then

I realize that GIT_PARSE_WITH() will either clear NO_CURL or set it to
literal "YesPlease", but the comment(s) in this file describing
NO_CURL say only to _define_ it to build without curl support. So, I'm
wondering if it would make more sense to take a looser view about the
value of NO_CURL. The existing check of NO_CURL doesn't bother looking
for a specific value, but instead just tests whether it has a value or
not. Perhaps the new check can be modeled after that one.

Also, I think you can reduce the scope of this change quite a bit by
merely wrapping the AC_CHECK_LIB() invocation. So, either:

    ...
    if test -z "$NO_CURL"; then
    GIT_STASH_FLAGS($CURLDIR)

    AC_CHECK_LIB([curl], [curl_global_init],
    [NO_CURL=],
    [NO_CURL=YesPlease])

    GIT_UNSTASH_FLAGS($CURLDIR)
    fi

    GIT_CONF_SUBST([NO_CURL])
    ...

or even:

    ...
    if test -z "$NO_CURL"; then
    AC_CHECK_LIB([curl], [curl_global_init],
    [NO_CURL=],
    [NO_CURL=YesPlease])
    fi
    ...

Same comments applies to the other patches, as well.

Re: [PATCH 1/3] configure: respect --without-curl flags

[Danh Doan]

On 2020-03-18 11:51:34-0400, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Mar 18, 2020 at 3:38 AM Đoàn Trần Công Danh
> <congdanhqx@gmail.com> wrote:
> > diff --git a/configure.ac b/configure.ac
> > @@ -592,6 +592,9 @@ fi
> >  # Define NO_CURL if you do not have libcurl installed.  git-http-pull and
> >  # git-http-push are not built, and you cannot use http:// and https://
> >  # transports.
> >
> > +# Respect --without-curl
> > +if test "x$NO_CURL" != "xYesPlease"; then
> > ...
> >  if test -z "$NO_CURL"; then
> 
> I realize that GIT_PARSE_WITH() will either clear NO_CURL or set it to
> literal "YesPlease", but the comment(s) in this file describing
> NO_CURL say only to _define_ it to build without curl support. So, I'm
> wondering if it would make more sense to take a looser view about the
> value of NO_CURL. The existing check of NO_CURL doesn't bother looking
> for a specific value, but instead just tests whether it has a value or
> not. Perhaps the new check can be modeled after that one.
> 
> Also, I think you can reduce the scope of this change quite a bit by
> merely wrapping the AC_CHECK_LIB() invocation. So, either:
> 
>     ...
>     if test -z "$NO_CURL"; then
>     GIT_STASH_FLAGS($CURLDIR)
> 
>     AC_CHECK_LIB([curl], [curl_global_init],
>     [NO_CURL=],
>     [NO_CURL=YesPlease])
> 
>     GIT_UNSTASH_FLAGS($CURLDIR)
>     fi
> 
>     GIT_CONF_SUBST([NO_CURL])
>     ...
> 
> or even:
> 
>     ...
>     if test -z "$NO_CURL"; then
>     AC_CHECK_LIB([curl], [curl_global_init],
>     [NO_CURL=],
>     [NO_CURL=YesPlease])
>     fi
>     ...
> 
> Same comments applies to the other patches, as well.

I've re-checked the configure.ac code.

We've already use:

	test -z "$NO_ICONV"

I think rewrite like your suggestion would be better choice, and
it'll be consistence with the current code.

I'll send a reroll after checking what should be done with NO_OPENSSL.

-- 
Danh

[PATCH 2/3] configure: respect --without-expat

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 configure.ac | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/configure.ac b/configure.ac
index f4742878c0..7468eb6bc8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -631,6 +631,9 @@ GIT_CONF_SUBST([NO_CURL])
 # Define NO_EXPAT if you do not have expat installed.  git-http-push is
 # not built, and you cannot push using http:// and https:// transports.
 
+# Respect --without-expat
+if test "x$NO_EXPAT" != xYesPlease; then
+
 GIT_STASH_FLAGS($EXPATDIR)
 
 AC_CHECK_LIB([expat], [XML_ParserCreate],
@@ -639,6 +642,8 @@ AC_CHECK_LIB([expat], [XML_ParserCreate],
 
 GIT_UNSTASH_FLAGS($EXPATDIR)
 
+fi
+
 GIT_CONF_SUBST([NO_EXPAT])
 
 #
-- 
2.26.0.rc2.234.g969ad452ca

[PATCH 3/3] configure: respect --without-openssl

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 configure.ac | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index 7468eb6bc8..20a11e1f32 100644
--- a/configure.ac
+++ b/configure.ac
@@ -544,9 +544,15 @@ GIT_STASH_FLAGS($OPENSSLDIR)
 
 AC_CHECK_LIB([crypto], [SHA1_Init],
 [NEEDS_SSL_WITH_CRYPTO=],
-[AC_CHECK_LIB([ssl], [SHA1_Init],
- [NEEDS_SSL_WITH_CRYPTO=YesPlease NO_OPENSSL=],
- [NEEDS_SSL_WITH_CRYPTO=          NO_OPENSSL=YesPlease])])
+[
+if "x$NO_OPENSSL" = xYesPlease; then
+	AC_MSG_ERROR([Disabled OpenSSL is required for SHA1])
+else
+	AC_CHECK_LIB([ssl], [SHA1_Init],
+	[NEEDS_SSL_WITH_CRYPTO=YesPlease NO_OPENSSL=],
+	[NEEDS_SSL_WITH_CRYPTO=          NO_OPENSSL=YesPlease])
+fi
+])
 
 GIT_UNSTASH_FLAGS($OPENSSLDIR)
 
-- 
2.26.0.rc2.234.g969ad452ca

Re: [PATCH 3/3] configure: respect --without-openssl

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 938 bytes --]

On 2020-03-18 at 07:38:02, Đoàn Trần Công Danh wrote:
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  configure.ac | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 7468eb6bc8..20a11e1f32 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -544,9 +544,15 @@ GIT_STASH_FLAGS($OPENSSLDIR)
>  
>  AC_CHECK_LIB([crypto], [SHA1_Init],
>  [NEEDS_SSL_WITH_CRYPTO=],
> -[AC_CHECK_LIB([ssl], [SHA1_Init],
> - [NEEDS_SSL_WITH_CRYPTO=YesPlease NO_OPENSSL=],
> - [NEEDS_SSL_WITH_CRYPTO=          NO_OPENSSL=YesPlease])])
> +[
> +if "x$NO_OPENSSL" = xYesPlease; then
> +	AC_MSG_ERROR([Disabled OpenSSL is required for SHA1])

I don't see this in the original.  Wouldn't we want to fall back to the
default in this case, which would be SHA1DC?
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 3/3] configure: respect --without-openssl

[Danh Doan]

On 2020-03-18 11:22:13+0000, "brian m. carlson" <sandals@crustytoothpaste.net> wrote:
> On 2020-03-18 at 07:38:02, Đoàn Trần Công Danh wrote:
> > Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> > ---
> >  configure.ac | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 7468eb6bc8..20a11e1f32 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -544,9 +544,15 @@ GIT_STASH_FLAGS($OPENSSLDIR)
> >  
> >  AC_CHECK_LIB([crypto], [SHA1_Init],
> >  [NEEDS_SSL_WITH_CRYPTO=],
> > -[AC_CHECK_LIB([ssl], [SHA1_Init],
> > - [NEEDS_SSL_WITH_CRYPTO=YesPlease NO_OPENSSL=],
> > - [NEEDS_SSL_WITH_CRYPTO=          NO_OPENSSL=YesPlease])])
> > +[
> > +if "x$NO_OPENSSL" = xYesPlease; then
> > +	AC_MSG_ERROR([Disabled OpenSSL is required for SHA1])
> 
> I don't see this in the original.  Wouldn't we want to fall back to the
> default in this case, which would be SHA1DC?

This block of code will be run into in macOS only.

Really, I wasn't sure what should be done in this case (I mentioned in
the cover letter).

Should we set:

	NEEDS_SSL_WITH_CRYPTO=
	NO_OPENSSL=YesPlease

in this case instead?

-- 
Danh

[PATCH v2 0/3] configure: respect --without-<package> flags

[Đoàn Trần Công Danh]

Current git's configure script allow --without-<package>,
it rightfully set NO_<PACKAGE> in the beginning of the configure script,
but overwrite them with autodetect later.

Respect them now.

Change from v1:
- Simply check if NO_<PACKAGE> is empty or not instead of explicitly
  check for YesPlease
- --without-openssl will ignore libcrypto all-together and fallback
  into DC_SHA1

Đoàn Trần Công Danh (3):
  configure: respect --without-curl flag
  configure: respect --without-expat flag
  configure: respect --without-openssl flag

 configure.ac | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Interdiff against v1:
diff --git a/configure.ac b/configure.ac
index 20a11e1f32..807bb8bc5a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -542,17 +542,18 @@ AC_MSG_NOTICE([CHECKS for libraries])
 
 GIT_STASH_FLAGS($OPENSSLDIR)
 
+if test -z "$NO_OPENSSL"; then
 AC_CHECK_LIB([crypto], [SHA1_Init],
 [NEEDS_SSL_WITH_CRYPTO=],
-[
-if "x$NO_OPENSSL" = xYesPlease; then
-	AC_MSG_ERROR([Disabled OpenSSL is required for SHA1])
+[AC_CHECK_LIB([ssl], [SHA1_Init],
+ [NEEDS_SSL_WITH_CRYPTO=YesPlease NO_OPENSSL=],
+ [NEEDS_SSL_WITH_CRYPTO=          NO_OPENSSL=YesPlease])])
 else
-	AC_CHECK_LIB([ssl], [SHA1_Init],
-	[NEEDS_SSL_WITH_CRYPTO=YesPlease NO_OPENSSL=],
-	[NEEDS_SSL_WITH_CRYPTO=          NO_OPENSSL=YesPlease])
+	# Fallback to DC SHA1
+	NEEDS_SSL_WITH_CRYPTO=
+	DC_SHA1=YesPlease
+	GIT_CONF_SUBST([DC_SHA1])
 fi
-])
 
 GIT_UNSTASH_FLAGS($OPENSSLDIR)
 
@@ -598,17 +599,19 @@ fi
 # git-http-push are not built, and you cannot use http:// and https://
 # transports.
 
-# Respect --without-curl
-if test "x$NO_CURL" != "xYesPlease"; then
-
 GIT_STASH_FLAGS($CURLDIR)
 
+# Respect --without-curl
+if test -z "$NO_CURL"; then
 AC_CHECK_LIB([curl], [curl_global_init],
 [NO_CURL=],
 [NO_CURL=YesPlease])
+fi
 
 GIT_UNSTASH_FLAGS($CURLDIR)
 
+GIT_CONF_SUBST([NO_CURL])
+
 if test -z "$NO_CURL"; then
 
 AC_CHECK_PROG([CURL_CONFIG], [curl-config],
@@ -629,27 +632,22 @@ fi
 
 fi
 
-fi
-
-GIT_CONF_SUBST([NO_CURL])
 
 #
 # Define NO_EXPAT if you do not have expat installed.  git-http-push is
 # not built, and you cannot push using http:// and https:// transports.
 
-# Respect --without-expat
-if test "x$NO_EXPAT" != xYesPlease; then
-
 GIT_STASH_FLAGS($EXPATDIR)
 
+# Respect --without-expat
+if test -z "$NO_EXPAT"; then
 AC_CHECK_LIB([expat], [XML_ParserCreate],
 [NO_EXPAT=],
 [NO_EXPAT=YesPlease])
+fi
 
 GIT_UNSTASH_FLAGS($EXPATDIR)
 
-fi
-
 GIT_CONF_SUBST([NO_EXPAT])
 
 #
-- 
2.26.0.rc2.234.g969ad452ca

[PATCH v2 1/3] configure: respect --without-curl flag

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 configure.ac | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/configure.ac b/configure.ac
index 66aedb9288..5a73a592e1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -594,9 +594,12 @@ fi
 
 GIT_STASH_FLAGS($CURLDIR)
 
+# Respect --without-curl
+if test -z "$NO_CURL"; then
 AC_CHECK_LIB([curl], [curl_global_init],
 [NO_CURL=],
 [NO_CURL=YesPlease])
+fi
 
 GIT_UNSTASH_FLAGS($CURLDIR)
 
-- 
2.26.0.rc2.234.g969ad452ca

[PATCH v2 2/3] configure: respect --without-expat flag

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 configure.ac | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/configure.ac b/configure.ac
index 5a73a592e1..896996e177 100644
--- a/configure.ac
+++ b/configure.ac
@@ -632,9 +632,12 @@ fi
 
 GIT_STASH_FLAGS($EXPATDIR)
 
+# Respect --without-expat
+if test -z "$NO_EXPAT"; then
 AC_CHECK_LIB([expat], [XML_ParserCreate],
 [NO_EXPAT=],
 [NO_EXPAT=YesPlease])
+fi
 
 GIT_UNSTASH_FLAGS($EXPATDIR)
 
-- 
2.26.0.rc2.234.g969ad452ca

[PATCH v2 3/3] configure: respect --without-openssl flag

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 configure.ac | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/configure.ac b/configure.ac
index 896996e177..807bb8bc5a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -542,11 +542,18 @@ AC_MSG_NOTICE([CHECKS for libraries])
 
 GIT_STASH_FLAGS($OPENSSLDIR)
 
+if test -z "$NO_OPENSSL"; then
 AC_CHECK_LIB([crypto], [SHA1_Init],
 [NEEDS_SSL_WITH_CRYPTO=],
 [AC_CHECK_LIB([ssl], [SHA1_Init],
  [NEEDS_SSL_WITH_CRYPTO=YesPlease NO_OPENSSL=],
  [NEEDS_SSL_WITH_CRYPTO=          NO_OPENSSL=YesPlease])])
+else
+	# Fallback to DC SHA1
+	NEEDS_SSL_WITH_CRYPTO=
+	DC_SHA1=YesPlease
+	GIT_CONF_SUBST([DC_SHA1])
+fi
 
 GIT_UNSTASH_FLAGS($OPENSSLDIR)
 
-- 
2.26.0.rc2.234.g969ad452ca

Re: [PATCH v2 0/3] configure: respect --without-<package> flags

[Danh Doan]

On 2020-03-19 10:58:54+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> Current git's configure script allow --without-<package>,
> it rightfully set NO_<PACKAGE> in the beginning of the configure script,
> but overwrite them with autodetect later.
> 
> Respect them now.

Hi Junio,

Can I get your opinion on this serie?

I didn't see it in cooking list.
I guess it was lost because of 2.26.

> 
> Change from v1:
> - Simply check if NO_<PACKAGE> is empty or not instead of explicitly
>   check for YesPlease
> - --without-openssl will ignore libcrypto all-together and fallback
>   into DC_SHA1
> 
> Đoàn Trần Công Danh (3):
>   configure: respect --without-curl flag
>   configure: respect --without-expat flag
>   configure: respect --without-openssl flag
> 
>  configure.ac | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> Interdiff against v1:
> diff --git a/configure.ac b/configure.ac
> index 20a11e1f32..807bb8bc5a 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -542,17 +542,18 @@ AC_MSG_NOTICE([CHECKS for libraries])
>  
>  GIT_STASH_FLAGS($OPENSSLDIR)
>  
> +if test -z "$NO_OPENSSL"; then
>  AC_CHECK_LIB([crypto], [SHA1_Init],
>  [NEEDS_SSL_WITH_CRYPTO=],
> -[
> -if "x$NO_OPENSSL" = xYesPlease; then
> -	AC_MSG_ERROR([Disabled OpenSSL is required for SHA1])
> +[AC_CHECK_LIB([ssl], [SHA1_Init],
> + [NEEDS_SSL_WITH_CRYPTO=YesPlease NO_OPENSSL=],
> + [NEEDS_SSL_WITH_CRYPTO=          NO_OPENSSL=YesPlease])])
>  else
> -	AC_CHECK_LIB([ssl], [SHA1_Init],
> -	[NEEDS_SSL_WITH_CRYPTO=YesPlease NO_OPENSSL=],
> -	[NEEDS_SSL_WITH_CRYPTO=          NO_OPENSSL=YesPlease])
> +	# Fallback to DC SHA1
> +	NEEDS_SSL_WITH_CRYPTO=
> +	DC_SHA1=YesPlease
> +	GIT_CONF_SUBST([DC_SHA1])
>  fi
> -])
>  
>  GIT_UNSTASH_FLAGS($OPENSSLDIR)
>  
> @@ -598,17 +599,19 @@ fi
>  # git-http-push are not built, and you cannot use http:// and https://
>  # transports.
>  
> -# Respect --without-curl
> -if test "x$NO_CURL" != "xYesPlease"; then
> -
>  GIT_STASH_FLAGS($CURLDIR)
>  
> +# Respect --without-curl
> +if test -z "$NO_CURL"; then
>  AC_CHECK_LIB([curl], [curl_global_init],
>  [NO_CURL=],
>  [NO_CURL=YesPlease])
> +fi
>  
>  GIT_UNSTASH_FLAGS($CURLDIR)
>  
> +GIT_CONF_SUBST([NO_CURL])
> +
>  if test -z "$NO_CURL"; then
>  
>  AC_CHECK_PROG([CURL_CONFIG], [curl-config],
> @@ -629,27 +632,22 @@ fi
>  
>  fi
>  
> -fi
> -
> -GIT_CONF_SUBST([NO_CURL])
>  
>  #
>  # Define NO_EXPAT if you do not have expat installed.  git-http-push is
>  # not built, and you cannot push using http:// and https:// transports.
>  
> -# Respect --without-expat
> -if test "x$NO_EXPAT" != xYesPlease; then
> -
>  GIT_STASH_FLAGS($EXPATDIR)
>  
> +# Respect --without-expat
> +if test -z "$NO_EXPAT"; then
>  AC_CHECK_LIB([expat], [XML_ParserCreate],
>  [NO_EXPAT=],
>  [NO_EXPAT=YesPlease])
> +fi
>  
>  GIT_UNSTASH_FLAGS($EXPATDIR)
>  
> -fi
> -
>  GIT_CONF_SUBST([NO_EXPAT])
>  
>  #
> -- 
> 2.26.0.rc2.234.g969ad452ca
> 

-- 
Danh

Re: [PATCH v2 0/3] configure: respect --without-<package> flags

[Junio C Hamano]

Danh Doan <congdanhqx@gmail.com> writes:

> On 2020-03-19 10:58:54+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
>> Current git's configure script allow --without-<package>,
>> it rightfully set NO_<PACKAGE> in the beginning of the configure script,
>> but overwrite them with autodetect later.
>> 
>> Respect them now.
>
> Hi Junio,
>
> Can I get your opinion on this serie?

I do not have opinion on everything ;-)  If you sent it during the
pre-release freeze, it is very possible that I totally ignored it,
especially if nobody showed interest in the topic.

Not limited to this topic, but I'd prefer to see leftover topics
reappear after the tip of 'next' gets rewind.

Thanks.