Bug 218408

I will review this after I go back from shopping

Well, before I go out for shopping:

I tried to rebuild in mockbuild and rpmlint complains about:
---------------------------------------
E: xl2tpd tag-not-utf8 %changelog
E: xl2tpd obsolete-not-provided l2tpd
E: xl2tpd non-readable /etc/xl2tpd/l2tp-secrets 0600
E: xl2tpd non-readable /etc/ppp/chap-secrets.sample 0600
W: xl2tpd dangerous-command-in-%post mv
E: xl2tpd no-chkconfig-line /etc/rc.d/init.d/xl2tpd
E: xl2tpd tag-not-utf8 %changelog
E: xl2tpd non-utf8-spec-file xl2tpd.spec
W: xl2tpd unversioned-explicit-obsoletes l2tpd
W: xl2tpd mixed-use-of-spaces-and-tabs (spaces: line 65, tab: line 52)
E: xl2tpd-debuginfo tag-not-utf8 %changelog
------------------------------------------------------------
Please fix these or explain the reason when these rpmlint message
can be ignored.

Also, please add %?dist tag (I have not yet checked this package,
I only rebuilt this)

* Tue Dec  5 2006 Paul Wouters <paul> 1.1.06-2
- Changed Mr. Karlsen's name to not be a utf8 problem
- Fixed Obosoletes/Provides to be more specific wrt l2tpd.

Spec URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd.spec
SRPM URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd-1.1.06-2.src.rpm

