Bug 453083 - (Samba4) Review Request: Samba4 - Samba4 CIFS and AD server and client
Review Request: Samba4 - Samba4 CIFS and AD server and client
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Matthias Clasen
Fedora Extras Quality Assurance
:
Depends On:
Blocks: libmapi 476315
  Show dependency treegraph
 
Reported: 2008-06-27 03:09 EDT by Andrew Bartlett
Modified: 2014-01-21 18:03 EST (History)
24 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-26 21:30:33 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mclasen: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Andrew Bartlett 2008-06-27 03:09:45 EDT
Spec URL: http://abartlet.net/samba4-rpm/samba4.spec
SRPM URL: http://abartlet.net/samba4-rpm/samba4-4.0.0-0.alpha4.1.fc9.src.rpm
Description: 
Samba 4 is the ambitious next version of the Samba suite that is being
developed in parallel to the stable 3.0 series. The main emphasis in
this branch is support for the Active Directory logon protocols used
by Windows 2000 and above, and to provide client libraries for these protocols.

This package has been specifically designed not to conflict with Samba3, and to provide the libraries and tools required to build OpenChange.
Comment 1 Jason Tibbitts 2008-06-27 13:00:39 EDT
Shouldn't this depend on the recently submitted heimdal package?
Comment 2 Andrew Bartlett 2008-06-27 19:00:50 EDT
It should, but at the moment we have not finished the extraction of
Samba-specific hacks from the internal copy of Heimdal (and the incorporation of
build system magic to detect an appropriate system Heimdal etc).  We are very
close, but not quite there yet. 
Comment 3 Jason Tibbitts 2008-06-29 13:57:59 EDT
Then I guess the next question is whether you would like a review of this now or
whether you've opened this to track the development effort.  Not that I can
promise to do a review myself, of course; this is a big package and several
people will probably needed share the review work.

Some initial comments from a quick look at the spec; I did not build the package:

Please use the proper versioning scheme for prerelease packages:
  Release: 0.1.alpha%{alpha_version}%{?dist}
             ^
and increment the '1' with each new release until 4.0.0 is actually released, at
which point you can just go to "Release: 1%{?dist}".  See
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Pre-Release_packages

Please use the correct License: tag; comma-separation is ambiguous and not
valid.  I'm not sure if the code is triple-licensed or if different pieces of
the built package have different licenses, but both situations are covered in
http://fedoraproject.org/wiki/Packaging/LicensingGuidelines.

Any possibility of parallel make?

The ldconfig call in %install is confusing to me.  What's it for?

Please use the proper scriptlets for user/group creation;
http://fedoraproject.org/wiki/Packaging/UsersAndGroups

Shouldn't the condrestart go in %postun, not %post?

You need the proper dependencies for the scriptlets.
  Requires(post): /sbin/chkconfig, /sbin/service
and so on.
Comment 4 Andrew Bartlett 2008-06-29 18:28:17 EDT
This package is submitted for immediate review.  I hope to see it included in
Fedora well before the final release of Samba 4.0.0.

Parallel make is specifically excluded in the samba4 build process.

The ldconfig call in %install is to create the macros that a post-install
ldconfig would provide, as required by rpmlint.

The group add and other scriptlets are copied from the Samba 3.2 package. 
Please also file a bug there. 

Thanks!
Comment 5 Andrew Bartlett 2008-07-01 01:16:20 EDT
Updated packages will continue to be placed at:

http://abartlet.net/samba4-rpm/
Comment 6 Kevin Kofler 2008-07-01 09:55:46 EDT
The License tag is still not correct (should be "GPLv3+ and LGPLv3+ and BSD", 
not "GPLv3+, LGPLv3+, BSD").
Comment 7 Andrew Bartlett 2008-07-01 22:17:15 EDT
Frankly, like the Samba 3.2 package, the situation is not even as simple as
fixing the syntax.  While I believe all the code to be under GPLv3 compatible
licences, I'll probably have to do a full licence sweep to verify the full list.

I know we have at least some GPLv2+ code (a crypto cypher, as it happens), and
different components in Heimdal (which is bundled here) come under different
wordings of of MIT or BSD style licences. 

To give you an idea of the scope of the work, the python-wmi package is actually
a fork of Samba4 from over a year ago, but has this many licences:
http://changelogs.ubuntu.com/changelogs/pool/universe/w/wmi/wmi_0.1.6-1/python-wmi.copyright

Some of these have changed in the past year, but I use this as evidence I'm
pretty certain the licence tag header will only get longer. 

When I get the full list, I'll fix the tag.  
Comment 8 Rex Dieter 2008-07-24 14:00:50 EDT
Well, I'll chip away at this (any external review help welcome).
Comment 9 Rex Dieter 2008-07-24 14:17:44 EDT
1.  Should drop Epoch: 0

2.  drop
BR: ldconfig

3.  drop extraneous, hard-coded library deps
Requires: libtdb >= 0:%{tdb_version}
Requires: libtalloc >= 0:%{talloc_version}
should already be satisified by fedora's default versions (verified F9+)

4.  the -common deps
Requires(post): /sbin/chkconfig, /sbin/service, coreutils
Requires(preun): /sbin/chkconfig, /sbin/service
are misplaced.  these need to be in main pkg.  (and what's coreutils used for?)

5.  SHOULD use a recommended buildroot (cosmetic only).

Going to try some builds...
Comment 10 Andrew Bartlett 2008-07-24 20:03:48 EDT
I'll chance the tdb and talloc to BuildRequires. 

We need the BR: ldconfig because we use it in the build (see %install). 
Comment 11 Rex Dieter 2008-07-24 22:09:27 EDT
ldconfig is an exception, that you can assume is available on any (sane)
buildystem or runtime.  (I can find the packaging guideline about that if you want).
Comment 12 Andrew Bartlett 2008-07-25 02:43:33 EDT
Fixes for a few of these are not in Samba.org's git tree and on my website as above

Comment 13 Andrew Bartlett 2008-07-28 21:37:00 EDT
BTW, the reason I took and used the tdb_version and talloc_version stuff from
the Samba3 spec is that I do expect we might update this requirement in future,
as both are fairly core Samba packages. 
Comment 14 Andrew Bartlett 2008-07-28 21:43:04 EDT
And I need to be sponsored
Comment 15 Andrew Bartlett 2008-08-29 00:35:34 EDT
Updated packages are now on my web page, which should address most of these issues, and others that actual use by kdepim raised.
Comment 16 Matthew Barnes 2008-12-12 17:58:41 EST
Taking over this effort for the time being to get evolution-mapi approved.

I've updated the Samba4 packages to the GIT revision that OpenChange (rev 909) is currently requiring.

SPEC:
http://mbarnes.fedorapeople.org/mapi/SPECS/samba4.spec

SRPM:
http://mbarnes.fedorapeople.org/mapi/SRPMS/samba4-4.0.0-0.6.alpha6.GIT.3508a66.fc10.src.rpm

Changes from Andrew's latest revision:
http://mbarnes.fedorapeople.org/mapi/SPECS/samba4.spec.diff
Comment 17 Jerry Amundson 2008-12-13 21:52:01 EST
[root@walnut ~]# rpm -Uvh --test ~jerry/rpmbuild/RPMS/i386/samba4-common-4.0.0-0.6.alpha6.GIT.3508a66.fc11.i386.rpm ~jerry/rpmbuild/RPMS/i386/samba4-client-4.0.0-0.6.alpha6.GIT.3508a66.fc11.i386.rpm ~jerry/rpmbuild/RPMS/i386/samba4-winbind-4.0.0-0.6.alpha6.GIT.3508a66.fc11.i386.rpm ~jerry/rpmbuild/RPMS/i386/samba4-libs-4.0.0-0.6.alpha6.GIT.3508a66.fc11.i386.rpm ~jerry/rpmbuild/RPMS/i386/samba4-4.0.0-0.6.alpha6.GIT.3508a66.fc11.i386.rpm
Preparing...                ########################################### [100%]
        file /lib/libnss_winbind.so.2 from install of samba4-winbind-4.0.0-0.6.alpha6.GIT.3508a66.fc11.i386 conflicts with file from package samba-winbind-0:3.2.5-0.23.fc11.i386
        file /usr/bin/ntlm_auth from install of samba4-winbind-4.0.0-0.6.alpha6.GIT.3508a66.fc11.i386 conflicts with file from package samba-winbind-0:3.2.5-0.23.fc11.i386
        file /usr/bin/wbinfo from install of samba4-winbind-4.0.0-0.6.alpha6.GIT.3508a66.fc11.i386 conflicts with file from package samba-winbind-0:3.2.5-0.23.fc11.i386
        file /usr/lib/libnss_winbind.so from install of samba4-winbind-4.0.0-0.6.alpha6.GIT.3508a66.fc11.i386 conflicts with file from package samba-winbind-0:3.2.5-0.23.fc11.i386
Comment 18 Matthew Barnes 2008-12-13 23:14:35 EST
Yeah, not sure what to do about the library.

File conflicts in other samba4 subpackages were all binaries, which I resolved by simply appending a '4' to the name.

Ideas?
Comment 19 Matthew Barnes 2008-12-13 23:16:06 EST
Note, the samba4-winbind subpackage isn't needed by OpenChange.
Comment 20 David Robinson 2008-12-14 19:30:24 EST
Doesn't this need a "BuildRequires: python-devel"?
Comment 21 Andrew Bartlett 2008-12-15 00:19:56 EST
I will note that while it might not help with packaging, it is the intention of the Samba Team to only have one wbinfo and one nss_winbind, which will talk to either Samba3 or Samba4's winbindd (ie, share a protocol).
Comment 22 Matthew Barnes 2008-12-15 01:01:21 EST
What about libnss_winbind.so, is that also compatible with Samba3?

If so, perhaps we should just disable the samba4-winbind subpackage until we move to Samba4 officially?
Comment 23 Andrew Bartlett 2008-12-15 02:19:53 EST
So, I should clarify:

Samba4 and Samba 3.4 are being developed in the samba git repository - under a branch of 'master'.  

In that branch, there will be soon (I hope) a common libnss_winbind and a common wbinfo, along with many other shared things.  

But because this is not a stable protocol, the protocol used will be different to what Samba3 in current Fedora uses.
Comment 24 Jerry Amundson 2008-12-15 02:40:48 EST
(In reply to comment #23)
> So, I should clarify:
> 
> Samba4 and Samba 3.4 are being developed in the samba git repository - under a
> branch of 'master'.  
> 
> In that branch, there will be soon (I hope) a common libnss_winbind and a
> common wbinfo, along with many other shared things.  
> 
> But because this is not a stable protocol, the protocol used will be different
> to what Samba3 in current Fedora uses.

Then, logically, libnss_winbind and wbinfo, et. al, will not be "common", ergo they need to be packaged separately, or not at all, right?

Put another way, do the Samba developers consider the 4.x and 3.4 as environments that can exist in parallel, or not? Either it is or it isn't.

Otherwise, wouldn't the direction of this package be faulty? If one or more "shared things" are in the forecast then, it would seem, disabling it until v. 4 is "official" is the way to go.
Comment 25 Matthew Barnes 2008-12-15 07:24:49 EST
(In reply to comment #24)
> Otherwise, wouldn't the direction of this package be faulty? If one or more
> "shared things" are in the forecast then, it would seem, disabling it until v.
> 4 is "official" is the way to go.

Bearing in mind the primary reason for packaging Samba4 at this time is to support OpenChange, and OpenChange doesn't need the winbind stuff, I'm inclined to just omit the subpackage for now.

Will try to post another round of updates later today.
Comment 26 Matthew Barnes 2008-12-15 15:31:17 EST
Andrew, something annoying I've been struggling with:

If I already have the samba4 packages installed and I go to rebuild them, the configure script wants to link against the libldb.so from samba4-libs instead of building its own.  So I have to uninstall all my samba4 packages (along with openchange and evolution-mapi) before building new ones.

Any way to force samba to build its own ldb whether it finds one already installed or not?
Comment 27 Matthew Barnes 2008-12-16 11:25:57 EST
Update.  Disabled the winbind subpackage so there shouldn't be any more file conflicts.

http://mbarnes.fedorapeople.org/mapi/SPECS/samba4.spec
http://mbarnes.fedorapeople.org/mapi/SRPMS/samba4-4.0.0-0.7.alpha6.GIT.3508a66.fc10.src.rpm
Comment 28 Matthew Barnes 2008-12-18 10:21:54 EST
Update.  Found another file conflict with Samba3 (/usr/bin/smbstatus).

http://mbarnes.fedorapeople.org/mapi/SPECS/samba4.spec
http://mbarnes.fedorapeople.org/mapi/SRPMS/samba4-4.0.0-0.8.alpha6.GIT.3508a66.fc10.src.rpm
Comment 29 Jerry Amundson 2008-12-18 13:22:46 EST
(In reply to comment #28)
> Update.  Found another file conflict with Samba3 (/usr/bin/smbstatus).
> 
> http://mbarnes.fedorapeople.org/mapi/SPECS/samba4.spec
> http://mbarnes.fedorapeople.org/mapi/SRPMS/samba4-4.0.0-0.8.alpha6.GIT.3508a66.fc10.src.rpm

Processing files: samba4-devel-4.0.0-0.8.alpha6.GIT.3508a66.fc11
error: File not found: /home/jerry/rpmbuild/BUILDROOT/samba4-4.0.0-0.8.alpha6.GIT.3508a66.fc11.i386/usr/lib/libldb.so


RPM build errors:
    File not found: /home/jerry/rpmbuild/BUILDROOT/samba4-4.0.0-0.8.alpha6.GIT.3508a66.fc11.i386/usr/lib/libldb.so

Just me? I haven't had a chance to research why yet.
Comment 30 Matthew Barnes 2008-12-18 15:57:16 EST
See comment #26.

You have to uninstall samba4 before rebuilding it, otherwise it just links to the already-installed libldb.so and doesn't build it itself.
Comment 31 Jerry Amundson 2008-12-18 16:13:46 EST
/me smacks hand on forehead, I knew libldb.so sounded familiar!
Comment 32 Rex Dieter 2008-12-21 16:20:19 EST
my apologies, my time/interest has waned, so I'll withdraw to let someone else jump in.
Comment 33 Matthew Barnes 2009-01-20 07:25:02 EST
Update for official Alpha6 release.  No changes to packaging.

http://mbarnes.fedorapeople.org/mapi/SPECS/samba4.spec
http://mbarnes.fedorapeople.org/mapi/SRPMS/samba4-4.0.0-1.alpha6.fc10.src.rpm
Comment 34 Jerry Amundson 2009-01-21 21:10:43 EST
(In reply to comment #26)
> Andrew, something annoying I've been struggling with:
> 
> If I already have the samba4 packages installed and I go to rebuild them, the
> configure script wants to link against the libldb.so from samba4-libs instead
> of building its own.  So I have to uninstall all my samba4 packages (along with
> openchange and evolution-mapi) before building new ones.
> 
> Any way to force samba to build its own ldb whether it finds one already
> installed or not?

Has progress been made here? Annoying, yes, and I'm guessing would be a blocker at some point...
Comment 35 Matthew Barnes 2009-01-21 23:11:08 EST
Only solution I can think of that doesn't involve hacking on Samba's build scripts is to split ldb into it's own standalone package (not a subpackage) and list it as a BuildRequires in the samba4.spec.
Comment 36 Tom "spot" Callaway 2009-01-22 10:04:21 EST
Red Hat Legal is still deliberating over this one. Once I hear back from them, I should be able to move forward (in one way or another).
Comment 37 Tom "spot" Callaway 2009-01-23 13:05:47 EST
Lifting FE-Legal, Red Hat has determined this can proceed.
Comment 38 Trever Adams 2009-01-26 08:49:16 EST
Will we see this in F11-Rawhide soon?

Is it possible to add winbind for those of us who are playing with it looking to the day it can replace Samba3?
Comment 39 Matthew Barnes 2009-01-26 08:58:03 EST
(In reply to comment #38)
> Will we see this in F11-Rawhide soon?

Not until someone reviews the package.


> Is it possible to add winbind for those of us who are playing with it looking
> to the day it can replace Samba3?

For now, Samba4 is being added primarily to support OpenChange, and OpenChange does not require the winbind subpackage.  It also conflicts with Samba3.
Comment 40 Matthias Clasen 2009-01-28 00:37:17 EST
The srpm from comment 33 builds fine in mock.

Here is the rpmlint output:

samba4.i386: W: no-documentation
samba4.i386: E: non-standard-dir-perm /var/log/samba4/old 0700
samba4.i386: E: non-standard-dir-perm /var/log/samba4 0700
samba4.src: W: mixed-use-of-spaces-and-tabs (spaces: line 115, tab: line 145)
samba4-client.i386: W: no-documentation
samba4-common.i386: E: non-standard-dir-perm /var/lib/samba4/private 0700
samba4-debuginfo.i386: W: spurious-executable-perm /usr/src/debug/samba-4.0.0alpha6/source4/torture/raw/pingpong.c
samba4-devel.i386: W: no-documentation
samba4-python.i386: W: no-documentation
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/ndr.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/samdb.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/__init__.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/tests/dcerpc/unix.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/tests/samdb.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/samba3.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/getopt.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/tests/dcerpc/rpcecho.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/tests/dcerpc/sam.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/hostconfig.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/tests/dcerpc/registry.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/tests/__init__.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/idmap.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/tests/samba3.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/tests/upgrade.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/upgrade.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/tests/dcerpc/bare.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/dcerpc/__init__.py 0644
samba4-python.i386: E: non-executable-script /usr/lib/python2.6/site-packages/samba/tests/provision.py 0644
9 packages and 0 specfiles checked; 22 errors, 6 warnings.
Comment 41 Matthias Clasen 2009-01-28 11:13:01 EST
Looks like the description of the -python subpackage should end in a period, not a slash.
Comment 42 Matthias Clasen 2009-01-28 11:14:13 EST
And I think python wants to be capitalized when used as a name (compare descriptions of other python packages).
Comment 43 Matthias Clasen 2009-01-28 12:39:07 EST
There is some mixed use of $RPM_BUILD_ROOT and %{buildroot} that should probably be cleaned up always use the one or the other.
Comment 44 Matthias Clasen 2009-01-28 12:41:36 EST
I think the disabling of the winbind package would better be done with

%if %enable_winbind

or somesuch
Comment 45 Matthias Clasen 2009-01-28 14:37:50 EST
the main samba4 package include something called /usr/bin/tdbtorture, which from the looks of it, is a test program for the tdb code. As such, it should probably go in the -devel package, if at all.
Comment 46 Matthias Clasen 2009-01-28 14:42:58 EST
Similar for smbtorture and subunitrun in the -common subpackage
Comment 47 Matthias Clasen 2009-01-28 14:55:52 EST
And I'm not sure we want a program called /usr/bin/nsstest shipped as part of samba4-devel. 


The -python subpackage has a bunch of autogenerated provides that I don't think it should have:
atsvc.so  
auth.so  
base.so  
com.so  
credentials.so  
dfs.so  
drsuapi.so  
echo.so  
epmapper.so  
glue.so  
initshutdown.so  
irpc.so  
ldb.so  
lsa.so  
messaging.so  
mgmt.so  
misc.so  
nbt.so  
net.so  
netbios.so  
param.so  
registry.so  
samr.so  
security.so  
svcctl.so  
tdb.so  
tevent.so  
unixinfo.so  
uuid.so  
winreg.so  
wkssvc.so
Comment 48 Matthias Clasen 2009-01-28 14:56:54 EST
The -devel package shouldn't own /usr/lib/pkgconfig
Comment 49 Matthias Clasen 2009-01-28 16:26:20 EST
If you clean up these things, I'll start a formal review.
Comment 50 Matthew Barnes 2009-01-29 14:54:23 EST
Simo and I were talking about splitting several Samba libraries (talloc, tevent, tdb and ldb) into a separate, standalone "samba-base" package to be consumed by Samba3, Samba4, OpenChange and SSSD [1].  We also think we want to disable, for the time being, any parts of Samba4 not needed by OpenChange (by means of some simple toggle settings in the spec file).

Expect more churn here before a formal review can start.  In the meantime I'll clean up the details Matthias pointed out.  Thanks again, Matthias.

[1] http://fedoraproject.org/wiki/Features/SSSD
Comment 51 Matthias Clasen 2009-02-21 02:04:04 EST
Any update here ?
Comment 52 Matthew Barnes 2009-02-21 12:33:42 EST
It's looking like Samba4 is again not going to make the cut.  I'm waiting on Simo Sorce to deliver a interim subset package containing the Samba4 libraries needed for OpenChange.
Comment 53 Simo Sorce 2009-02-21 13:19:23 EST
Release of separate libraries is also slipping :-(

Matt I think it would be safe for now to just grab your own copy of samba4 frozen in a status the openchange people is ok with, and just build the few libraries you need yourself and store them into a private path as part of the openchange package.

While not ideal, I think this will make it easier to release openchange in F11, once there, we will try to gradually split out dependencies as pieces stabilize upstream and become available as official releases.
Comment 54 Matthias Clasen 2009-02-21 14:58:53 EST
Too much indecision here... 
Matt, lets just go with the initial packaging approach.
Comment 55 Matthew Barnes 2009-02-21 16:00:42 EST
Agreed.  I'll continue addressing your review comments.

I don't want to be supporting a quick and dirty hack in RHEL 6.
Comment 56 Andrew Bartlett 2009-02-22 20:35:22 EST
Is there anything more I can do from the Samba4 development standpoint to help this along?   Do you need another alpha snapshot, or is it OK to cut a GIT revision at the appropriate time?
Comment 57 Matthew Barnes 2009-02-22 22:08:49 EST
Summary of some offline conversation with Simo:

The approach we're going to try is to stick with the existing Samba4 package already under review here, but for Fedora 11 disable the bits not needed by OpenChange.  Basically lobotomize Samba4 until we're ready to support it.

Simo needs a more recent revision of talloc and tdb, so we'll patch Samba3 to provide that (assuming those libraries haven't broken backward compatibility).

I expect OpenChange to be frozen at 0.8 for the duration of Fedora 11, so alpha6 is sufficient for me.  If Simo needs something newer I'd prefer a new upstream release over rolling my own GIT revision, to avoid distro-specific quirks.
Comment 58 Andrew Bartlett 2009-02-22 22:19:38 EST
I fully support this plan of action.
Comment 59 Simo Sorce 2009-02-22 22:30:18 EST
Yep I definitely need the latest tevent and ldb code from samba4.
Comment 60 Matthew Barnes 2009-02-23 16:56:43 EST
New packages up for review.  This update disables most of Samba4, leaving only what OpenChange needs.  It also addresses most of Matthias' review comments.  Not sure how to deal with the Python provides issue in comment #47, but the subpackage is disabled so it's probably not worth worrying about for now.

Successfully built and installed OpenChange and Evolution-MAPI against this package set.

http://mbarnes.fedorapeople.org/mapi/SPECS/samba4.spec
http://mbarnes.fedorapeople.org/mapi/SRPMS/samba4-4.0.0-2.alpha6.fc10.src.rpm
Comment 61 Matthias Clasen 2009-02-25 12:35:07 EST
Indeed, all my earlier comments have been taken care of, thanks.
Comment 62 Matthias Clasen 2009-02-25 13:14:32 EST
Package builds fine in mock

Formal review: 

rpmlint output: 

samba4.x86_64: E: no-binary
samba4-devel.x86_64: W: no-documentation
6 packages and 0 specfiles checked; 1 errors, 1 warnings.

The warning is ignorable, the error is caused by the main package being an empty shell for now. I understand this is just temporary, until samba4 gets
released, so I don't think this is an issue.

package name: ok
spec file name: ok
packaging guidelines: ok; I guess you could be proactive and adapt to the coming recommendation of %global over %define, but thats not ratified yet, afaik
license: ok
license field: ok, but it would be nice to specify more exactly what parts are LGPL
license file: ok
spec language: ok
spec legible: ok
upstream source: ok
buildable: ok
buildrequires: ok
excludearch: ok
locale handling: ok
ldconfig: ok
relocatable: ok
directory ownership: ok
duplicate files: ok
permissions: ok, I notice that pidl uses %defattr(-,root,root,-) whereas the others use %defattr(-,root,root). Accident ? The former is preferred, I think
%clean: ok
macro use: ok
permissible content: ok
large docs: ok
%doc content: ok
headers: ok
pkgconfig: ok
shared libs: ok
-devel requires: ok, it requires -libs
la files: ok
gui apps: ok
overlap with other packages: NOT ok. -pidl includes things that are owned by other packages, notably perl-Parse-Yapp
%install: ok
utf8 filenames: ok


summary: 
- consider using %global
- add license comment
- consider cleaning up %defattr variation
- fix -pidl conflicts
Comment 63 Matthew Barnes 2009-02-25 16:46:51 EST
Latest (hopefully final) update.

http://mbarnes.fedorapeople.org/mapi/SPECS/samba4.spec
http://mbarnes.fedorapeople.org/mapi/SRPMS/samba4-4.0.0-3.alpha6.fc10.src.rpm

> summary: 
> - consider using %global

I think I'll hold off on this until it's ratified and just convert all my packages en masse.

> - add license comment

Done.  Simo said the library licenses are still subject to GPL/LGPL fluctuation, but everything else is GPL.

> - consider cleaning up %defattr variation

Done.

> - fix -pidl conflicts

Done, I think.  Only conflicts I found were:

   /usr/lib/perl5/vendor_perl/5.10.0/Parse/Yapp
   /usr/lib/perl5/vendor_perl/5.10.0/Parse/Yapp/Driver.pm

I removed Samba's copy and added perl-Parse-Yapp as a build requirement.
Were there any others?
Comment 64 Matthias Clasen 2009-02-25 17:57:18 EST
Were there any others?

I think /usr/lib/perl5/vendor_perl/5.10.0/Parse

is multiply owned. Not sure if thats intentional, or if there is some official owner. Easiest way out might be to Require perl(Parse::Yapp) - or do you pick that Requires up anyway ?


The rest looks ok, so approved under the assumption that you have a Requires for an owner of that directory.
Comment 65 Matthew Barnes 2009-02-25 18:21:49 EST
(In reply to comment #64)
> I think /usr/lib/perl5/vendor_perl/5.10.0/Parse
> 
> is multiply owned. Not sure if thats intentional, or if there is some official
> owner. Easiest way out might be to Require perl(Parse::Yapp) - or do you pick
> that Requires up anyway ?

Not sure if build requirements get propagated as requirements with Perl modules.  I'll just make it an explicit requirement, to be sure.

Thanks for the review.
Comment 66 Matthew Barnes 2009-02-25 18:29:53 EST
New Package CVS Request
=======================
Package Name: samba4
Short Description: Samba version 4
Owners: mbarnes
Branches:
InitialCC: simo
Comment 67 Kevin Fenzi 2009-02-26 19:31:08 EST
cvs done.
Comment 68 Matthew Barnes 2009-02-26 21:30:33 EST
Package built.  Closing review.  Finally.

http://kojipkgs.fedoraproject.org/packages/samba4/
Comment 69 Fedora Update System 2012-11-28 09:43:19 EST
gnome-vfs2-2.24.4-9.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/gnome-vfs2-2.24.4-9.fc18

Note You need to log in before you can comment on or make changes to this bug.