Bug 820544

The freerdp API is immature and probably not very stable. I assume the build requirement must be versioned to something that prevent freerdp < 1.0. The next major version of freerdp will probably change a lot too, so I guess the requirement also should be < 1.1 somehow.

Added the change, thanks.

Let me take this review.

Why is there the following :
BuildRequires:  libguac-devel%{?_isa} = %{version}
BuildRequires:  cairo-devel%{?_isa}, freerdp-devel%{?_isa} < 1.1


srpm are not rebuildable on x86_64 if built on 32 bits. I think that's to avoid.
( I would also place each BuildRequires on 1 line, as this permit easier patch review ).

(In reply to comment #4)
> Why is there the following :
> BuildRequires:  libguac-devel%{?_isa} = %{version}
> BuildRequires:  cairo-devel%{?_isa}, freerdp-devel%{?_isa} < 1.1
> 
> srpm are not rebuildable on x86_64 if built on 32 bits. I think that's to
> avoid.

i686 devel packages can be installed on x86_64 machines but do obviously not provide what is needed to build x86_64 packages.

"Normal" users will obviously not see this ... but those who build packages are rarely normal. I prefer being explicit in the requirements - but you are making the official review so it is your call ;-)

I hereby officially change my mind. While explicit requires makes the build more reliable it also put the build host architecture into the data used by yum-builddep which thus will fail on other architectures.

yum-builddep with rely on the arch on which it is run, not the arch on which the srpm was built.  Assuming identical package availability in yum, if I build srpm on one arch, and then yum-builddep it on another, everything will work as intended.  The arch-specific BRs are good.