two comments of me got lost, and i think two of you :(

I aded the dist tag. fixes the spaces, and the obsolete/provides issue in the
above srpm.

Well, again full review of this package.

A. From http://fedoraproject.org/wiki/Packaging/Guidelines :
* rpmlint:
- rpmlint output is not silent.
-----------------------------------------------------
1. E: xl2tpd non-readable /etc/xl2tpd/l2tp-secrets 0600
2. E: xl2tpd non-readable /etc/ppp/chap-secrets.sample 0600
3. W: xl2tpd dangerous-command-in-%post mv
4. E: xl2tpd no-chkconfig-line /etc/rc.d/init.d/xl2tpd
-----------------------------------------------------
1. 2. : I don't know well about this package, however,
        please check if the permisson of these files are
        correct.
3. : I think that this warnings may ignorable, however:
     * By the way which is done first, doing %post scriptlet
       of xl2tpd, or obsoleting (i.e. deleting) xl2tpd?
       I vaguely remember several case (by yum) that
       obsoleting a package is done _before_ installing
       a substitute rpm.

       I think moving or copying should be done by 
       %triggerun
4. : This error can be suppressed by replacing tab with space
     in '# chkconfig:    - 80 30' line.

* Timestamps
- Use 'install -p' to keep timestamps.
  And I recommend that Makefile should be also fixed to
  use 'install -p'.

B. http://fedoraproject.org/wiki/Packaging/ScriptletSnippets :
* Initscripts Conventions
  - Please add the appropriate requirement in
    Requires(post), etc.

C. From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines :
   (Okay).

spec URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd.spec
SRPM URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd-1.1.06-3.src.rpm

* Tue Dec  5 2006 Paul Wouters <paul> 1.1.06-3
- Added Requires(post) / Requires(preun)
- changed init file to create /var/run/xl2tpd, fixed a tab/space
- changed control file to be within /var/run/xl2tpd/

1/2 is correct. The secrets should only be readable by root
3 I've tested migration and the order is correct. the orignally installed
  xl2tpd configs are kept as rmpsave files, the ones from /etc/l2tpd/ are
  migrated into /etc/xl2tpd/, and then l2tpd gets uninstalled, and creates
  its own rpmsave files if needed.
4 done
Timestamps: I am not sure why we don't want to show the timestamps of the
package build?
B: done

Before checking 1.1.06-3:

(In reply to comment #6)
> Timestamps: I am not sure why we don't want to show the timestamps of the
> package build?

I always strongly recommend to keep timestamps as:
- They show if vendor (like you) have added some modifications against
  the original sources.
- They show when the files are actually created. 

These are especially important when binary rpms contain a lot of
header files, image files, documentations, etc....
Setting the timestamps of these files to build time, not the time
when the contents of the files are actually changed confuses people
when they want to check how those files are changed.

Well,

* Please keep timestamps, as I said in comment #7 and the section
  'timestamps' in http://fedoraproject.org/wiki/Packaging/Guidelines
  Please use 'install -p'. Timestamps should be changed when the
  contents of the files are actually changed, this should be especially
  for text files.

  (In reply to comment #5)
* I still get the rpmlint complaint:
------------------------------------------
E: xl2tpd no-chkconfig-line /etc/rc.d/init.d/xl2tpd
------------------------------------------
> 4. : This error can be suppressed by replacing tab with space
>      in '# chkconfig:    - 80 30' line.
Well, what I wanted to say by this is: "please use space in 
'#chkconfig: - 80 30' line, not tab.

> B. http://fedoraproject.org/wiki/Packaging/ScriptletSnippets :
> * Initscripts Conventions
>   - Please add the appropriate requirement in
>     Requires(post), etc.
Well, requirement for /sbin/service is still missing

(In reply to comment #6)
> 3 I've tested migration and the order is correct. the orignally installed
>   xl2tpd configs are kept as rmpsave files, the ones from /etc/l2tpd/ are
>   migrated into /etc/xl2tpd/, and then l2tpd gets uninstalled, and creates
>   its own rpmsave files if needed.

I still have a concern about the case in which some people may
customize original l2tpd rpm and reinstall it with release number
incremented by themself.
In this case, l2tpd is not uninstalled, which is correct that 
customizing by themself means that they don't want to have l2tpd
automatically removed by xl2tpd. However, current %post scripts
anyway 'moves' files in l2tpd, then l2tpd is installed with some
files missing.

And... I don't know about l2tpd, however, if removing l2tpd is
done after installing x12tpd, then there arises a moment anyway 
when l2tpd is installed while some files in l2tpd is missing.
In the case uninstalling l2tpd exits with non-zeron status and
it won't be completed.

This state is critical when these files are 'required' for 
uninstalling l2tpd. Even if not, removing files in l2tpd should
be done when l2tpd is exactly about being removed.

spec URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd.spec
SRPM URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd-1.1.06-4.src.rpm

- I still prefer to see the build timestamps compared to the original tar ball
  timestampts. But I added -p, beacause I just want to get over this.
- the space vs tab issue was still there because that error is in the original
  tarball. I dont want to release the usptream version just for this, so I
  added a temp sed line to the install process.
  writing a patch to just cover a week before that release seems a bit overkill.
- Added /sbin/service requirement.
- as for the "mv". Please give me another method to distinguish between an
upgrade from l2tpd->xl2tpd and one from xl2tpd->xl2tpd. AFAIK, I cannot tell in
any scriplets. If I use "mv", the migration will not happen more then once. If I
use "cp", it will be possible to happen more then once. Alternatively, if you
are that worried I can leave out the entire migration and let users do it
manually, but then it will surely need human intervention after installing the
upgrade.

AS for your last notes, as I told before, there are NO errors when doing an
l2tpd -> xl2tpd upgrade. l2tpd uninstalls fine. xl2tpd installs fine. Please
test if you do not believe me.

What's left now is:
[root@bofh devel]# rpmlint /usr/src/redhat/RPMS/x86_64/xl2tpd-1.1.06-4.x86_64.rpm
E: xl2tpd non-readable /etc/xl2tpd/l2tp-secrets 0600
E: xl2tpd non-readable /etc/ppp/chap-secrets.sample 0600
W: xl2tpd dangerous-command-in-%post mv
[root@bofh devel]# rpmlint /usr/src/redhat/SRPMS/xl2tpd-1.1.06-4.src.rpm 

the secrets are supposed to be root-only readable.
the "mv" is discussed above.

Created attachment 143068 [details]
Diff to xl2tpd.spec

(In reply to comment #9)
> - the space vs tab issue was still there because that error is in the
original
>   tarball. I dont want to release the usptream version just for this, so I
>   added a temp sed line to the install process.
For this purpose, applying the patch attached will be more
smart.

Created attachment 143071 [details]
l2tpd -> xl2tpd log

> 
> What's left now is:
> [root@bofh devel]# rpmlint
/usr/src/redhat/RPMS/x86_64/xl2tpd-1.1.06-4.x86_64.rpm
> E: xl2tpd non-readable /etc/xl2tpd/l2tp-secrets 0600
> E: xl2tpd non-readable /etc/ppp/chap-secrets.sample 0600
> W: xl2tpd dangerous-command-in-%post mv
> [root@bofh devel]# rpmlint /usr/src/redhat/SRPMS/xl2tpd-1.1.06-4.src.rpm 
> 
> the secrets are supposed to be root-only readable.
> the "mv" is discussed above.

I checked this for to make sure that your script works,
and it is okay (at least it seems okay for me).

Please check if the log attached is what you expect.

Now I approve this package

-------------------------------------------------------
  This package (xl2tpd) is APPROVED by me.
-------------------------------------------------------

Please consider the patch for dealing with init file tab issue
attached on comment #9 .

looks good to me.

I used your sed command

spec URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd.spec
SRPM URL: ftp://ftp.xelerance.com/xl2tpd/xl2tpd-1.1.06-5.src.rpm

Please change the status to CLOSED NEXTRELEASE when
rebuilding for FE-devel is done.

Package Change Request
======================
Package Name: xl2tpd
New Branches: epel7
Owners: pwouters

Git done (by process-git-requests).