504022 - Crypto Build system should use pkg-config

Status: RESOLVED WONTFIX

(Firefox :: Sync, enhancement)

Description

[Nirbheek Chauhan]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1) Gecko/20090630 Gentoo Firefox/3.5
Build Identifier: 

The Crypto Build-system currently relies on environment variables and hard-coded paths for finding xulrunner includes (dependant on the dir structure of installed xulrunner-sdk), and nss/nspr includes (assumes that xulrunner uses inbuilt nss/nspr).

Using a program explicitly made for the task of finding libraries, includes, etc would be less prone to unexpected failures for us 1%

Reproducible: Always




An example:

headers = -I$(sdkdir)/include \
[snip]
          -I$(sdkdir)/include/nss \
[snip]
          -I$(sdkdir)/include/nspr

If xulrunner is built with system nss/nspr, those directories will not exist. mozilla-{nss,nspr}.pc contain the correct paths at all times.

Comment 1

[Ed Lee :Mardak]

thunder should probably take a look.. I'm assuming he did some of the weave crypto build system. (p.s., we have a new component to watch: [e-mail])

Comment 2

[Nirbheek Chauhan]

Attachment: Use pkg-config if available

Attached patch tries to use pkg-config if sdkdir/MOZSDKDIR are not defined.

Patch will also apply after bug 503429 is merged, just needs to be applied inside /crypto/ then

Comment 3

[Dan Mills [:thunder]]

Looks good, I think, but it doesn't apply anymore.  I replaced "src" with "crypto" in the patch and:

host-3-191:weave thunder$ patch --dry-run -p1 < ~/mozilla-weave-use-pkgconfig.patch 
patching file Makefile
Hunk #1 FAILED at 39.
1 out of 1 hunk FAILED -- saving rejects to file Makefile.rej
patching file crypto/Makefile
Hunk #1 FAILED at 37.
Hunk #2 FAILED at 142.
Hunk #3 FAILED at 150.
Hunk #4 FAILED at 161.
Hunk #5 FAILED at 340.
5 out of 5 hunks FAILED -- saving rejects to file crypto/Makefile.rej

Comment 4

[Nirbheek Chauhan]

Attachment: Updated patch

(In reply to comment #3)
> Looks good, I think, but it doesn't apply anymore.  I replaced "src" with
> "crypto" in the patch and:
> 

Well, after the changes in bug 503429 the patch should've been applied inside crypto/ :)

The attached patch should apply to tip just fine (in the root directory)

PS: Sorry for the late reply, I was out of town

Comment 5

[Nirbheek Chauhan]

Ping? :)

Comment 6

[Jory A. Pratt [:anarchy]]

Nirbheek you need to put the patch up for review, if you do not know how let me know and I will assist you.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Updated patch managing WINCE

Updated patch on 24/08/2009 version of the repository.
I've added management of WINCE checks.

Let me know what should I do for helping reviewing.

Comment 8

[Mounir Lamouri (:mounir)]

The patch is still working with 1.0b2.
Anyone to review/push ?

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 396163 [details] [diff] [review]
Updated patch managing WINCE

Punting this to dolske, he knows this way better than I want to... :)

Comment 10

[Justin Dolske [:Dolske]]

What problem is this actually solving?

Most of the crypto Makefile goop goes away with bug 513798.

Comment 11

[Nirbheek Chauhan]

(In reply to comment #10)
> What problem is this actually solving?
> 

The patch made weave use the standard way of finding includes/libraries (pkg-config) instead of using environment variables, etc.

> Most of the crypto Makefile goop goes away with bug 513798.

I see from that bug that no compilation will be required at all (against libxul-unstable, nss, nspr or whatever); in which case pkg-config will be of no use.

So, I suppose this bug has been obsoleted by that one :)

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 396163 [details] [diff] [review]
Updated patch managing WINCE

Clearing review flag, js-ctypes will save us.

Comment 13

[Mike Connor [:mconnor]]

js-ctypes is trunk only, and some ways off...

Comment 14

[Mike Connor [:mconnor]]

We're in the process of abandoning the C++ component, marking as WONTFIX.