Bug 1411324 - Review Request: Skydive - a real time network analyzer
Summary: Review Request: Skydive - a real time network analyzer
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: RDO
Classification: Community
Component: Package Review
Version: unspecified
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
: trunk
Assignee: Matthias Runge
QA Contact: hguemar
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-01-09 13:30 UTC by Sylvain Baubeau
Modified: 2017-06-22 16:17 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-06-22 16:17:58 UTC
mrunge: rdo-review+


Attachments (Terms of Use)
Specfile with bundled libraries (34.55 KB, text/x-matlab)
2017-01-09 13:30 UTC, Sylvain Baubeau
no flags Details
Specfile with bundled libraries (29.53 KB, text/x-matlab)
2017-01-11 10:10 UTC, Sylvain Baubeau
no flags Details

Description Sylvain Baubeau 2017-01-09 13:30:16 UTC
Created attachment 1238772 [details]
Specfile with bundled libraries

=== Main package ===

== skydive ==

Spec URL: https://github.com/skydive-project/skydive/blob/master/contrib/packaging/rpm/skydive.spec
Copr repository: https://copr.fedorainfracloud.org/coprs/skydive/skydive/
Description: A real time network analyzer

Skydive is written in Go and uses "govendor" for dependencies.

A script (https://github.com/skydive-project/skydive/blob/master/contrib/packaging/rpm/generate-skydive-bootstrap.sh) is used to create the tarball with:
- the sources (from Git)
- some generated Go files (Go files generated from protobuf, Go files generated to serve static data)
- the dependencies (retrieved by govendor and put into the vendor/ directory)

The script also generates a spec file where the bundled libraries are listed using 'Provides: bundles(golang(github.com/user/library)) = SHA1'. An example of a generated spec file is provided as attachment to this bug.

Comment 1 Matthias Runge 2017-01-10 14:36:29 UTC
A few remarks:
- I see a lot of bundled golang stuff here, at least some of it is already packaged for Fedora. 
- which spec file should I check? there are now two in this review request?
- the builders do NOT have internet access. thus the spec from skydive-project itself can't work. ( https://github.com/skydive-project/skydive/blob/master/contrib/packaging/rpm/skydive.spec#L77 )
- %{openvswitch_version} is undefined
- there's no need to clean the buiuldroot before compiling
- is it correct that both agent and analyzer don't have any files, other than a service file?
- the lines https://github.com/skydive-project/skydive/blob/master/contrib/packaging/rpm/skydive.spec#L261-L274 are more what we'd expect for changelog. The upper part of changelog is probably a bit detailed and should be moved to a changelog file.

Comment 2 Sylvain Baubeau 2017-01-10 15:30:41 UTC
Thanks for the review.

> A few remarks:
> - I see a lot of bundled golang stuff here, at least some of it is already packaged for Fedora. 

The unit and integration tests are executed on our CI with the bundled versions. I could give the packaged versions provided by CentOS a try, but I'm afraid we don't have enough bandwidth to maintain the package for 2 Fedora versions and CentOS.

> - which spec file should I check? there are now two in this review request?

Sorry, that was not clear. The one to review is the one attached to the bug. The github version will be updated with the remarks of this review.

> - the builders do NOT have internet access. thus the spec from skydive-project itself can't work. ( https://github.com/skydive-project/skydive/blob/master/contrib/packaging/rpm/skydive.spec#L77 )

"github.com/kardianos/govendor" is provided in the tarball, therefore internet access is not required (our Copr repository builds the RPM without Internet connectiviy).

> - %{openvswitch_version} is undefined

Indeed. I will fix this.

> - there's no need to clean the buiuldroot before compiling

Fixed.

> - is it correct that both agent and analyzer don't have any files, other than a service file?

Correct. There is only one binary for both agent and analyzer.

> - the lines https://github.com/skydive-project/skydive/blob/master/contrib/packaging/rpm/skydive.spec#L261-L274 are more what we'd expect for changelog. The upper part of changelog is probably a bit detailed and should be moved to a changelog file.

This is already in a separate CHANGELOG file in the repository and it's copy/pasted into the specfile. But I'm perfectly fine with keeping only the packaging aspect of the changelog in the specfile.

Comment 3 Sylvain Baubeau 2017-01-11 10:10:37 UTC
Created attachment 1239400 [details]
Specfile with bundled libraries

Comment 4 Matthias Runge 2017-01-12 08:01:06 UTC
looks fine now to me.

License is ASL 2.0, which matches both SPEC and license in the package.

Package approved.


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