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.

Sign up for GitHub

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

Jump to bottom

lnav: fix argument to --with-libcurl and simplify #52

Closed
wants to merge 1 commit into from
Closed

lnav: fix argument to --with-libcurl and simplify #52

wants to merge 1 commit into from

Conversation

ilovezfs
Copy link
Contributor

@ilovezfs ilovezfs commented Apr 4, 2016

No description provided.

@DomT4
Copy link
Member

DomT4 commented Apr 4, 2016

👍

@apjanke
Copy link
Contributor

apjanke commented Apr 5, 2016

Should this get a revision bump if the old way was not pulling in the curl lib properly?

@tdsmith
Copy link
Contributor

tdsmith commented Apr 5, 2016

libssh2 is already linked to libssl and libcrypto; unless lnav uses openssl symbols directly it shouldn't have to link openssl again? Am I confused or is there broken encapsulation somewhere?

@DomT4
Copy link
Member

DomT4 commented Apr 5, 2016

Should this get a revision bump if the old way was not pulling in the curl lib properly?

We don't bother on optional things, since to date we've had no idea how heavily they're used and whether it's worthwhile making even users who haven't built with the option rebuild.

Am I confused or is there broken encapsulation somewhere?

It does work fine without the ENV.append tweak locally, finally having a moment to test it. So having only the arg passed in-formula works fine, now that it actually points to the right place? @ilovezfs reproduced the failure in Slack earlier I believe, maybe I'm forgetting something from that chat.

@ilovezfs ilovezfs changed the title lnav: don't set bin, fix opt_lib path lnav: fix curl opt_lib and simplify curl handling Apr 6, 2016
@ilovezfs
Copy link
Contributor Author

ilovezfs commented Apr 6, 2016

@apjanke @DomT4 @tdsmith PR refreshed.

@ilovezfs
lnav: fix argument to --with-libcurl and simplify
894074f 
- The flag "--with-libcurl" wants curl's `opt_lib` not `opt_prefix`
- There's no need to add curl's `bin` to the PATH or to add
  "-lcrypto -lssl" to the LIBS environment variable. This happens
  indirectly via libssh2.
@ilovezfs ilovezfs changed the title lnav: fix curl opt_lib and simplify curl handling lnav: fix argument to --with-libcurl and simplify Apr 6, 2016
@DomT4
Copy link
Member

DomT4 commented Apr 6, 2016

👍 Feel free to ship when 🍏.

@ilovezfs ilovezfs closed this in b7b3a0d Apr 6, 2016
@ilovezfs ilovezfs deleted the lnav-curl-tweaks branch April 9, 2016 04:42
@wjj2329 wjj2329 mentioned this pull request Oct 27, 2017
Closed
@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ilovezfs @DomT4 @apjanke @tdsmith