search

infinity0 / mozilla-gnome-keyring-legacy

A firefox extension that enables Gnome Keyring integration (legacy version)
https://bugzilla.mozilla.org/show_bug.cgi?id=309807
Other
55 stars 8 forks source link

Makefile: Use -rpath instead of LD_LIBARY_PATH for xpcom_abi #7

Closed fat-lobyte closed 12 years ago

fat-lobyte commented 12 years ago

Hi, please pull my commit about rpath-linking. This simplifies the invocation of the xpcom_abi and fixes cases, where multiple library directories are needed (ubuntu once again).

Makefile:

fat-lobyte commented 12 years ago

Ok, I just read http://wiki.debian.org/RpathIssue, and it might not be a good Idea after all. Please tell me what you think.

infinity0 commented 12 years ago

Hello, I am reading through that article now. In the meantime, I have committed a fix to get_abi being called every single time even on "clean", which was part of an earlier edition of your main comment.

infinity0 commented 12 years ago

OK, that issue doesn't apply to xpcom_abi, because the binary in question is being run in a very specific way and is never distributed to others.

However, if you add it to XUL_LDFLAGS this also affects compilation of TARGET. Here, that issue does apply, and I don't want to risk ignoring its advice especially since there is no need for that extra flag - it works perfectly fine without it since it's loaded by firefox itself.

I suppose you could add it to the xpcom_abi target directly, which would allow us to call ./xpcom_abi directly and get rid of the "get_abi" target.

infinity0 commented 12 years ago

re-comment in case github doesn't send out notification emails on comment-update:

I suppose you could add it to the xpcom_abi target directly, which would allow us to call ./xpcom_abi directly and get rid of the "get_abi" target.

fat-lobyte commented 12 years ago

I suppose you could add it to the xpcom_abi target directly, which would allow us to call ./xpcom_abi directly and get rid of the "get_abi" target.

That seems reasonable. Is it ok if I rebase the pull request to a version of the commit that does that?

infinity0 commented 12 years ago

yes rebase here is alright :)

fat-lobyte commented 12 years ago

yes rebase here is alright :)

Ok, thanks. Please review and if it makes sense, you can merge.

infinity0 commented 12 years ago

merged, thanks!