(In reply to comment #7)

Are you 100% sure? Obvioulsy _some_ macro expansion happens on srpm build time. I verified that rpm -q --requires on a srpm with isa buildrequires has foo-devel(x86-32). yum-builddep on that srpm on x86_64 pulls in the i686 package.

Mock, and fedora-review, failed to build, but it seems to be due to missing libguac-devel. I will wait until it can be installed.

(In reply to comment #5)
> (In reply to comment #4)
> > Why is there the following :
> > BuildRequires:  libguac-devel%{?_isa} = %{version}
> > BuildRequires:  cairo-devel%{?_isa}, freerdp-devel%{?_isa} < 1.1

IMO, isas in these BR's are not useful and would recommend to remove them.

(In reply to comment #9)
> Mock, and fedora-review, failed to build, but it seems to be due to missing
> libguac-devel. I will wait until it can be installed.

You can use the build from here:

http://koji.fedoraproject.org/koji/packageinfo?packageID=13907

I'm not pushing the library alone to the updates unless all the pieces of the stack have passed review.

(In reply to comment #10)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Why is there the following :
> > > BuildRequires:  libguac-devel%{?_isa} = %{version}
> > > BuildRequires:  cairo-devel%{?_isa}, freerdp-devel%{?_isa} < 1.1
> 
> IMO, isas in these BR's are not useful and would recommend to remove them.

Depending on who's doing the review I got mixed results regarding the ISA'ed BRs. Some time is necessary to get the package approved, in some cases it's the opposite.

Since libguac has already been approved with ISA'ed BRs and there's no difference in building I would leave them in the spec file for uniformity with all the pieces of the stack.

Spec URL: http://slaanesh.fedorapeople.org/libguac-client-rdp.spec
SRPM URL: http://slaanesh.fedorapeople.org/libguac-client-rdp-0.6.0-2.fc17.src.rpm

Small spec file changes for conformity with libguac-client-vnc review.

(In reply to comment #11)
> I'm not pushing the library alone to the updates unless all the pieces of
> the stack have passed review.

That's not how Fedora packaging works and not helpful.

Maybe your sponsor should have a serious talk with you.

(In reply to comment #14)
> (In reply to comment #11)
> > I'm not pushing the library alone to the updates unless all the pieces of
> > the stack have passed review.
> 
> That's not how Fedora packaging works and not helpful.
> 
> Maybe your sponsor should have a serious talk with you.

It's funny, because depending on who I talk to I got completely opposing comments. When begging for reviews I've been said "hell no" until all the reviews for all the packages in the stack were in bugzilla.

If I push the library to the updates we'll have an orphaned library that nobody is using and if a later piece of the stack is blocked permanently for some reason the library is going to stay orphan.

If I remember correctly one the policies says that there should not be libraries in the distribution that nobody is using.

Anyway, if you need to run the build with the official updates and nobody is opposing I can push it to the repositories.

I've checked the _isa debate:

- https://fedoraproject.org/wiki/Packaging:ReviewGuidelines

"MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name}%{?_isa} = %{version}-%{release} [21]"


- https://fedoraproject.org/wiki/PackagingDrafts/DraftsTodo

"Use %{_isa} to make hardcoded Requires Arch Specific"


Additional info:

- https://fedoraproject.org/wiki/PackagingDrafts/ArchSpecificRequires
- http://www.rpm.org/wiki/PackagerDocs/ArchDependencies

So I assume those are to be kept in the spec file.

Thanks,
--Simone

I think I do not have the same understanding of the guideline as you do. That's not a Requires on base package, that's a BuildRequires. 

For the package, you are right that you should not add package that are not used, but the goal is to use the library once the reviews are finished, so this can be sent. Do not fear to push things, we can always remove if there is problem later.

Ok, pushed to updates-testing. Should I request "stable" directly or is "updates-testing" enough?

Thanks,
--Simone

Added more explicit files list as requested in other subpackage libguac-client-vnc (approved):

Spec URL: http://slaanesh.fedorapeople.org/libguac-client-rdp.spec
SRPM URL: http://slaanesh.fedorapeople.org/libguac-client-rdp-0.6.0-3.fc17.src.rpm

Regards,
--Simone

(In reply to comment #17)
> That's not a Requires on base package, that's a BuildRequires. 

Another note on _isa's:

The only difference I spot is when I build packages on x86_64 and have devel packages for i686 and x86_64 availables.

For example, I have installed libguac-devel.i686 and try to build libguac-client-rpd.x86_64 using rpmbuild. The dependency is satisfied, but the build fails as it should search for libguac-devel.x86_64.

From all the builds I've done (koji, rpmbuild, mock, 32 / 64 bit) where the arch is set at build time I see absolutely no difference in adding the _isa tag or not.

(In reply to comment #20)
> The only difference I spot is when I build packages on x86_64 and have devel
> packages for i686 and x86_64 availables.

Yes, that is exactly the case where isa'ed BRs can save a lot of confusion and prevent incorrect builds.

> From all the builds I've done (koji, rpmbuild, mock, 32 / 64 bit) where the
> arch is set at build time I see absolutely no difference in adding the _isa
> tag or not.

yum-builddep on a srpm built on x86_64 will however fail on i686 if it has isa'ed BRs.

Ok, so I assume it is good to leave them in place as this will lead to the expected behaviour.

Updated package with a separate %{_libdir}/guacamole/ just for plugins; this makes a lot of sense especially by the fact that the project is planning to add additional plugins like Spice, etc.

Requires libguac-0.6.0-6 that is currently building in Koji and will be pushed to updates-testing.

Spec URL: http://slaanesh.fedorapeople.org/libguac-client-rdp.spec
SRPM URL: http://slaanesh.fedorapeople.org/libguac-client-rdp-0.6.0-4.fc17.src.rpm

See bug #820543

Pushed the libguac build into updates-testing (fc17, fc16, el6), now it's available also outside of rawhide.

Any chance to see this moving forward? It's the only C piece of the stack that's missing from being approved.

All the other packages are in rawhide and updates-testing (f16/f17/el6) now.

Spec URL: http://slaanesh.fedorapeople.org/libguac-client-rdp.spec
SRPM URL: http://slaanesh.fedorapeople.org/libguac-client-rdp-0.6.0-5.fc17.src.rpm

- Added requirement on freerdp-plugins so the clipboard works as expected.

Thanks,
--Simone

I am unfortunately quite busy during the week with $DAYJOB, while try to finish the review tomorow

Thanks!
--Simone

So for some reason, i am still unable to build it in mock ( despites cleaning cache and so on ), and even if i see the package in rawhide. I am attempting a scratch build as a try.

So it fail on 64 bits :
http://koji.fedoraproject.org/koji/taskinfo?taskID=4120336

that's weird, cause i see the package in koji :/

Hello,

1) koji:

If you use koji you should build for rawhide, for all the other released distributions you should use buildroot overrides:

http://fedoraproject.org/wiki/Bodhi/BuildRootOverrides

Packages dependent one on the other are built and released all at the same time based on builds generated from build overrides.

In this case, you should create a root with libguac-0.6.0-6.fc17:

bodhi --buildroot-override=libguac-0.6.0-6.fc17 --notes="Test for review"

If you login to Bodhi you can see all the BuildRootOverrides that are used for the various stacks.

2) mock:

Based on your first request I also created testing updates for all the other parts, so if you want to use mock, for evertyhing except rawhide edit your /etc/mock/fedora-17-x86_64.cfg file adding the updates-testing repository.

Or install directly libguac / libguac-devel in the chroot.

---

That's the point of not putting updates before the whole stack is built: when you have all the stack at the same time to be updated, you create buildroots, build everything and then release all the packages that are dependent on each other at the same time so you don't break dependencies for the users.

Look for Evolution inside Bodhi, that's a clear example of that. You can't release an updated Evolution unless all the packages / plugins depending on it are built.

Well, you pushed them in updates-testing, so shouldn't they be in f17-candidate ( I am not that familliar with bodhi koji ) ?

And no, since there is no package existing currently, nothing would be broken if they are pushed to updates-testing.

Hello,

updates is not "f17-candidate", that's a tag in koji. Only in rawhide updates are pushed into the repositories, all other releases need staging. The packages are in "updates-testing" as you requested.

To test the build, please use one of the following:

1- rawhide:

koji, mock.

2- f17, f16, f15, el6:

koji + buildroot ovverrides OR mock + updates-testing enabled

--Simone

Any news? Have you tried to build the package?

Hello, another week has passed, any chance to see this progress forward? You took the review on the 25th of May.

btw, all the other Guacamole packages are now pushed to stable.

--Simone

I do not have time right now to make this go further, so better to let someone else do it, sorry for the delay.

* _isa in BuildRequires are insane. BuildRequires become the src.rpm's Requires.


> Requires:       freerdp-plugins < 1.1

Please add a comment to such explicit Requires. You can copy what you added to the %changelog:
https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires


* The following lead to some head-scratching:

> %post -p /sbin/ldconfig
> 
> %postun -p /sbin/ldconfig

> %files
...
> %{_libdir}/guacamole/%{name}.so
> %{_libdir}/guacamole/%{name}.so.*

So, some shared libs are stored in a private non-default path that is not in default search path for shared libs. ldconfig is run nevertheless. This prompted me to take a look at the libguac package. That one creates this private directory and extends the run-time linker's search path list.

Why? The libguac spec file doesn't answer the question.

The libguac source code dlopen's these modules. They are named "libguac-client-$(PROTOCOL).so". That's not a naming scheme that asks for trouble. The libs could be stored in %_libdir without much risk of causing file conflicts.

A comment in the spec file that %{_libdir}/guacamole/ is provided by libguac which is an automatic dependency would be nice, too.

Hello,

thanks for the comments.

(In reply to comment #36)
> * _isa in BuildRequires are insane. BuildRequires become the src.rpm's
> Requires.

And that's correct, as if I install a 64 bits package I expect to pull in 64 bit dependencies. Please read from comment #4 of this bug.

> > Requires:       freerdp-plugins < 1.1
> 
> Please add a comment to such explicit Requires. You can copy what you added
> to the %changelog:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

It's explained in comment #1, I will add a note, then.

> * The following lead to some head-scratching:
> 
> > %post -p /sbin/ldconfig
> > 
> > %postun -p /sbin/ldconfig

Yes, this is probably a leftover, I will remove it.
 
> > %files
> ...
> > %{_libdir}/guacamole/%{name}.so
> > %{_libdir}/guacamole/%{name}.so.*
> 
> So, some shared libs are stored in a private non-default path that is not in
> default search path for shared libs. ldconfig is run nevertheless. This
> prompted me to take a look at the libguac package. That one creates this
> private directory and extends the run-time linker's search path list.
> 
> Why? The libguac spec file doesn't answer the question.
> 
> The libguac source code dlopen's these modules. They are named
> "libguac-client-$(PROTOCOL).so". That's not a naming scheme that asks for
> trouble. The libs could be stored in %_libdir without much risk of causing
> file conflicts.
> 
> A comment in the spec file that %{_libdir}/guacamole/ is provided by libguac
> which is an automatic dependency would be nice, too.

They were like you're saying before I was asked to do the opposite for libguac-client-vnc. Please see comment #8 of [1] for the explanation. Developers will add other plugins to this (Spice, SSH, etc,).

[1] https://bugzilla.redhat.com/show_bug.cgi?id=820543

I will add a note for this as well, thanks.

--Simone

Can you post current links?

> And that's correct, as if I install a 64 bits package I expect
> to pull in 64 bit dependencies. Please read from comment #4 of this bug.

No, not again. :-/

It's disappointing you've not even thought twice before disagreeing with me.

A src.rpm package is arch-independent (!), which means it is built on an arbitrary build server using an arbitrary arch, and the single src.rpm is published in a _single_ package repository.

You can easily see the result of your packaging mistake if you downloaded the src.rpm of your own libguac-client-vnc package, for example:

http://dl.fedoraproject.org/pub/fedora/linux/updates/17/SRPMS/libguac-client-vnc-0.6.0-4.fc17.src.rpm

Note that's _the_ single SRPMS repo for all archs!

$ rpm -qpR libguac-client-vnc-0.6.0-4.fc17.src.rpm 
cairo-devel(x86-64)  
gnutls-devel(x86-64)  
libguac-devel(x86-64) = 0.6.0
libvncserver-devel(x86-64)  
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(CompressedFileNames) <= 3.0.4-1

Ooops! This is completely wrong for i686. Package resolvers will fail trying to download x86_64 packages which are not available for i686. One would first need to rebuild the src.rpm, which is highly recommended anyway, because there might be conditional (arch-dependent!) BuildRequires in the spec file, which only become active build requirements if the src.rpm is built on the correct target arch.


> > %post -p /sbin/ldconfig
> > 
> > %postun -p /sbin/ldconfig
>
> Yes, this is probably a leftover, I will remove it.

No, they are needed. How else would dlopen find the libs? Could it be that you just got lost due to lack of comments in your package?


> https://bugzilla.redhat.com/show_bug.cgi?id=820543#c8

That comment is misguided, unfortunately. There is a major difference between "hiding shared libs in a subdir of %_libdir" (and effectively hiding it from the run-time linker) and "moving shared libs into a subdir of %_libdir plus adding that subdir to the run-time linker's search path". Do you realize what the limited gain is when you do that?

Michael's comments in #39 seconded.

Hi guys,

I support Michael's words about the not-no-private-libs. 

Keep libs really private (apps might need patching) or let the default location be ok. No using in moving for the sake of it.

The use of %{__isa} is solving a problem, however creating a new one. 

I have lots a real life experience with the wrong arch devel package is installed issue, the archness of src package seems more a theoretical
problem. 

I have never used yum-builddep, mock/koji is better for such
operations any way?

Some comments are always good, when a stack has deps between packages
a common packager has no problem remembering how and why. However, Fedora
is large project and packages change maintainers all the time. Please
try to keep packages simple and easy to maintain for the next maintainer.


Btw: the package seems good outside this issues.

> It's disappointing you've not even thought twice before disagreeing with me.

Sorry I misunderstood your "BuildRequires become the src.rpm's Requires." I removed the "src." in my mind.

Spec URL: http://slaanesh.fedorapeople.org/libguac-client-rdp.spec
SRPM URL: http://slaanesh.fedorapeople.org/libguac-client-rdp-0.6.0-6.fc17.src.rpm

- Moved back plugins to _libdir.
- Removed _isaed build requirements.
- Added comment on Explicit BuildRequires.

I removed the previously introduced changes, I prefer not to patch things if it's not really needed and try to keep it simple; so I would go to the default _libdir.

Since all the other packages are new I can push the changes back to the other libraries, no problem:

http://pkgs.fedoraproject.org/gitweb/?p=libguac.git;a=summary
http://pkgs.fedoraproject.org/gitweb/?p=libguac-client-vnc.git;a=summary

Probably the "< 1.1" could be removed as well, if the API changes there would be a soname bump and compilation errors which are part of the normal building and testing.

(In reply to comment #39)
> Ooops! This is completely wrong for i686. Package resolvers will fail trying
> to download x86_64 packages which are not available for i686. One would
> first need to rebuild the src.rpm, which is highly recommended anyway,
> because there might be conditional (arch-dependent!) BuildRequires in the
> spec file, which only become active build requirements if the src.rpm is
> built on the correct target arch.

As you point out: The src.rpm dependencies (and thus yum-builddep) are not reliable anyway. It is for example (usually) not possible to do 'yum-builddep grub2' on i686. (IMO it would be better to deprecate yum-builddep and provide a simple way to install build dependencies from rpmbuild/fedpkg, using locally and correctly exapanded macros.)

It do however IMO matter that build requirements are 100% reliable that if just the stated requirements are met somehow then we can trust that the build will succeed and be valid. A unspecific requirement can be ambiguous and fatal on multilib systems. IMO it is thus better to make the arch requirement explicit when it matters.

But right, this is not how we do in Fedora where multilib has several grey areas ... and this is thus not the rigth place to discuss it.

> The src.rpm dependencies (and thus yum-builddep) are not reliable anyway.

Remains the question whether that is sufficient reason to break "rpm -qpR …src.rpm" queries and tools such as yum-builddep? Perhaps the RPM devs would also like to kill src.rpm Requires that are created from the BuildRequires.


> A unspecific requirement can be ambiguous and fatal on multilib systems.

... for cross-arch building only (such as i686 on x86_64 *without* using Mock), and mainly because not all -devel subpackages contain arch-specific base package dependencies yet (despite the added entry in the Packaging Guidelines).

The bad effects are not limited to yum-builddep, however. A manual "yum install 'gnutls-devel(x86-32)'"  currently still pulls in a mix of i686/x86_64 packages, for example. And that's just one example. One doesn't even get the minimum build environment via arch-specific dependencies alone. No glibc-devel.i686 and similar packages.


> ... and this is thus not the rigth place to discuss it.

:) That would be something for the FPC.

It would be half-hearted anyway that arch-specific base pkg deps are a MUST in the guidelines, but packagers don't update their spec files accordingly or even refuse to do so because they disagree with the guidelines. Not even glibc-devel contains an arch-specific base pkg dep yet.


> multilib has several grey areas 

This topic is not limited to "multilib".

I understand the intention to add %_isa to BuildRequires, but it's of questionable benefit and not free of problems either.

Good:

- rpmlint checks return:

libguac-client-rdp.src: W: spelling-error Summary(en_US) guacd -> guard
The value of this tag appears to be misspelled. Please double-check.

libguac-client-rdp.src: W: spelling-error %description -l en_US guacd -> guard
The value of this tag appears to be misspelled. Please double-check.

1 packages and 0 specfiles checked; 0 errors, 2 warnings.

Ignore.

libguac-client-rdp.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libguac-client-rdp.so
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

Is that really a plugin?  The presence of a solib and an include directory with headers makes me think a -devel package might be warranted.

- package meets naming guidelines
- package meets packaging guidelines
- license ( MPLv1.1 or GPLv2+ or LGPLv2+ ) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file 

Other than my questions about the need for a -devel package, it looks OK.

> Is that really a plugin?  

Yes, libguac dlopen()s these shared libs (bottom of comment 36). That's why a comment in the spec file would be helpful.


> The presence of a solib and an include directory with headers
> makes me think a -devel package might be warranted.

? The spec file doesn't package any headers.

(In reply to comment #46)
> > Is that really a plugin?  
> 
> Yes, libguac dlopen()s these shared libs (bottom of comment 36). That's why
> a comment in the spec file would be helpful.

Gotcha.  That would be great.

> 
> > The presence of a solib and an include directory with headers
> > makes me think a -devel package might be warranted.
> 
> ? The spec file doesn't package any headers.

Exactly.  But they're there in the tarball.  So I'm asking if possibly the headers should be packaged.

> But they're there in the tarball. So I'm asking if possibly the headers
> should be packaged.

Don't just ask, but try to come up with a more convincing rationale as why they should be packaged. ;)

The files in ./src include these headers. It looks like a normal split of C source code into .h and .c files, with no immediate reason to believe the headers define a _public_ interface.

Then there's no good reason to do so.  Usually those life in the same dir, but they don't need to.  So since that's resolved, put a comment in the spec about the dlopened plugin and I'm happy.

(In reply to comment #46)
> Yes, libguac dlopen()s these shared libs (bottom of comment 36). That's why
> a comment in the spec file would be helpful.

Yes, I forgot the comment, sorry. There should be no devel package for the plugins. I took part of your sentence. And put it in the %files section:

%files
%doc AUTHORS LICENSE README
# The libguac source code dlopen's these plugins, and they are named without
# the version in the shared object; i.e. "libguac-client-$(PROTOCOL).so".
%{_libdir}/%{name}.so
%{_libdir}/%{name}.so.*

Spec URL: http://slaanesh.fedorapeople.org/libguac-client-rdp.spec
SRPM URL: http://slaanesh.fedorapeople.org/libguac-client-rdp-0.6.0-7.fc17.src.rpm

I will add these and all the other changes to libguac-client-vnc and libguac after the review.

Thanks,
--Simone

Perfect, thanks!

APPROVED.

Applied the same changes to the other packages:

http://pkgs.fedoraproject.org/gitweb/?p=libguac-client-vnc.git;a=commitdiff;h=635e0a2f7944425a17772f43972bcb115cc2a579

http://pkgs.fedoraproject.org/gitweb/?p=libguac.git;a=commitdiff;h=b775e419dc0a30127d0433b2516830ede149c3a0

http://pkgs.fedoraproject.org/gitweb/?p=guacd.git;a=commitdiff;h=5f1b824a6b841d5c48c192ec6b76631de02ff138

Many thanks,
--Simone

New Package SCM Request
=======================
Package Name: libguac-client-rdp
Short Description: RDP support for guacd
Owners: slaanesh
Branches: f17 f16 el6
InitialCC:

Git done (by process-git-requests).