Hi Andrea, sorry it took so long, your mail was buried in my inbox. :( Unfortunately dogd already applied some of your changes.
Am Mittwoch, den 07.10.2009, 23:46 +0200 schrieb Andrea Florio: > > > >> --- lxdm.spec.in 2009-09-23 01:45:11.000000000 +0200 > >> +++ new.lxdm.spec 2009-09-23 01:46:24.000000000 +0200 > >> @@ -1,11 +1,23 @@ > >> +# on SuSE based distros that's allow compilation > >> +# without root rights, on other distros that's just > >> +# a comment, you should keep it. > >> +# norootforbuild > > > > This tag is for the SUSE buildsys, so it should not be in the generic > > spec. BTW: Packages should *never* be built as root. Can you tell me one > > good reason to do so? > > > > i think you are wrog here, as i wrote on the comment, ONLY an (open)SUSE > box will take care of it, any other distro will consider it like a > regular comment on the spec file, so no demages, only advantages. Correct me if I'm wrong here: It's not OpenSUSE specific but only the OpenSUSE build service will use it. As I said previously: The spec distributed in the upstream tarball should be generic and not distro-specfic. Even if it's just a comment. Packages should never be built as root, so we don't need this anyway. > >> +# On SuSE that's not a valid RPM group, > >> +# that MAY change on different distros (not sure) > >> +# SuSE group is commented > >> +#Group: System/GUI/Others > >> + > >> Group: User Interface/X > > > > Here SUSE is the only Distro to not follow the rpm upstream and use > > different groups. "User Interface/X" is upstream, see > > http://rpm.org/gitweb?p=rpm.git;a=blob_plain;f=GROUPS > > > > > i agree here, but in the same way, my change has been wrote as a > comment, so again, no real changes on spec file. Rreally, if i would > like to have both we can easly workaround like that: > > %if 0%suse_version > Group: SUSE-GROUP > %else > Group: RPM-GROUP > %endif We already had this discussions before (when you suggested LXDE should use OBS) and I'm sorry to repeat myself: If we started with this, we will end up in a large bunch of %if ... %else clauses. What if distro X, Y and Z come up and demand the same modifications? > >> URL: http://lxde.sourceforge.net/ > >> -Source0: https://lxde.svn.sourceforge.net/svnroot/lxde/trunk/lxdm/ > >> +# the source should be an archive... > >> +#Source0: https://lxde.svn.sourceforge.net/svnroot/lxde/trunk/lxdm/ > >> +Source0: lxdm.tar.bz2 > > > > As long as it's only available from SVN we need to stick with that, but > > of course referencing the svn URL as source wont work. > > > > > so here we agree, the best way will be to have > > Source0: http://......./lx...@[email protected] > > where the url will be changed with the url of the upstream stable > released tarball. (better a tar.bz2 than a tar.gz to made src.rpm smaller) Agreed, except for your comment: Instead of "archive" we should write: "Source should be provided with full URL when there is a stable release." The source should be http://downloads.sourceforge.net/sourceforge/lxde/%{name}-%{version}.tar.gz then (works for all LXDE components and can be accessed with wget or alike) > >> -%configure --prefix=/usr > >> +# always export $RPM_OPT_FLAGS > >> + > >> +export CFLAGS="$RPM_OPT_FLAGS" > >> +export CXXFLAGS="$RPM_OPT_FLAGS" > > > > This should be done by rpm, so it's not necessary to do it in the spec. > > In fact, you could accidentially overwrite flags set by the %configure > > macro. > > > > that sound like a news for me, actually on what i know, rpm do not > automatically export RPM_OPT_FLAGS (rpmlint infact perform a check on that) > > in particular that needs to be checked package by package, any way, lxdm > now use only "-g -O2" that are inside RPM_OPT_FLAGS (to clarify that to > non-rpm-packagers: > > RPM_OPT_FLAGS: "-fmessage-length=0 -O2 -Wall -D_FORTIFY_SOURCE=2 > -fstack-protector -funwind-tables -fasynchronous-unwind-tables -g" ) Run "rpm --eval %configure" to see what the %configure macro does. The first thing it does is to export the proper compiler flags. > >> +# %configure macro already pass "--prefix=%_prefix" > >> +# where prefix is /usr, if the user needs a different > >> +# _prefix that should be exported on spec file like: > >> +# %define _prefix /new/prefix > >> +# %configure --prefix=/usr > >> + > >> +%configure > >> + > > > i really believe here on that change, also because using > "%define _prefix /new/prefix" will allow to only change on line... > > example: > > we know that (where default prefix is /usr): > > %_datadir = %_prefix/share --> /usr/share > %_bindir = %_prefix/bin --> /usr/bin > > if we use "%define _prefix /usr/local": > > > %_datadir = %_prefix/share --> /usr/local/share > %_bindir = %_prefix/bin --> /usr/local/bin > > in particular we edit only one line. > Passing --prefix=/usr/local instead we'll need to change ALL the %files > section of the spec file. First of all I didn't criticize that change, but as you may already have guessed I don't really like it ether: 1. Too much comments IMO. This is a spec but not a book. ;) 2. We should trust %configure. 3. Using %define is problematic, you should prefer %global where possible. Remember your distro specific hacks "%if 0%suse_version ..." Imagine we used configure inside of such a conditional. %define is only valid till the end of that statement (%else or %endif), so it won't work for the files section and the build would fail. > >> +# a make macro should be used here > >> +# even a "job based" make if possible > >> +# %__make %{?jobs:-j%{jobs}} > >> + > >> +%__make > > > > I agree we should use macro if possible, but for a generic spec file we > > never know how they are defined on different systems. So a simple "make" > > is less specific and more secure. > > > > > agree here, in particular any way, my change is still a comment, so no > real changes on spec file /me still prefers a simple and straight forward spec > > Why use %makeinstall??? %makeinstall has a couple of downsides such as > > overwriting variables set by %configure, breaking libtool archives and > > so on. Please don't use it. > > > > > again that's a news for me, > > %makeinstall is a short form for the following (from default > /usr/lib/rpm/macros) : > > %makeinstall \ > make \\\ > prefix=%{?buildroot:%{buildroot}}%{_prefix} \\\ > exec_prefix=%{?buildroot:%{buildroot}}%{_exec_prefix} \\\ > bindir=%{?buildroot:%{buildroot}}%{_bindir} \\\ > sbindir=%{?buildroot:%{buildroot}}%{_sbindir} \\\ > sysconfdir=%{?buildroot:%{buildroot}}%{_sysconfdir} \\\ > datadir=%{?buildroot:%{buildroot}}%{_datadir} \\\ > includedir=%{?buildroot:%{buildroot}}%{_includedir} \\\ > libdir=%{?buildroot:%{buildroot}}%{_libdir} \\\ > libexecdir=%{?buildroot:%{buildroot}}%{_libexecdir} \\\ > localstatedir=%{?buildroot:%{buildroot}}%{_localstatedir} \\\ > sharedstatedir=%{?buildroot:%{buildroot}}%{_sharedstatedir} \\\ > mandir=%{?buildroot:%{buildroot}}%{_mandir} \\\ > infodir=%{?buildroot:%{buildroot}}%{_infodir} \\\ > install > > on other words > > %makeinstall = make DESTDIR=${RPM_BUILD_ROOT} install > > if i'm wrong here, please let me know. What is the sense of letting autotools set all the dirs through %configure if you set them afterwards with %makeinstall. Trust the autotools to do the right thing or don't use them. For more on this topic please read http://fedoraproject.org/wiki/Packaging/Guidelines#Why_the_.25makeinstall_macro_should_not_be_used > >> +# That's only required if translations are available > >> +# (.mo/.po/.gmo/ecc) files available, otherwise > >> +# that's not needed > >> > >> -%find_lang %{name} > >> +#%find_lang %{name} > >> > >> %clean > >> rm -rf $RPM_BUILD_ROOT > >> @@ -37,10 +77,13 @@ > >> > >> %postun > >> > >> -%files -f %{name}.lang > >> +# exactly as "%find_lang" macro > >> +#%files -f %{name}.lang > >> +%files > > > > Right, currently there are no locales and running find_lang will break > > the build. > > > > so we agree here. Sure. :) IMO there is no need for including all these commented things in the spec. It LXDM get's translated, we would need to ship a new package anyway - with an updated spec included. > > The last line should be > > %config(noreplace) %{_sysconfdir}/lxdm/Xsession > > to make sure custom configs don't get overwritten by package updates. > > > > i do not agree here. Xsession is not a config file, like lxdm.conf. > > in particular it defines new (and actually iproved upate by update) > setting to allow to succesfull start an xsession if default xinitrc > script is not found. As long as the file was not modified, it will get replaced even if it is %config(noreplace). And if it was modified, it was most likely modified on purpose. If you really think it should be overwritten, use %config without noreplace, but the %config tag is important for rpm's --verify. > anyway, following rpm direction, only NON > executable config files should be added as %config. Who says that? rpmlint? Please don't take rpmlint to seriously. It's just a helper but also has lots of false positives. It only checks if something marked as %config is executable but not if it's a script or a program. For example, take a look at the xorg-x11 package in your distribution with "rpm -qc xorg-x11". As you see these are executable config files, even /etc/x11/xdm/Xsession. > %config %{_sysconfdir}/lxdm/lxdm.conf > > should be > > %config(noreplace) %{_sysconfdir}/lxdm/lxdm.conf Agreed. > i love to exachange our toughts so we can both became better packagers. Ok, I if you have any questions, don't hesitate to ask. Regards, Christoph ------------------------------------------------------------------------------ Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now! http://p.sf.net/sfu/devconference _______________________________________________ Lxde-list mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/lxde-list
