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

fix non pkg-config NSS support kludge #171

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@mostynb
Contributor

mostynb commented Mar 21, 2015

I ran into some trouble trying to cross-compile curl with NSS support. It seems that the "Without pkg-config, we'll kludge in some defaults" settings are slightly off- this patch fixes that.

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Mar 21, 2015

What is actually the system you are trying to build curl on?

On Fedora 21, pkg-config --libs nss returns -lssl3 -lsmime3 -lnss3 -lnssutil3 -lplds4 -lplc4 -lnspr4 -lpthread -ldl. Also the include directories are named nss3 and nspr4. But as long as the patch fixes the build for you, I am all for applying it because on Fedora both pkg-config and nss-config can be used to detect nss libs properly.

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Mar 21, 2015

Reading it once again, I see you are cross-compiling, so pkg-config is not an option. But what is actually the target platform in your case?

@mostynb

This comment has been minimized.

Contributor

mostynb commented Mar 21, 2015

I am cross-compiling for several embedded linux targets: primarily testing with a broadcom mipsel target, but also with some ARM targets, some are glibc based and some are uClibc based.

I built NSS from source, and have pkg-config files that work with other build systems- I'm not sure why autoconf doesn't want to use pkg-config here (I can help debug that later if it would be useful for others too). I think the nss3 & nspr4 directory names might be fedora/redhat-specific, ubuntu and my builds from NSS source both have /usr/include/{nss,nspr}.

@bagder

This comment has been minimized.

Member

bagder commented Mar 21, 2015

@mostynb if your cross-compiled NSS build also include a pkg-config it is certainly worth checking why curl's configure can't find and use that, as it is usually so much better to get that with more accurate info rather than to "guess" on libs and paths. The parts from config.log where it checks for the NSS pkg-config should be able to give a first clue what checks that go wrong...

@mostynb

This comment has been minimized.

Contributor

mostynb commented Mar 29, 2015

@bagder: I think the problem with pkg-config not being used is due to inconsistent ./configure --help text. This is what it says re NSS:
--with-nss=PATH where to look for NSS, PATH points to the
installation root

But what the configure script actually does is check if the option is "no" or "yes". If the value is "no" then NSS support is disabled, and if the value is "yes" then pkg-config is used. If you actually specify the NSS prefix path as in the help text, then it skips down to the "Without pkg-config, we'll kludge in some defaults" code, which has incorrect default values (without this patch).

IMO the default values should be changed as in this pull request, and we should also resolve the help text vs the configure script behaviour. What options do you want to be considered valid for the --with-nss flag, and which should be assumed if no option is specified?

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Mar 30, 2015

I remember we changed this behavior for Openssl few years ago: 530fde3 We can apply something like this for NSS. By the way, I am not against applying your patch, too, if it makes curl build in your case.

@bagder

This comment has been minimized.

Member

bagder commented Mar 30, 2015

I think using the given path to try to find the pkg-config is a sensible improvement and only if not present do the "guess the libs" part.

@mostynb

This comment has been minimized.

Contributor

mostynb commented Mar 30, 2015

I can try implementing this, if you agree:

  • if --with-nss=yes or just --with-nss is specified then try pkg-config, fall back to default guessed values
  • if --with-nss=some/path is specified then use that with the guessed values
  • update the --with-nss help text to document these

I will probably want to fix up some whitespace formatting in this code first, it's a bit of a mess and difficult to follow at the moment.

@bagder

This comment has been minimized.

Member

bagder commented Mar 30, 2015

With the little correction that even with --with-nss=some/path it should first check if there's a pkg-config file on that specified path present to use to get the data from, and only if there isn't any pkg-config present we guess.

@mostynb

This comment has been minimized.

Contributor

mostynb commented Apr 6, 2015

I have reworked this autoconf code a little- I have given it some basic testing but will follow up with some more thorough checks tomorrow.

fix pkg-config configuration options
Allow --with-nss=path/to/nss as the help text says is accepted.
@mostynb

This comment has been minimized.

Contributor

mostynb commented Apr 7, 2015

Cross compilation WFM with --with-nss=yes (and PKG_CONFIG_PATH) and --with-nss=path/to/nss. I think this works as expected, so I have squashed the commits on my branch.

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Apr 8, 2015

Thanks for the patch! However, I do not think that a major rewrite was necessary. Also the values "yes" and "no" are not supposed to be explicitly specified by user. Those are rather used internally by autoconf to represent the --with/without-nss options without any argument specified. I would prefer not to mention them in the output of configure --help to stay consistent with other --with/without-... options.

I have proposed a patch that just makes --with-nss=PATH work with pkg-config:

http://curl.haxx.se/mail/lib-2015-04/0040.html

If we still need to change the hard-wired default for libs and compiler flags, I would prefer that as a separate commit.

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Apr 17, 2015

Pushed as 691a07d...8dc3bbf.

Please let me know if there is anything else needed to get your cross-compilation running.

@kdudka kdudka closed this Apr 17, 2015

@mostynb

This comment has been minimized.

Contributor

mostynb commented Apr 20, 2015

Sorry it took a while to get back to you. Your change works for me.

I will submit one or two trivial cleanup pull requests, while this is fresh in my memory.

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Apr 20, 2015

Great, I will have a look. Thanks!

@mostynb mostynb deleted the mostynb:nss_crosscompilation_fixups branch Apr 21, 2015

jgsogo added a commit to jgsogo/curl that referenced this pull request Oct 19, 2015

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