Closed Bug 594338 Opened 9 years ago Closed 9 years ago

-lmozjs is not available in latest xulrunner SDK... but still present in mozilla-js.pc.in

Categories

(Toolkit Graveyard :: Build Config, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: romaxa, Assigned: romaxa)

References

Details

Attachments

(3 files, 3 obsolete files)

I think -lmozjs should be removed together with mozilla-js.pc.in file

or  it should have -lmozjs added under some ifdef like ifndef MOZ_LIBXUL or something like that.

Benjamin what is correct way to proceed here?
this is preventing us from native extensions and embedding applications building.
Blocks: 580407
Attached patch Possible fix, (obsolete) — Splinter Review
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
Attachment #473804 - Flags: review?
Comment on attachment 473804 [details] [diff] [review]
Possible fix,

maybe we only care about the patch to mozilla-js.pc.in?
Attachment #473804 - Flags: review? → review?(khuey)
Comment on attachment 473804 [details] [diff] [review]
Possible fix,

You need to add MOZ_JS_LINK to the sed command.  r=me with that.
Attachment #473804 - Flags: review?(khuey) → review+
Actually, when js is in libxul, you need to make the flags for js the same as the flags for libxul.
Attached patch Share xul libs (obsolete) — Splinter Review
Attachment #473804 - Attachment is obsolete: true
Attachment #474063 - Flags: review?(khuey)
Attachment #474063 - Attachment is obsolete: true
Attachment #474066 - Flags: review?(khuey)
Attachment #474063 - Flags: review?(khuey)
Comment on attachment 474066 [details] [diff] [review]
Add missing sed part

>+MOZ_XUL_LINK= -lxpcomglue_s -lxul -lxpcom
>+ifdef JS_SHARED_LIBRARY
>+MOZ_JS_LINK= -lmozjs
>+else
>+MOZ_JS_LINK= $(MOZ_XUL_LINK)
>+endif
>+

Nit: lose the spaces after the '='

r=me
Attachment #474066 - Flags: review?(khuey) → review+
Attachment #474068 - Flags: approval2.0?
can we get approval2.0 for that patch? I think distributions will be happy about it
Also Meego mozilla-platform based apps will be happy about proper xulrunner SDK pkgconfig.
Comment on attachment 474068 [details] [diff] [review]
added missing spaces... comment 8 (PUSH ME)

this is a build bustage for people using the mozilla platform.  the test should be that we continue to compile, and these other apps no longer have a problem.  risk is low.

oleg, make sure you run through try before pushing.
Attachment #474068 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/bd56aa8a39cc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 474068 [details] [diff] [review]
added missing spaces... comment 8 (PUSH ME)

> 	-e "s|%NSPR_VERSION%|$(NSPR_VERSION)|" > $@
>+	-e "s|%MOZ_XUL_LINK%|$(MOZ_XUL_LINK)|" \
>+	-e "s|%MOZ_JS_LINK%|$(MOZ_JS_LINK)|" \
> 	chmod 644 $@

don't think that'll work
Heh, you are correct.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch I need to rest :( (obsolete) — Splinter Review
Attachment #474710 - Flags: review?(khuey)
Comment on attachment 474710 [details] [diff] [review]
I need to rest :(

Keep the NSPR stuff together please and just move the '> $@' down to the js link line.
Attachment #474710 - Flags: review?(khuey) → review+
Fixed comment
Attachment #474710 - Attachment is obsolete: true
Attachment #474714 - Flags: review?(khuey)
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/045bfb95fe6a
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.