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

configure: always check for pthreads when --enable-pthreads is specified #1295

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@Keruspe

Keruspe commented Feb 28, 2017

Otherwise if threaded resolver is disabled, configure will fail with
configure: error: --enable-pthreads but pthreads was not found

configure: always check for pthreads when --enable-pthreads is specified
Otherwise if threaded resolver is disabled, configure will fail with
configure: error: --enable-pthreads but pthreads was not found

Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
@mention-bot

This comment has been minimized.

mention-bot commented Feb 28, 2017

@Keruspe, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @yangtse and @dfandrich to be potential reviewers.

@jay jay added the build label Feb 28, 2017

@jay

This comment has been minimized.

Member

jay commented Feb 28, 2017

Can you give me the exact options to reproduce this in configure? When --enable-pthreads was added I built with and without the threaded resolver and didn't have this problem. Note pthreads detection is supposed to be ignored if --disable-rt is used. Perhaps we should add an error in case of explicit --disable-rt --enable-phtreads:

diff --git a/configure.ac b/configure.ac
index 1e76c49..55d057f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -3420,6 +3420,9 @@ AC_HELP_STRING([--disable-pthreads],[Disable POSIX threads]),
        want_pthreads=no
        ;;
   *)   AC_MSG_RESULT(yes)
+       if test "$dontwant_rt" = "yes"; then
+         AC_MSG_ERROR([options --disable-rt and --enable-pthreads are mutually exclusive])
+       fi
        want_pthreads=yes
        ;;
   esac ], [
@Keruspe

This comment has been minimized.

Keruspe commented Feb 28, 2017

Just ./configure --enable-pthreads without any other argument.

The problem is that pthreads checks are only performed if --enable-rt, and afterwards, if --enable-pthreads was specified and pthreads not found, we hit an error. As the script never actually checked for pthreads, we hit that error, which is really misleading.

The two solution here I guess are either to error out as you suggested or make --enable-pthreads a noop when --disable-rt (which is what my patch does)

jay added a commit to jay/curl that referenced this pull request Mar 1, 2017

configure: fix for --enable-pthreads. draft1
Better handle some options conflicts that can occur if --enable-pthreads.

Bug: curl#1295
Reported-by: Marc-Antoine Perennou
@jay

This comment has been minimized.

Member

jay commented Mar 1, 2017

Thanks, I see it now. Unfortunately to handle all the combinations is a little more complicated than I thought. Can you please try this:
https://github.com/curl/curl/compare/master...jay:fix_configure_enable-pthreads?expand=1

@Keruspe

This comment has been minimized.

Keruspe commented Mar 1, 2017

seems to work fine here with your patch

jay added a commit that referenced this pull request Mar 2, 2017

configure: fix for --enable-pthreads
Better handle options conflicts that can occur if --enable-pthreads.

Bug: #1295
Reported-by: Marc-Antoine Perennou
@jay

This comment has been minimized.

Member

jay commented Mar 2, 2017

Thanks, landed in 5f139d6.

@jay jay closed this Mar 2, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 24, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.