Bug 245708

Note that this is related to
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197867

stgt mentioned in that bugzilla is what I am packaging here. This is the project
that is replacing iet/iscsitarget, because the upstream kernel would not accept
iet/iscsitarget.

Oh yeah, one question on the review. I ran rpmlint and it did not report any errors.

On the iscsi-initiator-utils review, it was requested that I should hook that
into rc.sysinit. For scsi-target-utils, I was not sure if I should do the same.
Is there any guidelines about what should or should not be hooked into
rc.sysinit? Does this qualify? I did a init script, but let me know if you want
me to hook this into rc.sysinit and I will send a patch.

(In reply to comment #2)
> Oh yeah, one question on the review. I ran rpmlint and it did not report any
errors.
> 

One correction. I got this:
W: scsi-target-utils strange-permission tgtd.init 0755
But the packaging guidelines said I should be using 0755, so I am not sure what
I am messing up and if I am supposed to be using rc.sysinit instead of the init
script.

The initscript seems reasonable. Is tgtd configured only by tgtadm?

(In reply to comment #4)
> The initscript seems reasonable. Is tgtd configured only by tgtadm?
> 

At this point yes. We are waiting on upstream and other distros to see how
people want and like to configure it. For example for iscsi we are working on
tool which reads IET (iscsi-target project from
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=197867) config files and
will set things up using that info, in the hopes that this will ease the
transition from IET to tgt. Those tools are not done and we are still trying to
figure out what we want to do upstream more precisely.

MUST:
 - Package meets naming and packaging guidelines - ***

Upstream package name appears to be 'tgt'. While scsi-target-utils is certainly
more informative, is there a particular reason for the disconnect? I also notice
the upstream project name is 'stgt' while upstream source is 'tgt'.

 - Spec file matches base package name. - OK
 - Spec has consistant macro usage. - OK
 - Meets Packaging Guidelines. - OK
 - License - OK
 - License field in spec matches - OK
 - License file included in package - ***

A copy of GPL2 is not included. Doesn't appear to be included upstream, either.

 - Spec in American English - OK
 - Spec is legible. - OK
 - Sources match upstream md5sum: - OK

 - Package needs ExcludeArch - No
 - BuildRequires correct - OK
 - Spec handles locales/find_lang - N/A
 - Package is relocatable and has a reason to be. - N/A
 - Package has %defattr and permissions on files is good. - N/A
 - Package has a correct %clean section. - OK
 - Package has correct buildroot
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) - OK
 - Package is code or permissible content. - OK
 - Doc subpackage needed/used. - N/A
 - Packages %doc files don't affect runtime. - N/A

 - Headers/static libs in -devel subpackage. - N/A
 - Spec has needed ldconfig in post and postun - N/A
 - .pc files in -devel subpackage/requires pkgconfig - N/A
 - .so files in -devel subpackage. - N/A
 - -devel package Requires: %{name} = %{version}-%{release} - N/A
 - .la files are removed. - N/A

 - Package is a GUI app and has a .desktop file - N/A

 - Package compiles and builds on at least one arch. - OK
 - Package has no duplicate files in %files. - OK
 - Package doesn't own any directories other packages own. - OK
 - Package owns all the directories it creates. - OK
 - No rpmlint output. ***

Source rpm:

W: scsi-target-utils strange-permission tgtd.init 0755 - can be ignored

Binary rpm:
E: scsi-target-utils init-script-without-chkconfig-postin /etc/rc.d/init.d/tgtd
E: scsi-target-utils init-script-without-chkconfig-preun /etc/rc.d/init.d/tgtd

See below.

E: scsi-target-utils incoherent-subsys /etc/rc.d/init.d/tgtd tgtd]

Typo?

W: scsi-target-utils no-reload-entry /etc/rc.d/init.d/tgtd

See below.

W: scsi-target-utils incoherent-init-script-name tgtd

Not sure what to make of this warning.

 - final provides and requires are sane:

See below about chkconfig. Otherwise reasonable.

SHOULD Items:

 - Should build in mock. - OK
 - Should build on all supported archs - only tested x86_64
 - Should function as described. - did not test
 - Should have sane scriptlets. - ***

Missing the proper scriptlets for adding/removing scripts. See:

 http://fedoraproject.org/wiki/Packaging/ScriptletSnippets

See the section on chkconfig Requires: there as well; as it is now, they're wrong.

 - Should have subpackages require base package with fully versioned depend. - N/A
 - Should have dist tag - OK
 - Should package latest version - OK

Other comments:

1. Versioning - this uses 0.1 for the package version. Upstream versioning is
done by date; the package should reflect that.

2. The init script - various issues

1) doesn't actually do anything sane if it fails to start
2) doesn't have a reload entry (if it's possible)
3) doesn't use the proper LSB return codes

3. Xen support - is not built. Should it be?

Thanks for the review.

(In reply to comment #6)
> MUST:
>  - Package meets naming and packaging guidelines - ***
> 
> Upstream package name appears to be 'tgt'. While scsi-target-utils is certainly
> more informative, is there a particular reason for the disconnect? I also notice
> the upstream project name is 'stgt' while upstream source is 'tgt'.
> 

Upstream, we renamed the project a couple times. Originally it was stgt for scsi
target project, then we decided to support all block targets so we dropped the
's'. And we are not good at naming so people have been recomending new names
still :)

The scsi-target-utils naming for fedora was used to match the existnig naming
style we have. For Fedora we have iscsi-initiator-utils which has tools and
daemon for iscsi clients (initiator in scsi terms means cleint). The
scsi-target-utils then has tools and daemon for scsi targets.

Does that make sense or do you think it is confusing?

I will fix other issues you found and resend. Thanks.

(In reply to comment #6)
> 1) doesn't actually do anything sane if it fails to start

For this one, it just ends up printing failed instead of ok, is that ok?

Something like this:

        daemon tgtd
        RETVAL=$?
        [ $RETVAL -eq "0" ] && touch /var/lock/subsys/tgtd
        echo
        return $RETVAL

That looks better - I think just the spacing was odd when I first ran it.

SPEC
http://people.redhat.com/mchristi/target/FC7/rpm/scsi-target-utils/scsi-target-utils-0.0-0.20070620snap/scsi-target-utils.spec
SRC RPM
http://people.redhat.com/mchristi/target/FC7/rpm/scsi-target-utils/scsi-target-utils-0.0-0.20070620snap/scsi-target-utils-0.0-0.20070620snap.fc7.src.rpm

Here is update spec and rpm with pretty much all the comments handled. Here are
the ones I was not sure of or did not apply or I am working on upstream:

1. GPL file. I guess this one is not required since upstream does not have a
licensne file (all the source files have proper headers though), but I am
working on getting a license file added to upstream package.

2. W: scsi-target-utils incoherent-init-script-name tgtd
I could not figure this one out.

3. doesn't have a reload entry (if it's possible).
This does not apply right now. We do not have any values which would be reloaded
at this time.

4. Xen Support not being built.
This pacakge should be usable with Xen in domU or dom0. However, I am not sure
how to do get it build in. Ok, I am not even sure what that means :) Is this
asking me to set some sort of Arch flag in the spec file so it gets built into
some sort of xen arch? This is an all userspace code package, so I was thinking
it would not need anything special. You should be able to install the rpm into
any dom and off you go.

5. Naming. The iscsi-initiator-utils naming for iscsi tools and daemon was done
before I started, so like I said in comment #7 I kept the same style for the
scsi target tools not knowing if this was the preferred scheme or if the person
that named iscsi-initiator-utils was on drugs :) If my guess was wrong and you
want me to change the package name to just be "tgt" like upstream, let me know
and I will reroll.

Thanks.

For xen, there's a XEN=1 target in the makefile that builds.... something. Not
sure what, as it doesn't appear to build right at the moment. Wasn't sure if
this was intentional or an accident.

For %post, the chkconfig --add should not be wrapped in the 'if [ "$1"...'
statement. 

It should probably have a %postun that does condrestart, if the service supports it.

Re: name - leave it as scsi-target-utils; that's at least legible as to what the
package does (unlike 'tgt'). If you want to bribe upstream into renaming, all
the better. 

(In reply to comment #11)
> For xen, there's a XEN=1 target in the makefile that builds.... something. Not
> sure what, as it doesn't appear to build right at the moment. Wasn't sure if
> this was intentional or an accident.

Ah, I see my fault. Yeah I did not built that on purpose. The Xen code in tgt is
for a Xen SCSI target. It is still in development. It is not ready for fedora.

> 
> For %post, the chkconfig --add should not be wrapped in the 'if [ "$1"...'
> statement. 
> 
> It should probably have a %postun that does condrestart, if the service
supports it.
> 

Ok will fix.

thanks.

SPEC
http://people.redhat.com/mchristi/target/FC7/rpm/scsi-target-utils/take2/scsi-target-utils.spec

SRC RPM
http://people.redhat.com/mchristi/target/FC7/rpm/scsi-target-utils/take2/scsi-target-utils-0.0-0.20070620snap.fc7.src.rpm

Here is the updated src rpm and spec with your comments handled.

tgtd.init needs an 'echo' after the daemon call.

My only question is with the versioning - if this will use 'normal' versioning
and the date is just a prerelease snapshot, it's fine. If it's intended to use
the date as the *actual* verison going forward, it should be in %{version}.

(In reply to comment #15)
> tgtd.init needs an 'echo' after the daemon call.
> 

Fixed. I also found another missing echo.

> My only question is with the versioning - if this will use 'normal' versioning
> and the date is just a prerelease snapshot, it's fine.

Yeah, the date versioning is temporary. Those data based tarballs are for users
that do not want to grab what is in git and so distros can download something
right now. When we do a real first release, we will use a more normal X.Y.Z naming.

SPEC
http://people.redhat.com/mchristi/target/FC7/rpm/scsi-target-utils/take3/scsi-target-utils.spec

SRC RPM
http://people.redhat.com/mchristi/target/FC7/rpm/scsi-target-utils/take3/scsi-target-utils-0.0-0.20070620snap.fc7.src.rpm


Here are links to the updated rpms and spec (spec is the same, just a udpdated
tgtd.init in the src rpm).

Works for me. APPROVED.

New Package CVS Request
=======================
Package Name: scsi-target-utils
Short Description: SCSI Target Daemon
Owners: mchristi
Branches: F-7
InitialCC:

cvs done.

What is going on here? It's over two months since the package was approved and
I can't find anything in CVS.

There are some bugs too:

 o license policy has changed since review:
     http://fedoraproject.org/wiki/Licensing
   License should be changed to GPLv2

 o build not honor compiler flags:
     http://tinyurl.com/2n3qqh

 o should build latest release:
   new release from 2007-08-03 available

I have created an update package fixing these bugs, it's here:

SPEC: http://terjeros.fedorapeople.org/scsi-target-utils/scsi-target-utils.spec
SRPM:
http://terjeros.fedorapeople.org/scsi-target-utils/scsi-target-utils-0.0-0.20070803snap.fc7.src.rpm


Do you still want to maintain this package for Fedora?

My fault. I must have just done the RHEL sources and forgot to do FC for some
reason.

If you want to maintain it or help, that is fine with me.

Ok, I can take it.

I did a updated package with some style changes, can notting have a quick look
and (hopefully) ack?

SPEC: http://terjeros.fedorapeople.org/scsi-target-utils/scsi-target-utils.spec
SRPM:
http://terjeros.fedorapeople.org/scsi-target-utils/scsi-target-utils-0.0-1.20070803snap.fc7.src.rpm

You then have to set fedora-cvs to ? and say something like this:

Package Change Request
======================
Package Name: scsi-target-utils 
Change Owner To: terjeros

Thanks for the nice work and quick reply!

For the package change request, are you supposed to open a new bugzilla? I am
just going by this and am not 100% sure.
http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure#other

The existing one is fine. You should also be able to use pkgdb
https://admin.fedoraproject.org/pkgdb to give cvs access to other maintainers if
you prefer to keep ownership of the package. 

terjeros.no what do you prefer?

I normally do upstream development (I am co maintainer of upstream package) and
am getting used to doing fedora. Do you want to be fedora maintainer and give me
access rights or let me be fedora maintainer and give you access rights?

It does not matter to me.

> (I am co maintainer of upstream package)
> and am getting used to doing fedora.

That's great, we always need more people in Fedora.

> Do you want to be fedora maintainer and give me
> access rights or let me be fedora maintainer and give you access rights?

Give me access rights and we are fine.

Will you do the initial import or should I do it?

(In reply to comment #26)
> > (I am co maintainer of upstream package)
> > and am getting used to doing fedora.
> 
> That's great, we always need more people in Fedora.
> 
> > Do you want to be fedora maintainer and give me
> > access rights or let me be fedora maintainer and give you access rights?
> 
> Give me access rights and we are fine.

Ok, I went to https://admin.fedoraproject.org/pkgdb/packages/name/scsi-target-utils

and checked the "group members can commit" and hit the "add myself to package"
button and approved myself.

If you go there and hit the "add myself to package" and request cvs and acls and
the bugzilla and commit ones or whatever ones you want, I will approve them, or
approve them yourself if you can.



> 
> Will you do the initial import or should I do it?
> 

Since you have the updated src rpm which fixes stuff in mine go ahead.

> If you go there and hit the "add myself to package" and request cvs and acls 
> the bugzilla and commit ones or whatever ones you want, I will approve them,
> or approve them yourself if you can.

Seems like you have to approve.

> Since you have the updated src rpm which fixes stuff in mine go ahead.

Will do.

The build fails on ppc in mock:

 http://koji.fedoraproject.org/koji/getfile?taskID=191584&name=build.log

Any ideas?

OK, it don't build in rawhide at all (not even i386), however it build just fine
in F-7.

Something in toolchain (gcc, glibc or glibc-headers etc) in rawhide 
must have broken tgt. It might be glibc-headers, see 

  https://bugzilla.redhat.com/show_bug.cgi?id=189696

Will have to wait to see what drepper says.

Built for rawhide and F-7. Pushed to F-7 testing by bodhi.