Closed Bug 418851 Opened 12 years ago Closed 11 years ago

implement a 'make deb' packaging target for xulrunner

Categories

(Firefox Build System :: General, defect)

Other
Maemo
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bhearsum, Assigned: blassey)

References

Details

(Keywords: mobile)

Attachments

(2 files, 6 obsolete files)

This was spawned from bug 414622. To ease installation on Mobile devices we should be able to create a deb package (and serve it from somewhere, but that's another bug).
Blocks: 418852
-> brad.
Assignee: nobody → blassey
Status: NEW → ASSIGNED
Attached patch implements make deb for firefox (obsolete) — Splinter Review
these changes work for creating a .deb installer for firefox.  Still need to do something for xulrunner.

I'm attaching this wip patch to get comments.  Any are welcome.
OS: Mac OS X → Linux Maemo
Hardware: PC → Other
Attachment #312211 - Flags: review?(ted.mielczarek)
the Makefile.in code may belong in browser/installer and xulrunner/installer, but it works for both at the top level as is.  Again, looking for any input on this
Attachment #312211 - Attachment is obsolete: true
Attachment #312393 - Flags: review?(ted.mielczarek)
Attachment #312211 - Flags: review?(ted.mielczarek)
Attached patch update to previous patch (obsolete) — Splinter Review
The previous patch was missing the line to include package-name.mk
Attachment #312393 - Attachment is obsolete: true
Attachment #313100 - Flags: review?
Attachment #312393 - Flags: review?(ted.mielczarek)
Attachment #313100 - Flags: review? → review?(benjamin)
Attachment #313100 - Flags: review?(benjamin) → review?(ted.mielczarek)
Comment on attachment 313100 [details] [diff] [review]
update to previous patch


I'm not really reviewing the Debian bits, since I don't know much about deb packages. 

>+Description: The mozilla xulrunner

"The mozilla xulrunner" sounds a little weird. :) Also, I would capitalize Mozilla.

>Index: Makefile.in
>===================================================================

So, this is the wrong place for all this build goop. Ideally it would go in toolkit/mozapps/installer/packager.mk, but that makefile is a bit complicated, so I would accept putting it in a makefile next to that, like toolkit/mozapps/installer/debian.mk. Then just follow the example of |make package| and put your deb target in these makefiles:
http://lxr.mozilla.org/mozilla/source/browser/build.mk
http://lxr.mozilla.org/mozilla/source/xulrunner/build.mk

and then in the respective installer/Makefile.ins:
http://lxr.mozilla.org/mozilla/source/browser/installer/Makefile.in
http://lxr.mozilla.org/mozilla/source/xulrunner/installer/Makefile.in

