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

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

Closed
wants to merge 1 commit into from

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Feb 12, 2021

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.

Copy link
Member

@bagder bagder left a comment

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

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

breaks the build

@jsha
Copy link
Contributor Author

jsha commented Feb 12, 2021

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
Copy link
Member

bagder commented Feb 14, 2021

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
Copy link
Contributor Author

jsha commented Feb 18, 2021

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
Copy link
Member

bagder commented Feb 23, 2021

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.

None yet

2 participants