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

fix non pkg-config NSS support kludge #171

Closed
wants to merge 1 commit into from
Closed

fix non pkg-config NSS support kludge #171

wants to merge 1 commit into from

Conversation

mostynb
Copy link
Contributor

@mostynb 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
Copy link
Contributor

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

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

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

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

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

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

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.

Allow --with-nss=path/to/nss as the help text says is accepted.
@mostynb
Copy link
Contributor Author

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

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

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

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

kdudka commented Apr 20, 2015

Great, I will have a look. Thanks!

@mostynb mostynb deleted the nss_crosscompilation_fixups branch April 21, 2015 12:54
jgsogo pushed a commit to jgsogo/curl that referenced this pull request Oct 19, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

3 participants