Closed Bug 460913 Opened 11 years ago Closed 11 years ago

Installer shouldn't copy xulrunner files into Firefox install directory

Categories

(Firefox Build System :: General, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: reed, Assigned: fta+bugzilla)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 4 obsolete files)

Attached patch patch - v1 (obsolete) — Splinter Review
Patch to prevent the installer to dump xulrunner files into firefox install dir.

Taken from Ubuntu bzr repo.
Attachment #344021 - Flags: review?(ted.mielczarek)
Comment on attachment 344021 [details] [diff] [review]
patch - v1

We *want* to copy xulrunner by default, so that the app is usable without doing an install. If anything, we should change the install rule not to install those files...
Attachment #344021 - Flags: review?(ted.mielczarek) → review-
fta, can you supply a patch to do what bsmedberg proposes?
can't we just add a flag like the --with-system-jpeg, which would make a SYSTEM_JPEG= ?

--with-system-libxul and SYSTEM_LIBXUL  ?

Apps could then use SYSTEM_LIBXUL to copy or not copy xulrunner with the app.
(In reply to comment #3)
> can't we just add a flag like the --with-system-jpeg, which would make a
> SYSTEM_JPEG= ?

I don't think we should add --with-system-jpeg. Our libjpeg is completely different than anything the distros ship (afaict), so unless they start taking our libjpeg as their system jpeg, we shouldn't even offer that as an option.

> --with-system-libxul and SYSTEM_LIBXUL  ?
> 
> Apps could then use SYSTEM_LIBXUL to copy or not copy xulrunner with the app.

There's already --with-libxul-sdk... does that meet this need?
(In reply to comment #4)
> (In reply to comment #3)
> > can't we just add a flag like the --with-system-jpeg, which would make a
> > SYSTEM_JPEG= ?
> 
> I don't think we should add --with-system-jpeg. Our libjpeg is completely
> different than anything the distros ship (afaict), so unless they start taking
> our libjpeg as their system jpeg, we shouldn't even offer that as an option.

Whoops! This was just an example. Since we already have flags like --with-system-jpeg, I think we could add a flag for using the system xulrunner.

> 
> > --with-system-libxul and SYSTEM_LIBXUL  ?
> > 
> > Apps could then use SYSTEM_LIBXUL to copy or not copy xulrunner with the app.
> 
> There's already --with-libxul-sdk... does that meet this need?

No. --with-libxul-sdk only points to an existing LibXUL SDK (not the same as a system libxul runtime) which is used for compiling the app _and_ is used as the source when copying the xulrunner files into the app bundle.

We would need an explicit flag to tell us that we are using the system libxul runtime and we should not package a xulrunner runtime with the app.
Attached patch patch - v2 (obsolete) — Splinter Review
This patch adds --with-system-libxul-sdk. It requires --with-libxul-sdk=PFX.
It is disabled by default.
Attachment #344021 - Attachment is obsolete: true
Attachment #354889 - Flags: review?
Attached patch patch - v2b (obsolete) — Splinter Review
Oops. I didn't plan to force people to use xul-sdk. Fixed.
Attachment #354889 - Attachment is obsolete: true
Attachment #354891 - Flags: review?
Attachment #354889 - Flags: review?
Attached patch patch - v2c (obsolete) — Splinter Review
Sorry, wrong version.
Attachment #354891 - Attachment is obsolete: true
Attachment #354893 - Flags: review?
Attachment #354891 - Flags: review?
Probably just a nit, but maybe SYSTEM_LIBXUL_SDK should be SYSTEM_LIBXUL. When talking about the system xulrunner are we talking about the SDK or the runtime? I'm assuming the runtime, so we could drop the _SDK from the flag.

If we really mean the SDK and not the runtime, then fine by me.

Also, let's get benjamin to look at this.
Attachment #354893 - Flags: review? → review?(benjamin)
Attachment #354893 - Flags: review?(benjamin) → review+
bsmedberg, did you prefer SYSTEM_LIBXUL_SDK or SYSTEM_LIBXUL as the name?
SYSTEM_LIBXUL is slightly better.
My logic was foo -> system-foo

So --with-libxul-sdk => --with-system-libxul-sdk and
LIBXUL_SDK => SYSTEM_LIBXUL_SDK

But I don't mind. Feel free to change.
Fabien - update the patch (use SYSTEM_LIBXUL) and add checkin-needed keyword?
Attached patch patch - v2dSplinter Review
v2c + comment #11 addressed
Attachment #354893 - Attachment is obsolete: true
Keywords: checkin-needed
I need it in 1.9.1 too. For fennec, prism, and other xul apps.
Flags: wanted1.9.1?
http://hg.mozilla.org/mozilla-central/rev/b3facc5a7ccb
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Attachment #357017 - Flags: approval1.9.1?
Attachment #357017 - Flags: approval1.9.0.7?
Comment on attachment 357017 [details] [diff] [review]
patch - v2d

a191=beltzner
Attachment #357017 - Flags: approval1.9.1? → approval1.9.1+
Why do we want this on 1.9.0? Please explain exactly what this patch does, how to test it, and any risk involved. Why can't this wait until 1.9.1?
Comment on attachment 357017 [details] [diff] [review]
patch - v2d

Ubuntu Mozilla Team is fine with just having this one 1.9.1 and newer.
Attachment #357017 - Flags: approval1.9.0.7?
Flags: wanted1.9.1?
Flags: in-testsuite-
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.