Skip to content

Make hyper opt-in, and fail when missing - #6598

Closed
jsha wants to merge 1 commit into
curl:masterfrom
jsha:configure-error-on-missing-libs
Closed

Make hyper opt-in, and fail when missing#6598
jsha wants to merge 1 commit into
curl:masterfrom
jsha:configure-error-on-missing-libs

Conversation

@jsha

@jsha jsha commented Feb 12, 2021

Copy link
Copy Markdown
Contributor

Previously, configure would look for hyper by default, and use it if
found; otherwise it would not use hyper, and not error.

Now, configure will not look for hyper unless --with-hyper is passed. If
configure looks for hyper and fails, it will error.

Also, add -ld -lpthread -lm to Hyper's libs. I think they are required.

@bagder bagder left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this isn't sufficient as it seems we run through this condition when the option isn't even specified so this change causes build failures. example

@bagder bagder left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

breaks the build

@jsha

jsha commented Feb 12, 2021

Copy link
Copy Markdown
Contributor Author

I had assumed the ./configure behavior was not to try hyper unless --with-hyper was passed, but looking again I see that the behavior is actually to try always look for hyper and use it if the library is available. What do you think of changing that, so building with hyper requires the --with-hyper flag?

Rationale: I was trying to build with Hyper yesterday, and was having trouble getting it configured correctly. I needed to make sure to build with the right feature flags and the right RUSTFLAGS, and put it in the right place, and set up LD_LIBRARY_PATH and CFLAGS=-L/usr/local/lib. That's all fine: part of the software life. But my debugging cycles were longer than necessary, because if my Hyper install was broken, ./configure --with-hyper would happily continue and set up curl without Hyper. Given that Hyper is still experimental, it seems sensible to require both the flag and the library install to turn it on. That would enable us to treat OPT_HYPER=yes as a stronger signal of user intent.

@bagder

bagder commented Feb 14, 2021

Copy link
Copy Markdown
Member

I had assumed the ./configure behavior was not to try hyper unless --with-hyper was passed, but looking again I see that the behavior is actually to try always look for hyper and use it if the library is available. What do you think of changing that, so building with hyper requires the --with-hyper flag?

I think it was a mistake. I think we should require the --with-hyper flag for a hyper build. In particular because the backend is still marked experimental but also because I think users expect it to behave that way!

Previously, configure would look for hyper by default, and use it if
found; otherwise it would not use hyper, and not error.

Now, configure will not look for hyper unless --with-hyper is passed. If
configure looks for hyper and fails, it will error.

Also, add -ld -lpthread -lm to Hyper's libs. I think they are required.
@jsha
jsha force-pushed the configure-error-on-missing-libs branch from bd43f30 to 067fddb Compare February 18, 2021 04:26
@jsha jsha changed the title Make configure fail when requested lib missing. Make hyper opt-in, and fail when missing Feb 18, 2021
@jsha

jsha commented Feb 18, 2021

Copy link
Copy Markdown
Contributor Author

Updated the change (including PR title and description) based on the conversation above.

@jsha
jsha requested a review from bagder February 18, 2021 04:52
@bagder

bagder commented Feb 23, 2021

Copy link
Copy Markdown
Member

Thanks!

@bagder bagder closed this in aab3c6c Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants