Bug 226387

Simo,
Is it a good idea to start review now (using data from CVS), or maybe wait for
some more changes finished?

We are close, I should have something v. close to final this afternoon (EST Time)

Remarks and nitpicks
(against the current rawhide's 3.0.24-8):

- Whether the first line's "auth" macro is actually useful? Since "pam" includes
/etc/pam.d/system-auth (since at least FC1 time), just a requirement of "pam"
should be enough.
- The Summary tag values should not end in a period.
- Either drop "Epoch: 0" (seems unneeded?) or use it for changelog entries too
(i.e. "0:3.0.24-8")
- rpmlint prefers short "GPL" form for "License" field.
- Source0 should use new url (us4.samba.org), previous one is nonexistent now.
- "BuildRoot" must contain at least %{name}-%{version}-%{release}
- "BuildRequires" can be minimized. "libtool" already requires "autoconf", 
"openssl-devel" already requires "krb5-devel", and "cups-devel" already requires
"gnutls-devel". "fileutils" is obsoleted by "coreutils", which is in "BR
exception list" (since included in minimal build environment). 
- Either "samba-swat" must require "samba-doc", or move "docs/htmldocs" to
"samba-swat" sub-package (else swat will contain broken links to help/docs etc.)
- I don't know a package who removes its /var/log stuff on uninstall... :| . And
please, do not remove my /var/lib/samba/netlogon stuff too (<sarcasm>).
- use init scripts throw /sbin/service only (add Requires(post): /sbin/service etc.)
- ":" seems to be more robast/ancient rather than "true" (/bin/true) :)
- "libsmbslient" subpackage must do "/sbin/ldconfig" too.
- Explicit requirements of "pam" and "initscripts" seems to be unneeded (don't
sure about "logrotate")
- "libsmbslient-devel" looks like "one file package". Maybe merge it with
"libsmbclient", and add "Provides: libsmbclient-devel" for "libsmbclient" ?
- Since static libs considered deprecated now, either drop
"libsmbslient-devel-static", or write some words/links about necessity of static
libs (write as a comment into .spec file).
- Get rid of "macros in %changelog" (reported by rpmlint).
- Non-UTF8 symbols in %changelog (at 28 Aug 2002 and perhaps further to EOF).
- Init scripts must not be marked as %config files.
- Explicit "/etc" should be replaced to "%{_sysconfdir}".

Anyway, run "rpmlint -v package_file" for both src and binary rpms, and consider
the output (certainly some rpmlint points can be ignored).

Created attachment 151207 [details]
Some of proposed changes for .spec file

Thanks for the feedback, it is very valuable.
I will clean up as much as possible the spec file, and thanks for the patch,
makes things easier.

Some counter-remarks :-)

- We have GPL, LGPL and something like MIT/X11 in the mix, not everything is
GPL, is there a way to represent this? GPL v2 or later woule be inaccurate.
- Why did you remove the (pre) for samba-common Requires? Irt was there for a
specific purpose connected to upgrades.
- as far as I know we don't use libtool, I will check if it is really required.
- I was required to split out libsmbcliente-devel as that's how the build system
recognize the fact we need a multilib package for 64bit archs, I was also told
that, to package .a files I must create a separate -static package. What should
I do? Just drop the -static and never ship the libsmbclient.a library ?


P.S.: it would be much easier to parse the patch if you avoid cosmetic changes,
or if you provide 2 separate patches, one for cosmetic changes and the other for
the real ones.

> I was also told that, to package .a files I must create a separate 
> -static package.

It's just that static libs are (strongly) discouraged, so either omit it, or 
if you choose to include it, provide justification for doing so.

> not everything is GPL, is there a way to represent this?
AFAIK, things like "GPL, MIT" are used (I saw it in some Extras packages)

> Why did you remove the (pre) for samba-common Requires?
I had removed (pre) for "samba" requires, since "samba" package has no any
"%pre" section...

> libtool
If not used, then return back "Requires: autoconf" :)

> Just drop the -static and never ship the libsmbclient.a library ?
Yes, exactly.

> cosmetic changes
OK :)
But cosmetic changes seem to be needed, at least for readability.

I can check in CVS too, just report me when some changes will be made.

the Required(pre) is used to make sure samba-common pre is run before everything
else. The problem is that the migration script MUST be run before the smbd
binary (in samba) is restarted so that it can be safely stopped by samba-common
pre script and the migration properly done. COnvluted and complex I know, but so
far the only solution to make it right that I could come up with.

I will do the checkins I like to make sure it works as I need it to :-)

Whether it could be better to put migration stuff into system init script(s) ?
In general, consider that you are upstream, therefore you provide only tarball
(not rpm or deb package), and you have to do some migration things for new
version...

Also a good idea could be to play with trigger scripts, i.e.:

%triggerpostun -- samba <= latest_version_with_old_layout
some_code_here...

or

%triggerpostun common -- samba-common <= latest_...

Such a technique is already used by some packages (alsa-lib, openssh etc.) where
something should be cleaned up after old version deinstalled.
May be another type of trigger scripts are suitable here (%triggerin etc.)


Since /etc/samba/smbpasswd is a text file very similar to system /etc/passwd,
maybe better to save it under /etc/samba/ dir, not move too dip under
/var/lib/samba/private. (in other words, why /etc/passwd is not moved under,
say, /var/lib/security/secrets/ ? ).

(In reply to comment #9)
> Whether it could be better to put migration stuff into system init script(s) ?
> In general, consider that you are upstream, therefore you provide only tarball
> (not rpm or deb package), and you have to do some migration things for new
> version...

This is not something that happened inside samba it is a pure packaging problem
RH instroduce a long time ago and I am trying to fix now for better FHS compliance.
 
[..]

No, I need to make sure I do the migration when samba is stopped but _before_
smbd is restarted with condrestart.

> Since /etc/samba/smbpasswd is a text file very similar to system /etc/passwd,
> maybe better to save it under /etc/samba/ dir, not move too dip under
> /var/lib/samba/private. (in other words, why /etc/passwd is not moved under,
> say, /var/lib/security/secrets/ ? ).

While smbpasswd is a text file it is not intended to be manually manipulated, so
it goes with all other passdb backend files (like passdb.tdb which are binary).

smbpasswd is deprecated anyway, I am going to change the default smb.conf file
to use tdbsam as backend so that new installations stop using smbpasswd.
 

>- Either "samba-swat" must require "samba-doc", or move "docs/htmldocs" to
> "samba-swat" sub-package (else swat will contain broken links to help/docs)
Oops, actually another situation here :)
Currently "samba-swat" has all needed stuff, but it is completely duplicated in
samba-doc (Consider, f.e., /usr/share/swat/{help,using_samba} and
/usr/share/doc/samba-doc-3.0.24/htmldocs ).

Maybe leave all swat-needed in swat, all "big" PDFs and similar go to samba-doc,
all another things (examples, readmes etc.) go as "%doc" to an appropriate
subpackage. (BTW, now user have to install all the docs just to obtain some
small example files etc.)

I doubt whether samba.schema file should be provided in its "true" place,
/etc/openldap/schema. It leads to both openldap and samba own this dir...
Anyway a ldap admin must add "include /etc/openldap/schema/samba.schema"
manually to the /etc/openldap/slapd.conf file, therefore it is not too hard to
him/her to move the file to the right place just before slapd.conf editing.
Moreover, since the samba.schema file is not marked as %config, it can be
(unexpectedly) overwritten at the next samba update, which can potentially
conflict with the actual ldap base's data.

I love I have to go over all decision already made and re-explain everything,
but at least this way we have a public trace :-)

Ack for the docs, I'll see what can be done. Examples are docs, the user can go
on the website if really he is pissed off at downloading the docs package, we
can't split everything in down or, as someone said, we will end up with one
package per file :)

This was a specific request from users, the samba team from time, to time adds
stuff to the schema (this is why it is NOT marked as config, tracking it down is
the duty of the distro, by putting it there we make life easy for admins, and we
do no harm, they just need to restart ldap when they are confortable to read the
new schema (which is always backwads compatible so no harm in changing it). And
no, we don't own the directory.

> do the migration when samba is stopped but _before_ smbd is restarted with
condrestart.

Well, put the stuff into initrd script, somewhere _before_ the actual smbd/nmbd
invokation.
I.e., each time before the start(), initscript checks whether the correspond
files in /var/cache/samba/* exists and does all needed work before start the
daemon. Seems more robast for me, even notwithstanding it should be implemented
in all {smb,nmb,winbind} init scripts (perhaps splitted).

No can't be done in an init script.
a) it is not the right place to deal with packaging issues
b) we don't need to check at every restart, just on one of the upgrades from a
pre-script packages to a post-script one
c) (and most important) all three daemons read/write stuff in that dir, and it
is absolutely not correct that on of the scripts stop any other just to do the
migration, no way.

Schema:
> And no, we don't own the directory.
Hmmm, I see "%dir %{_sysconfdir}/openldap/schema" in the "samba" package's
%files section...

Well, maybe just mark the schema file as "%config(noreplace)" ? Openldap does it
for its schema files.


Docs:
> the user can go on the website 
"Conflicts: area-without-internet-access" ? ;)

> we can't split everything in down
I do not mean "split samba-doc to several subpackages", I mean "do not cause
user to install 13.5Mb package just to get some of the small files". Consider
autofs/, libsmbclient/, misc/, printer-accounting/, printer/, registry/
directories, for example. Unlike surely LDAP/, htmldocs/ (swat :) ) and *.pdf .

BTW, "COPYING" IMHO should be in the main sub-package (i.e. in "samba").

Generally, the recommendation "move big documentation to separate sub-package"
does dot mean "move all the documentation". Move just _really big_ stuff to it.


Migration:
> it is absolutely not correct that on of the scripts stop any other just
> to do the migration
I mean that each script does migration of its _own part_ of data. Certainly, if
it is possible (i.e. no situation where two different daemons have simultaneous
access to some *.tdb file).

Well, since initscript idea is not suitable, maybe consider triggers? It seems that:

%triggerun common -- samba-common < 3.0.24
stop_all_running_daemons and touch_its_var_lock
do_the_migration

cause migration to be performed before any "samba-common < 3.0.24" uninstall,
which looks more clean than "Requires(pre):" .

(BTW, if I'm not wrong, according to dependences, old "samba" is removed first,
then old "samba-common", ..., then new "samba-common" installed, then "samba".
It seems that if we use "%triggerpostun", not "%triggerun", then
"stop_all_running_daemons and touch_its_var_lock" is not needed at all...)

(In reply to comment #15)
> Schema:
> Well, maybe just mark the schema file as "%config(noreplace)" ? Openldap does
it for its schema files.

I'll think about that, but schema files are not really configuration files, so
it is suspicious for me.

> Docs:
> > the user can go on the website 
> "Conflicts: area-without-internet-access" ? ;)
Solution: Dload samba-docs :-)

> BTW, "COPYING" IMHO should be in the main sub-package (i.e. in "samba").

Yes, that was an oversight, I think.

> Generally, the recommendation "move big documentation to separate sub-package"
> does dot mean "move all the documentation". Move just _really big_ stuff to it.

Ok, I'll review that.

> Migration:
> > it is absolutely not correct that on of the scripts stop any other just
> > to do the migration
> I mean that each script does migration of its _own part_ of data. Certainly,
if it is possible (i.e. no situation where two different daemons have
simultaneous access to some *.tdb file).

Not really, that is why I want to do all in one place at one time, when I have
control of everything.

> Well, since initscript idea is not suitable, maybe consider triggers? It seems
that:
> 
> %triggerun common -- samba-common < 3.0.24
> stop_all_running_daemons and touch_its_var_lock
> do_the_migration
> 
> cause migration to be performed before any "samba-common < 3.0.24" uninstall,
> which looks more clean than "Requires(pre):" .
> 
> (BTW, if I'm not wrong, according to dependences, old "samba" is removed first,
> then old "samba-common", ..., then new "samba-common" installed, then "samba".
> It seems that if we use "%triggerpostun", not "%triggerun", then
> "stop_all_running_daemons and touch_its_var_lock" is not needed at all...)

No, removal happens _after_ installation on upgrades. Don;t aske me why but I
tested this stuf carefully, and I don;t see, right now, any better way, it works
and I'll keep like that, unless someone can show me a patch and the testing
procedure used to make sure it works :)

Sorry, I've understood that triggers do not help here (%post section is invoked
_before_).

But for "Requires(pre): samba-common-..." statement:

I've browsed "rpm-4.4.2" sources and it seems that "Requires(pre): " is unuseful
for package without "%pre" section. 

Have you checked up that such a requirement actually has an effect?

> I tested this stuff carefully
> it works
Well, it could mean that "Requires(pre):" just not needed at all...

Schema:
> but schema files are not really configuration files,
but allow it technically
> so it is suspicious for me
Since OpenLdap decides that schema files is (according to its openldap.spec):
%attr(0644,root,root) %config(noreplace) %{_sysconfdir}/openldap/schema/*.schema*

let's consider it as a precedent? ;)

> "Requires(pre):" just not needed at all...
to be more clear: "(pre)" seems to be unneeded, but usual
"Requires: samba-common = ..."
must be used.

(In reply to comment #19)

> Since OpenLdap decides that schema files is (according to its openldap.spec):
> %attr(0644,root,root) %config(noreplace) %{_sysconfdir}/openldap/schema/*.schema*
> 
> let's consider it as a precedent? ;)

Asked Nalin who made the original openldap package, at that time (1.x) it made
sense cause people were allowed to change tyhe schema files. Today people should
never change them. So I'll keep by not marking it as configuration.

Against 3.0.24-10:

- What about "auth" macro? Why the explicit or implicit "Requires: pam >= ..."
could not be enough?

- Certainly "samba" subpackage must require "samba-common". For the current rpm
program, "Requires(pre): something" implies an ordinary "Requires: something"
too, but to be more robast, add explicit "Requires: samba-common = ...." anyway,
notwithstanding of "...(pre)" existence.

- Whether "samba-doc" actually requires "samba-common"?

- Replace "/usr/lib", "/usr/lib*" just to %{_libdir}

- Changelog section still has "macros" (since 21 May 2001 and to EOF)

- It seems to be more useful to move "docs/registry" from "-docs" to the main
subpackage.

- All the docs/manpages contain non-ascii non-UTF8 symbols. Moreover, it looks
not like a "national symbol in author's name", it could be some formatting mess.
Do "iconv -f UTF-8 < nmbd.8" for example.

- Init scripts: there is no need to check for /etc/rc.d/init.d/finctions
existence. (f.e. httpd and vsftpd do not such an extra check).

- winbind.init: /var/lock/subsys name should match the initscript name, i.e.
should be "/var/lock/subsys/winbind", not ".../winbindd" (rmplint complains
about it).

Since "/var/cache/samba" is no more used, maybe create a symlink to
/var/lib/samba? Perhaps this way (old and new places are playing together) the
migration procedure could be simplified...

Shouldn't /etc/samba/smb.conf be a marked as %config(noreplace) or at least
%config? At the moment smb.conf gets overridden on every update.

For comment #23 :

Where you have seen it exactly?
I see "%config(noreplace) %{_sysconfdir}/samba/smb.conf" anywhere...

The file is replaced only if it was never touched at all IIRC.
It _IS_ marked as %config(noreplace)

Simo,

this ticket was not about comment #23 only... ;)

ops! :)
Do you still have open questions about the current package?

Maybe we can open a new bug with any missing piece?

We are going to move to 3.0.25 btw.

> Maybe we can open a new bug with any missing piece?

AFAIK the review ticket should be unique for all package's lifetime...

> We are going to move to 3.0.25 btw.

I know. Comment #22 seems still actual...

Created attachment 152814 [details]
some of proposed changes to the .spec file

according to the comment #22

Still pending remarks:

- Does "/usr/lib* instead of %{_libdir}" hack still needed for the latest
upstream versions?

- docs/manpages contain non-ascii non-UTF8 symbols (yet not checked out for
3.0.25 :) ). Non-UTF8 is deprecated now.
(BTW, run f.e. "man smbd" and see the man's warning in the first 3 lines).

- Init scripts: /etc/rc.d/init.d/finctions existance is checked "hardly",
whereas /etc/sysconfig/network is not... Since the init scripts are separate
sources for Fedora, it can be fixed easily.

- winbind.init: /var/lock/subsys name should match the initscript name, i.e.
should be "/var/lock/subsys/winbind", not ".../winbindd" (rmplint complains
about it).

Since "/var/cache/samba" is no more used, maybe create a symlink to
/var/lib/samba? (at least "for compatibility reasons" :) ) Perhaps this way
(old and new places are playing together) the migration procedure could be
simplified...

Created attachment 152907 [details]
proposed changes for winbind.init script

some cosmetic changes (useful for {smb|nmb}.init too), plus rename
subsys name to be matched with initscript name.

(In reply to comment #30)
> Still pending remarks:
> 
> - Does "/usr/lib* instead of %{_libdir}" hack still needed for the latest
> upstream versions?

I am trying to change it to %{_libdir}, let's see what happens.

> - docs/manpages contain non-ascii non-UTF8 symbols (yet not checked out for
> 3.0.25 :) ). Non-UTF8 is deprecated now.
> (BTW, run f.e. "man smbd" and see the man's warning in the first 3 lines).

This is an upstream problem, trying to fix it there.

> - Init scripts: /etc/rc.d/init.d/finctions existance is checked "hardly",
> whereas /etc/sysconfig/network is not... Since the init scripts are separate
> sources for Fedora, it can be fixed easily.

Ok, fixind this.

> - winbind.init: /var/lock/subsys name should match the initscript name, i.e.
> should be "/var/lock/subsys/winbind", not ".../winbindd" (rmplint complains
> about it).

Sorry but I can change the subsystem file name in the lock dir or upgrades would
fail to condrestart. Or I would have to check for both which is not pretty.

But you made me see that we don't start winbindd if networking is set to no.
With the winbindd offline mode, I think we may want to start it anyway.

Thougths about this?

> Since "/var/cache/samba" is no more used, maybe create a symlink to
> /var/lib/samba? (at least "for compatibility reasons" :) ) Perhaps this way
> (old and new places are playing together) the migration procedure could be
> simplified...

No, the migration would just break, as the script will find stuff in
/var/cache/samba and in /var/lib/samba, it will move the stuff in /var/lib/samba
to a backup and try to move /var/cache/samba to /var/lib/samba.
But as we moved away that stuff to backup it, the script will fail finiding
anything in /var/cache/samba and as result we just break new installtion by
leaving them without files in the real /var/lib/samba

Besides I want to see the absence of a /var/cache/samba directory as a mark we
have upgraded.

Simo.

> I can change the subsystem file name in the lock dir or upgrades would
fail to condrestart.
> Or I would have to check for both which is not pretty.

Since /var/cache/samba is already moved, does subsys renaming look like just an
addition for this process?..

Anyway, if you decided to cleanup samba, don't stop at the half of the way.
Let's do all the hard changes at once!

BTW, it seems that you can just rename lock files somewhere in the samba-common
%post script, because all condrestarts will be invoked later. 


> But you made me see that we don't start winbindd if networking is set to no.
> With the winbindd offline mode, I think we may want to start it anyway.

If "winbind offline mode" is designed not for temporary network absence only,
then maybe yes. 

Created attachment 153413 [details]
proposed .spec changes to handle winbindd --> winbind change

Consider this patch. Since %post section will be executed with the new code
(i.e. with "winbind", not "winbindd"), just check for and rename "winbindd"
lock to "winbind" before the actual service stop.

And apply the latest winbind.init cosmetic changes to smb.init and nmb.init
too.

Created attachment 154749 [details]
.spec file changes to handle winbindd --> winbind renaming

More robast way. Always check for old name in the %post section.

dmitry I will consider this for inclusion after F7 is released, as we are in
deep freeze I guess we have no other choice.

It seems that "winbind" name changes must be applied for all releases...

Consider the system boot/shutdown process. According to /etc/inittab,
"l0:0:wait:/etc/rc.d/rc 0" or
"l6:6:wait:/etc/rc.d/rc 6"

In "/etc/rc.d/rc" script we have such a fragment:

---start here----
# First, run the KILL scripts.
for i in /etc/rc$runlevel.d/K* ; do
        check_runlevel "$i" || continue

        # Check if the subsystem is already up.
        subsys=${i#/etc/rc$runlevel.d/K??}
        [ -f /var/lock/subsys/$subsys -o -f /var/lock/subsys/$subsys.init ] \
                || continue

        # Bring the subsystem down.
        if LC_ALL=C egrep -q "(killproc |action )" $i ; then
                $i stop
        else
                action $"Stopping $subsys: " $i stop
        fi
done

---end here---

For winbind we have "/etc/rc.d/rc0.d/K35winbind" symlink. The "subsys" value,
computed from this filename, is "winbind". But when we check
/var/lock/subsys/$subsys , we find nothing, as "/var/lock/subsys/winbindd"
exists there (double-d at the end instead of one).

IOW, it seems that with "winbindd" name, the init script will not be invoked for
"stop" at halt/reboot . And the similar when changing between runlevels (from 3
to 5 etc.)

Created attachment 158345 [details]
.spec file changes to work wiith winbindd to winbind renaming

This variant seems more safe. First, just duplicate "new" subsys lock, if old
is present, then remove the old lock, if it still exists.

No idea what about FC6 (and RHEL?) .

Ping.

We have to do something with the "winbind/winbindd" issue.

All three init scripts need to comply with LSB standard (headers and exit codes).

I am awaiting 3.2.0pre2 to appear to make a these changes.
I will also try to check LSB compliance at the same time.

Mass reassigning all merge reviews to their component. For more details, see this FESCO ticket:

  https://fedorahosted.org/fesco/ticket/1269

If you don't know what merge reviews are about, please see:

  https://fedoraproject.org/wiki/Merge_Reviews

How to handle this bug is left to the discretion of the package maintainer.

This bug appears to have been reported against 'rawhide' during the Fedora 23 development cycle.
Changing version to '23'.

(As we did not run this process for some time, it could affect also pre-Fedora 23 development
cycle bugs. We are very sorry. It will help us with cleanup during Fedora 23 End Of Life. Thank you.)

More information and reason for this action is here:
https://fedoraproject.org/wiki/BugZappers/HouseKeeping/Fedora23

This message is a reminder that Fedora 23 is nearing its end of life.
Approximately 4 (four) weeks from now Fedora will stop maintaining
and issuing updates for Fedora 23. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as EOL if it remains open with a Fedora  'version'
of '23'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora 23 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora, you are encouraged  change the 'version' to a later Fedora 
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

We rewrote the whole spec file for Samba 4.0.