Closed Bug 639884 Opened 13 years ago Closed 13 years ago

Teach packaging code how to create RPM files from normal builds

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
All
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: jhford, Assigned: jhford)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

improvments as suggested by Mike
12.15 KB, patch
glandium
: review+
Details | Diff | Splinter Review 
The mozilla build system should know how to generate an RPM as a packaging type.
There are all sorts of details that would need to be worked out here. Was this filed at the request of the product team? I'm not convinced we actually want to be spending time doing this.
(In reply to comment #1)
> There are all sorts of details that would need to be worked out here. Was this
> filed at the request of the product team? I'm not convinced we actually want to
> be spending time doing this.

I have a patch already written that does this, just running some tests before I upload it.  The bug that this is blocking is specifically for creating a yum repository for nightly users.
This is a patch that adds a rpm as a packaging format.  Normally rpmbuild runs the entire build process (%prep/%build in spec).  The RPMs generated by this patch short circuit the normal rpmbuild process and essentially run make install inside of an already built objdir.  I chose this approach because it allows us to integrate with our existing build infrastructure easier as well as allowing us to use DEB as another packaging format without having to re-run the build portions of the packaging process.  I am also not aware of any way to make the tags (like name, version, release) in the spec file change depending on autoconf values from the build process. 

This patch deals has two subpackages, tests and devel.  The tests subpackage installs the entire package-tests contents into /usr/share/appname-version/tests/.  I haven't gotten these tests to work (file permissions to log files).  At some point, I plan on writing some scripts that are installed as part of the tests subpackage to allow users to run the test suites.  The devel subpackage installs the libraries, headers and interface descriptions that are from the SDK.  Both subpackages are generated only if the appropriate build system variables are defined (ENABLE_TESTS, INSTALL_SDK).  As well, both subpackages depend on the exact matching product, version and buildid version of the browser.  I am not sure if the devel subpackage should have this version restriction as someone may want to have more than one sdk installed.

I am not certain how I will end up implementing version and release rpm tags at this point, but I don't think that is material to this review.  A simple implementation that given 4.0b12pre with buildid 12345 a full rpm version of 0:4.0-b12pre.12345 is used.

One quick optimization that I could make is to wrap the zip -r9D line in the make package-tests target with something like:

ifndef MOZ_SKIP_TESTS_ARCHIVE
	zip -r9D <argv[2:]>
endif

I am not doing this in this patch to make sure that this patch is as NPOTB as possible.
Assignee: nobody → jhford
Status: NEW → ASSIGNED
Attachment #517792 - Flags: review?(mh+mozilla)
Hardware: x86_64 → All
Where is the app installed? I believe that any RPMs we create should completely avoid the standard install locations, so /opt/lib/firefox-<version> or such.

What set of distributions are we targeting? Just some fairly recent versions of Fedora?
I agree with Benjamin, here, the install path should be /opt/something. Probably /opt/mozilla/firefox-version.

> I am also not aware of any way to make the tags (like name, version, release)
> in the spec file change depending on autoconf values from the build process. 

Check the various .in files in the tree, and how they are preprocessed with Preprocessor.py.

> A simple implementation that given 4.0b12pre with buildid 12345 a full rpm
> version of 0:4.0-b12pre.12345 is used.

I don't know the exact version comparison rules for rpm, but you should make sure that this version is smaller than whatever version will be used for final, and greater than whatever version would be used for 3.6.

I'll take a look at the patch itself later this week.
(In reply to comment #4)
> Where is the app installed? I believe that any RPMs we create should completely
> avoid the standard install locations, so /opt/lib/firefox-<version> or such.
> 
> What set of distributions are we targeting? Just some fairly recent versions of
> Fedora?

In this patch, the install path is controlled by the autoconf variables for the prefix used in the rpm spec file
(In reply to comment #5)
> I agree with Benjamin, here, the install path should be /opt/something.
> Probably /opt/mozilla/firefox-version.

Yep, fine by me (comment 6)

> > I am also not aware of any way to make the tags (like name, version, release)
> > in the spec file change depending on autoconf values from the build process. 
> 
> Check the various .in files in the tree, and how they are preprocessed with
> Preprocessor.py.

Cool, I'll check that out.
 
> > A simple implementation that given 4.0b12pre with buildid 12345 a full rpm
> > version of 0:4.0-b12pre.12345 is used.
> 
> I don't know the exact version comparison rules for rpm, but you should make
> sure that this version is smaller than whatever version will be used for final,
> and greater than whatever version would be used for 3.6.

I looked at that yesterday (using rpmdev-vercmp).  Version numbers and distribution channels are something that I'd like to look at as part of the bug that is dependent on this.

> I'll take a look at the patch itself later this week.

Cool!  If there is anything I can do to help, please let me know.  We have a goal of getting a repository populated with nightly build rpms by the end of march.
I have uploaded a sample rpm that I generated on my local machine to http://people.mozilla.com/~jford/sample-rpm/

I also modified browser/confvars.sh to get the MOZ_APP_NAME change to work.
Comment on attachment 517792 [details] [diff] [review]
add rpm generation to the packaging scripts

Overall, this is not too bad, but I'd have a few suggestions:
- The desktop file shouldn't be specific to rpm. I think it should be made part of the normal files "make install" would install (you'd probably need to preprocess it, too).
- Instead of a lot of --define passed to rpmbuild, I'd just preprocess the spec file.
- I wouldn't make the spec file call the install rule, but live it to packager.mk.
- Like the desktop file, the icon files should be installed by make install, too.
Attachment #517792 - Flags: review?(mh+mozilla) → review-
> Comment on attachment 517792 [details] [diff] [review]
> add rpm generation to the packaging scripts
> 
> Overall, this is not too bad, but I'd have a few suggestions:
> - The desktop file shouldn't be specific to rpm. I think it should be made part
> of the normal files "make install" would install (you'd probably need to
> preprocess it, too).

I agree.  I can look at how hard this would be to do, but if it seems like too much scope creep for this bug, do you mind if I fix this as a followup? (same for the icon files)

> - Instead of a lot of --define passed to rpmbuild, I'd just preprocess the spec
> file.

From mxr, it looks like most invocations of Preprocessor.py pass in macros that are defined in the make variables $(DEFINES) and $(ACDEFINES).  With the exclusion of MOZ_APP_NAME, every value that I need to use isn't defined in either of these variables and would result in the same number of -D arguments to the preprocessor as I currently have --defines.  No matter what, I still have to define the internal RPM macros (%{_topdir}, etc) on the rpmbuild command line.  Given that, it seems that using one method of getting the data into the spec file instead of a two step process is more clear.

> - I wouldn't make the spec file call the install rule, but live it to
> packager.mk.

Unlike debian packaging, the make install target is usually called as part of the spec file.  Part of the reason for this is that the RPM_BUILD_ROOT directory is nuked at the beginning of the %install script.  As this is a common pattern in spec files, and is something that needs to be done regardless, I'd prefer to leave it as is.  The alternative would be to do |make install DESTDIR=destdir| in the make files then something like |cp -a destdir/* $RPM_BUILD_ROOT/| in the spec.

> - Like the desktop file, the icon files should be installed by make install,
> too.
(In reply to comment #10)
> I agree.  I can look at how hard this would be to do, but if it seems like too
> much scope creep for this bug, do you mind if I fix this as a followup? (same
> for the icon files)

jhford: have you had time to look at this yet?

glandium: any thoughts on jhford's other comments?
No comment to add. Sounds fair.
Just a thought, though: it's sad that rpmbuild doesn't allow to build a package off a set of metadata and a prepared directory. e.g. something as simple as "tar -c". If we'd ever build debian/ubuntu packages, that's all we'd need.
(In reply to comment #11)
> (In reply to comment #10)
> > I agree.  I can look at how hard this would be to do, but if it seems like too
> > much scope creep for this bug, do you mind if I fix this as a followup? (same
> > for the icon files)
> 
> jhford: have you had time to look at this yet?

I have looked into this and I feel that it is a follow up bug

(In reply to comment #12)
> No comment to add. Sounds fair.

Ok, I'll check for bit rot and prepare another patch.

(In reply to comment #13)
> Just a thought, though: it's sad that rpmbuild doesn't allow to build a package
> off a set of metadata and a prepared directory. e.g. something as simple as
> "tar -c". If we'd ever build debian/ubuntu packages, that's all we'd need.

I have looked into it and it is indeed very simple :D
Attached patch unbitrot the patch (obsolete) — — Splinter Review 
Updated to clear up bitrotting
Attachment #517792 - Attachment is obsolete: true
Comment on attachment 521601 [details] [diff] [review]
unbitrot the patch

Hey, so I updated the patch.  If this patch gets r+, I'll file the dep bugs to deal with .desktop file and the icons.
Attachment #521601 - Flags: review?(mh+mozilla)
Comment on attachment 521601 [details] [diff] [review]
unbitrot the patch

>+StartupWMClass=mozilla-bin
>+MimeType=text/html;text/xml;application/xhtml+xml;application/vnd.mozilla.xul+xml;text/mml;

Please add x-scheme-handler/http;x-scheme-handler/https

You may also want to add X-MultipleArgs=false

>+LINUX_PRETTY_NAME= \
>+	$(shell $(PYTHON) -c "import string ; print string.capwords('$(MOZ_APP_NAME)')")

Why not simply use MOZ_APP_DISPLAYNAME ?

>+  sed -e "s,^Name=.*,Name=Mozilla $(LINUX_PRETTY_NAME)," \
>+	  -e "s,^Exec=.*,Exec=$(MOZ_APP_NAME)," \
>+	  -e "s,^Icon=.*,Icon=$(MOZ_APP_NAME)," \
>+	  -e "s,^StartupWMClass=.*,StartupWMClass=$(MOZ_APP_NAME)-bin," \
>+	  < $(RPM_INCIDENTALS)/mozilla.desktop \
>+	  > $(RPMBUILD_SOURCEDIR)/$(MOZ_APP_NAME).desktop && \

That really ought to use Preprocessor.py instead.
Attachment #521601 - Flags: review?(mh+mozilla) → review-
>+  sed -e "s,^Name=.*,Name=Mozilla $(LINUX_PRETTY_NAME)," \

Also, prepending a hardcoded Mozilla is probably wrong, though I'm not sure what should be used instead.
Ideally we'd be using the branding information in $(DIST)/branding.
(In reply to comment #17)
> Please add x-scheme-handler/http;x-scheme-handler/https

Done
 
> You may also want to add X-MultipleArgs=false

Done, but I am not 100% sure what this parameter is for.

> >+LINUX_PRETTY_NAME= \
> >+	$(shell $(PYTHON) -c "import string ; print string.capwords('$(MOZ_APP_NAME)')")
> 
> Why not simply use MOZ_APP_DISPLAYNAME ?

Good question.  Now using MOZ_APP_DISPLAYNAME.

> >+  sed -e "s,^Name=.*,Name=Mozilla $(LINUX_PRETTY_NAME)," \
> >+	  -e "s,^Exec=.*,Exec=$(MOZ_APP_NAME)," \
> >+	  -e "s,^Icon=.*,Icon=$(MOZ_APP_NAME)," \
> >+	  -e "s,^StartupWMClass=.*,StartupWMClass=$(MOZ_APP_NAME)-bin," \
> >+	  < $(RPM_INCIDENTALS)/mozilla.desktop \
> >+	  > $(RPMBUILD_SOURCEDIR)/$(MOZ_APP_NAME).desktop && \
> 
> That really ought to use Preprocessor.py instead.

Agreed.  It is now :)


(In reply to comment #18)
> >+  sed -e "s,^Name=.*,Name=Mozilla $(LINUX_PRETTY_NAME)," \
> 
> Also, prepending a hardcoded Mozilla is probably wrong, though I'm not sure
> what should be used instead.

Lets leave it as MOZ_APP_DISPLAYNAME for now.  If we decide to go ahead with branding Firefox as Mozilla Firefox, we should probably make that change for all OS/Package formats.  Also, since I changed it to DISPLAYNAME, we could end up with "Mozilla Mozilla Firefox".

(In reply to comment #19)
> Ideally we'd be using the branding information in $(DIST)/branding.

The only information in there (that i see anyway) is the icon files.  As long as MOZ_BRANDING_DIRECTORY maps to $(DIST)/branding, we are good there.
Attachment #521601 - Attachment is obsolete: true
Attachment #521978 - Flags: review?(mh+mozilla)
There should be a branding.dtd and a branding.properties in there.  That's where the proper localized product name lives.
Oh, it ends up in dist/branding with a patch I have in my queue :-)
Hmm, neat.  Maybe I can take a look at that for a follow up when I add localization to the mix.
Comment on attachment 521978 [details] [diff] [review]
improvments as suggested by Mike

Looks good for a start. Please file follow-ups for icons and desktop file and CC me.
Attachment #521978 - Flags: review?(mh+mozilla) → review+
Blocks: 646518
Blocks: 646520
Comment on attachment 521978 [details] [diff] [review]
improvments as suggested by Mike

http://hg.mozilla.org/projects/build-system/rev/3a205e920ec0

I spoke to Kyle, and there is going to be a build-system merge most likely tonight
http://hg.mozilla.org/mozilla-central/rev/3a205e920ec0
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Why include text/mml? Also Firefox doesn't open up XUL files directly anymore so I don't think application/vnd.mozilla.xul+xml should be there either.
(In reply to comment #29)
> Why include text/mml? Also Firefox doesn't open up XUL files directly anymore
> so I don't think application/vnd.mozilla.xul+xml should be there either.

If these shouldn't be there, please file a bug to remove them.
Blocks: 649721
Component: Build Config → General
Product: Firefox → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: