-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 06/11/2013 10:07 AM, Jakub Hrozek wrote:
> On Tue, Jun 11, 2013 at 09:37:49AM -0400, Stephen Gallagher wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>>
>> On 06/11/2013 06:03 AM, Jakub Hrozek wrote:
>>> On Mon, Jun 10, 2013 at 03:49:34PM -0400, Stephen Gallagher
>>> wrote: On 06/10/2013 02:24 PM, Jakub Hrozek wrote:
>>>>>> On Mon, Jun 10, 2013 at 08:56:37AM -0400, Stephen
>>>>>> Gallagher wrote:
>>>>>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>>>>>>>
>>>>>>> On 06/10/2013 07:17 AM, Lukas Slebodnik wrote:
>>>>>>>> On (10/06/13 10:03), Jakub Hrozek wrote:
>>>>>>>>> On Fri, Jun 07, 2013 at 06:24:48PM +0200, Lukas
>>>>>>>>> Slebodnik wrote:
>>>>>>>>>> 1. I like idea of divided subpackages. If
>>>>>>>>>> someone wants only ldap backend, he needn't
>>>>>>>>>> install samba-libs (and its dependencies)
>>>>>>>>>>
>>>>>>>>>> 2. There isn't any rpmlint warnings.
>>>>>>>>>>
>>>>>>>>>> I tested yum upgrade upgrade with installed
sssd
>>>>>>>>>> and freeipa-client. New packages were installed
>>>>>>>>>> for dependencies: sssd-ad sssd-common sssd-ipa
>>>>>>>>>> sssd-krb5 sssd-krb5-common sssd-ldap Everything
>>>>>>>>>> worked as expected.
>>>>>>>>>>
>>>>>>>>>> Then I decided to remove sssd-ad: yum remove
>>>>>>>>>> sssd-ad and packeges "freeipa-client,
sssd" were
>>>>>>>>>> also removed. I was little bit confused,
because
>>>>>>>>>> I didn't want to remove sssd and sssd
replied to
>>>>>>>>>> getent command after packages
"freeipa-client,
>>>>>>>>>> sssd" were removed.
>>>>>>>>>
>>>>>>>>> That's because freeipa-client currently
requires
>>>>>>>>> sssd, we might want open a ticket to make them
>>>>>>>>> require just sssd-ipa.
>>>>>>>>>
>>>>>>>>
>>>>>>>> The most confusing thing was for me that sssd was
>>>>>>>> removed. Yes, we should file a ticket to
>>>>>>>> freeipa-client after releasing sssd.
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I think, that other users may be also confused
>>>>>>>>>> with this situation. Then I looked to the patch
>>>>>>>>>> and I found out, that: --sssd is only
"meta
>>>>>>>>>> package",which require all backedns
subpackages
>>>>>>>>>> --sssd doesn't contain any useful files
>>>>>>>>>> --everything important is in package
>>>>>>>>>> sssd-common.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Maybe we should update package description of
>>>>>>>>>> sssd and sssd-common. I hope that system
>>>>>>>>>> administrators relies on output of "yum
info" and
>>>>>>>>>> there isn't it very well explained.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks, I updated the summary of both sssd and
>>>>>>>>> sssd-common. Hopefully it would be clearer now.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Thank you.
>>>>>>>>
>>>>>>>>>> Summary: Everything works well, but I was
little
>>>>>>>>>> bit confused.
>>>>>>>>>>
>>>>>>>>>> Any other opinions?
>>>>>>>>>>
>>>>>>>>>> One nitpick inline
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>> I think, that patches are OK. Does anybody have
>>>>>>>> another comments or objections?
>>>>>>>>
>>>>>>>
>>>>>>> Nack.
>>>>>>>
>>>>>>> When adding "Provides:" and
"Obsoletes:", it is
>>>>>>> inappropriate to do Obsoletes: libsss_sudo <
>>>>>>> %{version}-%{release}
>>>>>>>
>>>>>>> The Fedora guidelines require that the Obsoletes: be a
>>>>>>> specific version (the last one before the change
>>>>>>> occurred). So
>>>>>>>
>>>>>>> Obsoletes: libsss_sudo <= 1.9.5
>>>>>>>
>>>>>>
>>>>>> OK, in my defense I've seen these in some of the
>>>>>> specfiles I've got cloned from Fedora's dist-git (not
>>>>>> that it would make them correct).
>>>>>>
>>>>>> I made the changes in the version that is attached, just
>>>>>> with a differens version to compare against. I think
>>>>>> your suggestion would make sense for a Fedora package,
>>>>>> but in upstream, we want to obsolete all the nightly
>>>>>> builds prior to the switch as well.
>>>>>>
>>>>>> So I added another point version number (minor versions
>>>>>> come cheap) and compared against 1.9.93.
>>>>>>
>>>
>>> It needs to be <= 1.9.93. Just less-than won't upgrade
>>> packages *at* 1.9.93
>>>
>>> Wouldn't things be easier to remember if you just bumped
>>> version.m4 to 1.9.94 instead? Then you don't need to remember
>>> to revert the specfile patch. (You can just release 1.9.94. No
>>> one is really going to notice if the upstream minor release
>>> number jumped two digits instead of one between pre-releases).
>>>
>>>
>>>> Yes, if it's OK to bump the version then it's definitely
>>>> easier. Done.
>>>
>>>
>>>>>>> The reason for this is so we don't force yum to handle
>>>>>>> Obsoletes processing at every update (which I'm told
>>>>>>> is expensive).
>>>>>>>
>>>>>>
>>>>>> I see, I didn't know that.
>>>>>>
>>>
>>> Technically, it also allows the capability to add that back in
>>> as a package or subpackage at a higher version at some later
>>> date without breakage (because the re-added one would be at a
>>> higher N-V-R). That's not something that matters to us though,
>>> just a bit of academia.
>>>
>>>>>>> We only need to carry this explicit Obsoletes: until
>>>>>>> Fedora no longer has install media with the older
>>>>>>> version (so until F19 is the oldest supported
>>>>>>> version).
>>>>>>>
>>>>>>>
>>>>>>> Why do you have Conflicts: sssd <
>>>>>>> %{version}-%{release}? Given that the packages are
>>>>>>> identically-named (and that name isn't
'kernel'), this
>>>>>>> should be unnecessary.
>>>>>>>
>>>>>>
>>>>>> I wanted to be really defensive and make sure that
>>>>>> everything the old version contained is gone with the new
>>>>>> version. That Conflicts: is gone with the attached
>>>>>> patches.
>>>>>>
>>>>>>>
>>>>>>> Is there a particular reason that the proxy provider
>>>>>>> doesn't have a subpackage as well?
>>>>>>>
>>>>>>
>>>>>> Just that the proxy provider has no dependencies. But
>>>>>> from a logical standpoint it makes sense, so there is a
>>>>>> proxy subpackage now, too.
>>>>>>
>>>
>>> Thanks, but you forgot to add it to the dependencies of the
>>> 'sssd' meta-package.
>>>
>>>
>>>> Oops, fixed.
>>>
>>>
>>> If we're being pedantic about this, we should probably split
>>> out the 'simple' provider as well, but I'm not going to nack
>>> over it. It's < 40k.
>>>
>>>
>>>> I think it's better if the simple provider stays in the
>>>> common package. It can be used along with any other provider,
>>>> so the other providers would have to require it anyway. Also,
>>>> it has no dependencies.
>>>
>>>
>>>>>>> If we're breaking up packages to make it easier to
>>>>>>> create a minimal install, might it be best to just drop
>>>>>>> all of the manpages into an sssd-docs subpackage (that
>>>>>>> gets pulled in by the sssd metapackage)? Including
>>>>>>> translations, those can add up. Which brings me to my
>>>>>>> next point:
>>>>>>>
>>>>>>> The provider subpackages have only the English manpage
>>>>>>> included in them. The translated ones are all landing
>>>>>>> in the sssd-common package.
>>>>>>
>>>>>> I don't really like this option. As a user of SSSD I
>>>>>> would expect the sssd-ad man page to be present if I
>>>>>> installed the sssd-ad package. Also, I think it is better
>>>>>> to have a single man page installed (even if it's along
>>>>>> with the translations) for the provider I care about than
>>>>>> the whole bunch. The space savings provided by this split
>>>>>> would also come by the means of not installing all the
>>>>>> dependencies rather than not installing the files that
>>>>>> form the providers.
>>>>>>
>>>>>> Thank you for the review, new patches are attached.
>>>>>>
>>>
>>>
>>> Hmm, the translated manpages seem to have disappeared from the
>>> builds entirely:
>>>
>>>
>>>> I see the same behaviour with parallel build (also with
>>>> master w/o these patches), but with an in-tree build, the man
>>>> pages are generated fine.
>>>
>>>> Here is a scratch build I built:
>>>>
http://koji.fedoraproject.org/koji/taskinfo?taskID=5490069
>>>
>>>> For instance, check out the sssd-ldap subpackage: $ rpm -qpl
>>>> sssd-ldap-1.9.94-0.fc19.x86_64.rpm | grep man
>>>> /usr/share/man/ja/man5/sssd-ldap.5.gz
>>>> /usr/share/man/man5/sssd-ldap.5.gz
>>>> /usr/share/man/uk/man5/sssd-ldap.5.gz
>>>
>>>
>>> [sgallagh@sgallagh520:x86_64 (master *)]$ rpm -qpl
>>> /home/sgallagh/workspace/sssd/x86_64/rpmbuild/RPMS/x86_64/sssd-*.rpm
>>>
>>>
>>
>>>
|grep man
>>> /usr/share/man/man5/sssd-ad.5.gz
>>> /usr/share/man/man8/pam_sss.8.gz
>>> /usr/share/man/man8/sssd_krb5_locator_plugin.8.gz
>>> /usr/share/man/man1/sss_ssh_authorizedkeys.1.gz
>>> /usr/share/man/man1/sss_ssh_knownhostsproxy.1.gz
>>> /usr/share/man/man5/sssd-simple.5.gz
>>> /usr/share/man/man5/sssd-sudo.5.gz
>>> /usr/share/man/man5/sssd.conf.5.gz
>>> /usr/share/man/man8/sss_cache.8.gz
>>> /usr/share/man/man8/sssd.8.gz /usr/share/man/man5/sssd-ipa.5.gz
>>> /usr/share/man/man5/sssd-krb5.5.gz
>>> /usr/share/man/man5/sssd-ldap.5.gz
>>> /usr/share/man/man8/sss_debuglevel.8.gz
>>> /usr/share/man/man8/sss_groupadd.8.gz
>>> /usr/share/man/man8/sss_groupdel.8.gz
>>> /usr/share/man/man8/sss_groupmod.8.gz
>>> /usr/share/man/man8/sss_groupshow.8.gz
>>> /usr/share/man/man8/sss_obfuscate.8.gz
>>> /usr/share/man/man8/sss_seed.8.gz
>>> /usr/share/man/man8/sss_useradd.8.gz
>>> /usr/share/man/man8/sss_userdel.8.gz
>>> /usr/share/man/man8/sss_usermod.8.gz
>>>
>>>
>>> I'm running it as a Koji scratch-build right now to make sure
>>> it's not some peculiarity of my system, but I think there's
>>> something wrong with the *.lang files :-/
>>
>>
>> My scratch-build against F20 also did not include translated
>> manpages:
>>
http://koji.fedoraproject.org/koji/taskinfo?taskID=5488765
>>
>> Something broke somewhere, I assume. Let's track this down before
>> we push these patches, please.
>
> The version of the Koji build shows that you tested with the
> previous patches.
>
> How exactly did you create the SRPM? Does the same sequence work
> without the patches?
>
> Did you create the SRPM that was rebuilt in koji using a parallel
> build or an in-tree build? A parallel build didn't work for me
> either most probably because po4a couldn't find the strings:
>
Ahh, ok. Yes, I generated the SRPM in a parallel tree. So we need to
figure out eventually why 'make distdir' isn't pulling the
translations in properly. Let's open a separate bug for that.
For now: Ack to these patches.
Thank you for the review. I filed