You can probably structure it so that you just include toolkit/mozapps/installer/debian.mk (if that's what you choose to use) and put the |deb:| target in that file, then just have a few control variables that get set in the makefile that includes it.

Is this code only intended to work for Maemo, or is it a generic deb packaging? Either way, you should have some ifdefs in the Makefile so that trying to run this command on platforms where it won't work will either a) do nothing, or b) error with an explicit "not supported" message (see the xulrunner build.mk, the installer target).
Attachment #313100 - Flags: review?(ted.mielczarek) → review-
Brad: gentle ping... any update on this?
I'm planning on taking a look at it on the plane.
Attached patch xulrunner bits only (obsolete) — Splinter Review
This now works for desktop as well as maemo, it requires the debhelper package to be installed.  It also requires to the user to have super user privileges for two steps.  If the debhelper package isn't installed it fails at the end because dh_shlibdep doesn't exist.  We could check for that in configure, but it seems overkill.

I took the new short and long descriptions of xulrunner from mdc.

If this updated patch passes muster I'll update the browser bits in the same way.
Attachment #313100 - Attachment is obsolete: true
Attachment #315853 - Flags: review?
Attachment #315853 - Flags: review? → review?(ted.mielczarek)
(In reply to comment #9)
> It also requires to the user to have super user privileges
> for two steps.

Would fakeroot work?  If so, you may just want to hardcode fakeroot there.
fakeroot works fine, but adds another dependency.  Which is preferred? 
Keywords: mobile
taking Vlad's suggestion of just using fakeroot.  Also switched to using NS_HILDON instead of HILDON as suggested by bsmedberg in bug #362455
Attachment #315853 - Attachment is obsolete: true
Attachment #315963 - Flags: review?(ted.mielczarek)
Attachment #315853 - Flags: review?(ted.mielczarek)
Blocks: 430651
Comment on attachment 315963 [details] [diff] [review]
hardcodes fakeroot and uses NS_HILDON

Much better, just a few (much smaller) issues.

Index: xulrunner/installer/Makefile.in
===================================================================
+DESTDIR=$(DEPTH)/debian/$(MOZ_BUILD_APP)

You can't do that, that variable is used elsewhere. You'll have to use a different variable name for this path.

+deb: 

I'd like the contents of this target to be wrapped in ifeq($(OS_TARGET),Linux) or something like that, so that you can't run this target from a platform where it won't work.

+	cd $(DESTDIR)/usr/local; bunzip2 $(PKG_BASENAME).tar.bz2; tar -xvf $(PKG_BASENAME).tar; rm $(PKG_BASENAME).tar;
You should be able to do this in a single pipe, see for example:
http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/installer/packager.mk#423

+	mkdir -p $(DESTDIR)/etc/others-menu;

We don't use mkdir -p, we use nsinstall, see:
http://lxr.mozilla.org/mozilla/source/xulrunner/installer/Makefile.in#79

+	echo "$(MOZ_PKG_APPNAME) ($(MOZ_PKG_VERSION))  unstable; urgency=low" > $(DEPTH)/debian/changelog

Do you think you could make this be a pre-processed file instead of this series of echoes? See for example:
http://mxr.mozilla.org/mozilla/source/build/pgo/Makefile.in#90

I'm also not wild about the number of uses of `cp` here, but you're copying files to different directories with different names, so I guess there isn't much you can do about that. Still might be nice to do it via make rules in some way, but I won't hold you to that.
Attachment #315963 - Flags: review?(ted.mielczarek) → review-
Attached patch updated to address comments (obsolete) — Splinter Review
It would be nice to dynamically generate the change log based on cvs commit logs or something, but barring that there's really no reason it can be static (which eliminates the echos).

I don't suppose there's anyway to use nsinstall to rename a file as you copy it?  If so, we could get rid of that last cp.
Attachment #315963 - Attachment is obsolete: true
Attachment #318377 - Flags: review?(ted.mielczarek)
Comment on attachment 318377 [details] [diff] [review]
updated to address comments

Looks good. Just a nit:
+	rm -rf $(DEBDESTDIR)/usr/local/*;

You've got some extraneous trailing semicolons on a few lines.

Also
? xulrunner/installer/debian/changelog

Did you forget to CVS add that file?
Attachment #318377 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #15)
> (From update of attachment 318377 [details] [diff] [review])
> Looks good. Just a nit:
> +       rm -rf $(DEBDESTDIR)/usr/local/*;
no problem

> 
> You've got some extraneous trailing semicolons on a few lines.
> 
> Also
> ? xulrunner/installer/debian/changelog
> 
> Did you forget to CVS add that file?
> 

looks like I did.  Its just what the echos produced in the previous rev.

> It would be nice to dynamically generate the change log based on cvs commit
> logs or something, but barring that there's really no reason it can be static
> (which eliminates the echos).

Nah, I wouldn't do that -- that will just make the deb package bigger for no really useful reason.
Attachment #318377 - Flags: approval1.9?
This is XULRunner-only, I don't think you need approval.
XULRunner checkins do require approval by release-drivers, because the Linux distros are shipping FF+XR.
Ah. In that case: this patch adds a completely separate packaging target for xulrunner, and shouldn't affect any existing code.
Trying to lock down for release, and I think we could take this in a dot release.  a1.9-'ing, but if there's a dramatic reason why we should take this now, re-request approval.
Attachment #318377 - Flags: approval1.9? → approval1.9-
As discussed on the #build channel, having a static change log robs us of the version info.  I've modified to use configure to insert the app version and timestamp.

@dsicore what's the time frame for that?  This is blocking build from getting the mobile buildbots going in full swing.
Attachment #318377 - Attachment is obsolete: true
Attachment #318511 - Flags: review?(ted.mielczarek)
Damon: I really do think this is safe to take, especially since it's XulRunner-only.
And, what's the risk to the linux distros that ship FF+XR?
Essentially zero, as this doesn't change the existing packaging methods.
Based on comments 23 and 25, I'm reconsidering this.  
Comment on attachment 318377 [details] [diff] [review]
updated to address comments

a1.9+=damons

Ted is on the hook here if anything goes wrong.  ;-)
Attachment #318377 - Flags: approval1.9- → approval1.9+
Brad: I'm ok with you landing that approved patch for now (if you can still get it in, or after 1.9 ships, for 1.9.0.x), and getting the second patch as a follow-up.
Comment on attachment 318377 [details] [diff] [review]
updated to address comments

Minusing approval - let's get this in after 1.9
Attachment #318377 - Flags: approval1.9+ → approval1.9-
Comment on attachment 318511 [details] [diff] [review]
uses configure to generate debian files

Ok, you went a bit too far here. You only need to make the changelog preprocessed, the rest of the files can just be copied directly. I'm giving you r+ anyway, since your last patch was essentially right anyway, so I trust you to find the happy medium.
Attachment #318511 - Flags: review?(ted.mielczarek) → review+
Any update on this now that mozilla-central is open? 
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.