Bug 59830 - These are the changes required to build against spidermonkey esr17/js188.
Summary: These are the changes required to build against spidermonkey esr17/js188.
Status: RESOLVED FIXED
Alias: None
Product: PolicyKit
Classification: Unclassified
Component: daemon (show other bugs)
Version: unspecified
Hardware: All All
: medium normal
Assignee: David Zeuthen (not reading bugmail)
QA Contact: David Zeuthen (not reading bugmail)
URL:
Whiteboard:
Keywords:
: 51236 (view as bug list)
Depends on: 59781
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-25 01:35 UTC by darkxst
Modified: 2013-04-23 14:51 UTC (History)
5 users (show)

See Also:
i915 platform:
i915 features:


Attachments
api changes for js188 (6.31 KB, patch)
2013-01-25 01:37 UTC, darkxst
Details | Splinter Review
build patch for js188 (1.17 KB, patch)
2013-01-25 01:42 UTC, darkxst
Details | Splinter Review
0001-jsauthority-Use-JSVAL_NULL-rather-than-0-struct-init.patch (1.84 KB, patch)
2013-04-12 21:24 UTC, Colin Walters
Details | Splinter Review
0002-Revert-Dynamically-load-libmozjs185.so-and-cope-with.patch (32.26 KB, patch)
2013-04-12 21:24 UTC, Colin Walters
Details | Splinter Review
0003-jsauthority-Work-with-mozjs-17.0-too.patch (5.26 KB, patch)
2013-04-12 21:25 UTC, Colin Walters
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description darkxst 2013-01-25 01:35:32 UTC

    
Comment 1 darkxst 2013-01-25 01:37:01 UTC
Created attachment 73627 [details] [review]
api changes for js188
Comment 2 darkxst 2013-01-25 01:42:45 UTC
Created attachment 73628 [details] [review]
build patch for js188

Given the dlopen stuff, I would not have though linking of libmozjs would be required, however I was getting 'undefined reference' for macros defined in jsapi.h (such as JSVAL_NULL) while building.
Comment 3 Colin Walters 2013-03-13 19:15:25 UTC
*** Bug 51236 has been marked as a duplicate of this bug. ***
Comment 4 Colin Walters 2013-03-13 19:52:33 UTC
This effectively reverts the dynamic loading work from http://cgit.freedesktop.org/polkit/commit/?id=34cb6353b9affd6c04ea480df5fc39d0ca72319d

The commit message doesn't really explain why, but from what I understand, it was primarily requested by people making "minimal cloud images" and the like, where they may not actually be making use of polkit at runtime.  

(Sadly these people would be better served by compiling from source, but let's leave that aside).

So I think before we move forward with this patch, we'll have to figure out how to keep the dynamic loading.


There is a secondary high level concern, which was discussed in https://bugzilla.gnome.org/show_bug.cgi?id=690982 - basically just bumping the mozjs requirement is going to be painful.

Given the current scope of changes, it does look to me like polkit could get by with #ifdef.  I can investigate this and see.
Comment 5 darkxst 2013-03-13 23:39:32 UTC
(In reply to comment #4)
> This effectively reverts the dynamic loading work from
> http://cgit.freedesktop.org/polkit/commit/
> ?id=34cb6353b9affd6c04ea480df5fc39d0ca72319d
> 
> The commit message doesn't really explain why, but from what I understand,
> it was primarily requested by people making "minimal cloud images" and the
> like, where they may not actually be making use of polkit at runtime.  
> 
> (Sadly these people would be better served by compiling from source, but
> let's leave that aside).
> 
> So I think before we move forward with this patch, we'll have to figure out
> how to keep the dynamic loading.
> 


I could never get dynamic loading to work with the new spidermonkey library, probably because the mozilla build scripts seem to strip out the --export-dynamic flag. 

build/autoconf/pkg.m4:            ## Remove evil flags like -Wl,--export-dynamic
Comment 6 Colin Walters 2013-04-11 19:18:17 UTC
Ok, so I looked into this.  It has nothing to do with -export-dynamic, the problem is now JSVAL_NULL is a global data symbol, and not a #define anymore.  See:

extern JS_PUBLIC_DATA(const jsval) JSVAL_NULL;

So we'd need to dlsym() it like everything else.  Or theoretically we could just hack up our own local copy of:

const jsval JSVAL_NULL  = IMPL_TO_JSVAL(BUILD_JSVAL(JSVAL_TAG_NULL,      0));


Regardless though, what I'm thinking now it's that it'd be a **whole** lot simpler if we just had a build-time option --enable-javascript-rules.  Then GNOME (and its downstreams) could turn that on, and "tiny" downstreams (minimal cloud images) could disable it.

For binary package-based systems like Fedora and Debian that want to be a universe of bits that can be dynamically assembled per use case, they could build polkit twice, and have e.g. "polkit" and "polkit-nojs" packages.
Comment 7 Colin Walters 2013-04-11 19:27:26 UTC
David, Miroslav, what do you think?
Comment 8 Colin Walters 2013-04-11 19:28:28 UTC
Er, Miloslav, sorry, lots of tyops today.
Comment 9 David Zeuthen (not reading bugmail) 2013-04-11 22:01:53 UTC
Actually, I think we should just drop the whole "load JS interpreter dynamically" approach and just link statically. It never really made much sense and was added more or less as a compromise when I was at Red Hat in order to appease some people concerned about having js.rpm installed. Something about that this package is often updated for security issues and this tends to scare some people. I can't remember the details.

Anyway, I don't think it's unreasonable to ask Red Hat to just carry patches for this themselves if they want to hang on to that feature (probably want for RHEL7). Thoughts?
Comment 10 Samuli Suominen 2013-04-11 22:20:27 UTC
(In reply to comment #9)
> Actually, I think we should just drop the whole "load JS interpreter
> dynamically" approach and just link statically.
> It never really made much
> sense and was added more or less as a compromise when I was at Red Hat in
> order to appease some people concerned about having js.rpm installed.
> Something about that this package is often updated for security issues and
> this tends to scare some people. I can't remember the details.
> 
> Anyway, I don't think it's unreasonable to ask Red Hat to just carry patches
> for this themselves if they want to hang on to that feature (probably want
> for RHEL7). Thoughts?

When the system spidermonkey (libjs) package gets updated for security, like for an CVE, then polkit would need a recompile too.
And when user runs something like gjs linked against dynamic libjs, he'd get another copy running within polkit?
I don't see any benefits in linking it statically, only massive headacke.
Would be something we'd need to patch out at distribution level since it'd be against the QA policies of every distribution I know of -- "Useless static linking"
Comment 11 David Zeuthen (not reading bugmail) 2013-04-11 22:38:47 UTC
(In reply to comment #10)

Sorry, I meant "just link dynamically" (as opposed to dlopen() as we do today) instead of "just link statically. (FWIW, I agree it would be non-sense to link _statically_.)
Comment 12 Samuli Suominen 2013-04-11 22:47:38 UTC
(In reply to comment #11)
> (In reply to comment #10)
> 
> Sorry, I meant "just link dynamically" (as opposed to dlopen() as we do
> today) instead of "just link statically. (FWIW, I agree it would be
> non-sense to link _statically_.)

phew!  i'm really glad to hear this.  :-)

for source based distributions it doesn't make much difference at all if it's NEEDED or dlopen()'d but I can understand how it can be useful for binary distributions if the purpose is to keep libjs optional
i'll stop talking now.
Comment 13 Miloslav Trmac 2013-04-12 14:56:33 UTC
(In reply to comment #9)
> Actually, I think we should just drop the whole "load JS interpreter
> dynamically" approach and just link statically. 

That's my strong preference as well, because it means consistent polkit semantics, and more compiler checking.

> It never really made much
> sense and was added more or less as a compromise when I was at Red Hat in
> order to appease some people concerned about having js.rpm installed.

AFAIK the major concerns were different and the dlopen() stuff didn't resolve them, so...

> Anyway, I don't think it's unreasonable to ask Red Hat to just carry patches
> for this themselves if they want to hang on to that feature (probably want
> for RHEL7).

... for Fedora I intend to have a hard dependency on the JS intepreter in any case, even if the code continues to use dlopen().


(I also want to resurrect pkla, which is another story, but having a hard dependency on the JS interpreter will actually make it easier to carry as a local patch.  So one more eason to support just linking to the interpreter.)
Comment 14 Colin Walters 2013-04-12 15:29:20 UTC
(In reply to comment #13)

> AFAIK the major concerns were different and the dlopen() stuff didn't
> resolve them, so...

Hm, really?  I thought it came out of the minimal cloud push.  Well OK, I'll take a look at linking dynamically then, and once we do that, if someone cares a --disable-javascript build time option would be pretty easy.
Comment 15 David Zeuthen (not reading bugmail) 2013-04-12 15:38:33 UTC
(In reply to comment #13)
> (I also want to resurrect pkla, which is another story, but having a hard
> dependency on the JS interpreter will actually make it easier to carry as a
> local patch.  So one more eason to support just linking to the interpreter.)

Please don't. Please also don't ship it as a patch in Fedora as it will make it look like upstream supports it (which we of course won't). If all you want is a file-format instead of JS (and I'd argue such a desire is misguided given the nature of JS), I would suggest coming up with a simpler format than .pkla (and parse it from a JS rules files, at least for a first cut).
Comment 16 Miloslav Trmac 2013-04-12 15:52:52 UTC
(In reply to comment #15)
> (In reply to comment #13)
> > (I also want to resurrect pkla, which is another story, but having a hard
> > dependency on the JS interpreter will actually make it easier to carry as a
> > local patch.  So one more eason to support just linking to the interpreter.)
> 
> Please don't. Please also don't ship it as a patch in Fedora as it will make
> it look like upstream supports it (which we of course won't).

With a mandatory JS interpreter it can be done as a .rule that calls out to external helpers, which is as separate as it can get while still having a single consistent behavior within the distribution.

> If all you
> want is a file-format instead of JS (and I'd argue such a desire is
> misguided given the nature of JS), I would suggest coming up with a simpler
> format than .pkla (and parse it from a JS rules files, at least for a first
> cut).

Well, long-term I want an unified system-wide ACL/permissions mechanism, and neither .pkla nor anything else ran from the JS backend can be it (or at least it's not known that it can be it at this time).  So any file format would ideally be medium-term thing (3-5 years) and at that point it's just easier for everyone if we don't introduce a new format, especially a new Fedora-only format.
Comment 17 Colin Walters 2013-04-12 16:37:34 UTC
(In reply to comment #14)
> (In reply to comment #13)
> 
> > AFAIK the major concerns were different and the dlopen() stuff didn't
> > resolve them, so...
> 
> Hm, really?  I thought it came out of the minimal cloud push.  Well OK, I'll
> take a look at linking dynamically then,

This turns out to be just matter of reverting 34cb6353b9affd6c04ea480df5fc39d0ca72319d and fixing up a few conflicts.  Patches to come.
Comment 18 Colin Walters 2013-04-12 21:24:08 UTC
Created attachment 77911 [details] [review]
0001-jsauthority-Use-JSVAL_NULL-rather-than-0-struct-init.patch
Comment 19 Colin Walters 2013-04-12 21:24:28 UTC
Created attachment 77912 [details] [review]
0002-Revert-Dynamically-load-libmozjs185.so-and-cope-with.patch
Comment 20 Colin Walters 2013-04-12 21:25:13 UTC
Created attachment 77913 [details] [review]
0003-jsauthority-Work-with-mozjs-17.0-too.patch

All compiler warnings fixed, lightly tested in gnome-ostree buildmaster.
Comment 21 darkxst 2013-04-12 22:14:25 UTC
I guess this doesnt matter too much, however JS_VERSION has been deprecated and will be removed in the next major mozjs24 release. It has been replaced by MOZJS_MAJOR_VERSION and friends.
Comment 22 Colin Walters 2013-04-13 22:48:22 UTC
(In reply to comment #21)
> I guess this doesnt matter too much, however JS_VERSION has been deprecated
> and will be removed in the next major mozjs24 release. It has been replaced
> by MOZJS_MAJOR_VERSION and friends.

Yeah, that seems like an easy thing to deal with when updating to the next version.
Comment 23 Miloslav Trmac 2013-04-18 20:40:42 UTC
(In reply to comment #20)
> Created attachment 77913 [details] [review] [review]
> 0003-jsauthority-Work-with-mozjs-17.0-too.patch
> 
> All compiler warnings fixed, lightly tested in gnome-ostree buildmaster.

All three look good, test suite passes with both versions.

(I wouldn't mind a version that didn't have #ifdefs and just required the newer library,  OTOH I assume we don't expect many changes to the backend anyway, so the #ifdefs don't hurt much, and might make it easier on users, so both alternatives are fine with me.)
Comment 24 Colin Walters 2013-04-23 14:51:08 UTC
(In reply to comment #23)
> (In reply to comment #20)
> > Created attachment 77913 [details] [review] [review] [review]
> > 0003-jsauthority-Work-with-mozjs-17.0-too.patch
> > 
> > All compiler warnings fixed, lightly tested in gnome-ostree buildmaster.
> 
> All three look good, test suite passes with both versions.

Thanks, pushed all 3 patches.  Let's open a new bug for any non-js17 discussion that was in this bug.

> (I wouldn't mind a version that didn't have #ifdefs and just required the
> newer library,  OTOH I assume we don't expect many changes to the backend
> anyway, so the #ifdefs don't hurt much, and might make it easier on users,
> so both alternatives are fine with me.)

Yeah, it wasn't very hard so I decided to make things a bit easier for downstreams.  In contrast for gjs it would have been *really* ugly, so we just branched and declared the mozjs185 legacy.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.