Bug 1463492

Summary: Review Request: koko - container connector
Product: [Fedora] Fedora Reporter: s1061123
Component: Package ReviewAssignee: Athos Ribeiro <athoscribeiro>
Status: ASSIGNED --- QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: athoscribeiro, dcbw, jchaloup, package-review, samuel.rakitnican
Target Milestone: ---Flags: athoscribeiro: fedora‑review?
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On: 1484795    
Bug Blocks: 177841    

Description s1061123 2017-06-21 02:12:28 EDT
Spec URL: http://s1061123.net/koko/koko_v03.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/s1061123/koko/fedora-rawhide-x86_64/00568652-koko/koko-0.3-1.src.rpm
copr build URL: https://copr.fedorainfracloud.org/coprs/s1061123/koko/build/568652/
Description: koko is a simple tool which connects between Docker containers/linux netns processes with veth devices/vxlan of linux kernel. Using koko, you can simply make point-to-point connection for containers without linux bridges.
Fedora Account System Username:s1061123

Hi folks, this is my first package hence I am also looking for a sponsor.
I would appreciate it if someone could review it and be a sponsor.

Thanks,
Tomo
Comment 1 Dan Williams 2017-06-30 10:43:43 EDT
I would be happy to sponsor Tomo and this package.
Comment 2 Dan Williams 2017-06-30 10:51:38 EDT
At least I'm happy if I'm eligeable to sponsor, it's been a while so I can't recall whether I'm a sponsor or not.  If i'm not, please disregard...
Comment 3 s1061123 2017-06-30 15:37:18 EDT
(In reply to Dan Williams from comment #2)
> At least I'm happy if I'm eligeable to sponsor, it's been a while so I can't
> recall whether I'm a sponsor or not.  If i'm not, please disregard...

Thank you for your volunteer, Dan!
Comment 4 srakitnican 2017-07-08 06:47:13 EDT
Just a few comments:

1. Missing changelog, see: https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
2. The %defattr directive should not be used for default values, see: https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions
3. Why do you need git for building?
4. Empty and possibly wrongly located %post can be removed
Comment 5 s1061123 2017-07-09 09:42:06 EDT
(In reply to srakitnican from comment #4)
> Just a few comments:
> 
> 1. Missing changelog, see:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
> 2. The %defattr directive should not be used for default values, see:
> https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions
> 3. Why do you need git for building?
> 4. Empty and possibly wrongly located %post can be removed

Hi srakitnican,

Thank you for your comments!
I am going to take care of all your comments and to test it.
I will update the .spec and .SRPM after test completion.
Comment 6 srakitnican 2017-07-09 11:12:59 EDT
You're welcome.

Also think about adding a man page for your command, see https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages
Comment 7 srakitnican 2017-07-09 11:32:32 EDT
One more, you must include license file with the package if present with the sources, see https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#License_Text

Just add %license LICENSE to %files section.
Comment 8 srakitnican 2017-07-09 15:29:35 EDT
> Also, please make sure that there are no lines in the description longer than 80 characters.

https://fedoraproject.org/wiki/Packaging:Guidelines#Summary_and_description
Comment 9 s1061123 2017-07-10 01:38:47 EDT
Hi srakitnican,

Thank you for your review again!

Here is the modified spec/SRPM, all comments above are taken care of it.
BTW, I need to keep 'git' in its 'BuildRequires:' because 'go get' invokes
git command internally, hence build is failed without git command.

----
Spec URL: http://s1061123.net/koko/koko_v04.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/s1061123/koko/fedora-rawhide-x86_64/00577370-koko/koko-0.4-1.src.rpm
copr build URL: https://copr.fedorainfracloud.org/coprs/s1061123/koko/build/577370/
Description: koko is a simple tool which connects between Docker containers/linux netns processes with veth devices/vxlan of linux kernel. Using koko, you can simply make point-to-point connection for containers without linux bridges.
Fedora Account System Username:s1061123
Comment 10 srakitnican 2017-07-10 04:54:06 EDT
No sure about (In reply to s1061123 from comment #9)
> Hi srakitnican,
> 
> Thank you for your review again!
> 
> Here is the modified spec/SRPM, all comments above are taken care of it.
> BTW, I need to keep 'git' in its 'BuildRequires:' because 'go get' invokes
> git command internally, hence build is failed without git command.

I guess the question is now why you are doing "go get", don't you have the sources already available from a tarball? Either way, I don't know much about go, will leave this issue for someone else.


Small note, with your new spec file rpmlint complains about macro in the changelog. You can disable a macro with %, for example %%post .
Comment 11 s1061123 2017-07-10 05:37:26 EDT
(In reply to srakitnican from comment #10)
> No sure about (In reply to s1061123 from comment #9)
> > Hi srakitnican,
> > 
> > Thank you for your review again!
> > 
> > Here is the modified spec/SRPM, all comments above are taken care of it.
> > BTW, I need to keep 'git' in its 'BuildRequires:' because 'go get' invokes
> > git command internally, hence build is failed without git command.
> 
> I guess the question is now why you are doing "go get", don't you have the
> sources already available from a tarball? Either way, I don't know much
> about go, will leave this issue for someone else.
sure. let's wait other opinion.
 
> Small note, with your new spec file rpmlint complains about macro in the
> changelog. You can disable a macro with %, for example %%post .
I've taken care of it and upload it.

Spec URL: http://s1061123.net/koko/koko_v04.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/s1061123/koko/fedora-rawhide-x86_64/00577438-koko/koko-0.4-1.src.rpm
copr build URL: https://copr.fedorainfracloud.org/coprs/s1061123/koko/build/577438/
Comment 12 Dan Williams 2017-08-15 17:29:17 EDT
Samuel, any chance you could look at the latest files?  Is there anything left to do on this review?  Thanks!
Comment 13 srakitnican 2017-08-16 01:51:20 EDT
(In reply to Dan Williams from comment #12)
> Samuel, any chance you could look at the latest files?  Is there anything
> left to do on this review?  Thanks!

In my opinion format of the spec file looks good now, it is waiting for someone who knows a bit more about go to review that part, as I am not sure how this should be done.
Comment 14 s1061123 2017-08-16 04:08:35 EDT
Hi Dan and Samuel,

Thank you for your clarification about the review!

BTW, upstream releases newer version, hence I upload spec files.
I only modify version in spec file.
   
Spec URL: http://s1061123.net/koko/koko_v05.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/s1061123/koko/fedora-rawhide-x86_64/00590884-koko/koko-0.5-1.src.rpm
copr build URL: https://copr.fedorainfracloud.org/coprs/s1061123/koko/build/590884/
Comment 15 srakitnican 2017-08-16 05:04:39 EDT
The newer SPEC file doesn't have changelog entry for the new package release 0.5-1 . 


(In reply to Dan Williams from comment #12)
> Samuel, any chance you could look at the latest files?  Is there anything
> left to do on this review?  Thanks!

The major concern I have is that build system is trying to connect to download some stuff. No package should do that, I think Go is no exception to that. Trying to build in mock gives following.

+ export GOPATH=/builddir/build/BUILD/koko-0.5/_build:/usr/share/gocode
+ GOPATH=/builddir/build/BUILD/koko-0.5/_build:/usr/share/gocode
+ cd ./_build/src/github.com/redhat-nfvpe/koko
+ go get .
# cd .; git clone https://github.com/MakeNowJust/heredoc /builddir/build/BUILD/koko-0.5/_build/src/github.com/MakeNowJust/heredoc
Cloning into '/builddir/build/BUILD/koko-0.5/_build/src/github.com/MakeNowJust/heredoc'...
fatal: unable to access 'https://github.com/MakeNowJust/heredoc/': Could not resolve host: github.com
package github.com/MakeNowJust/heredoc: exit status 128
# cd .; git clone https://github.com/mattn/go-getopt /builddir/build/BUILD/koko-0.5/_build/src/github.com/mattn/go-getopt
Cloning into '/builddir/build/BUILD/koko-0.5/_build/src/github.com/mattn/go-getopt'...
fatal: unable to access 'https://github.com/mattn/go-getopt/': Could not resolve host: github.com
package github.com/mattn/go-getopt: exit status 128
# cd .; git clone https://github.com/containernetworking/plugins /builddir/build/BUILD/koko-0.5/_build/src/github.com/containernetworking/plugins
Cloning into '/builddir/build/BUILD/koko-0.5/_build/src/github.com/containernetworking/plugins'...
fatal: unable to access 'https://github.com/containernetworking/plugins/': Could not resolve host: github.com
package github.com/containernetworking/plugins/pkg/ns: exit status 128
# cd .; git clone https://github.com/docker/docker /builddir/build/BUILD/koko-0.5/_build/src/github.com/docker/docker
Cloning into '/builddir/build/BUILD/koko-0.5/_build/src/github.com/docker/docker'...
fatal: unable to access 'https://github.com/docker/docker/': Could not resolve host: github.com
package github.com/docker/docker/client: exit status 128
# cd .; git clone https://github.com/vishvananda/netlink /builddir/build/BUILD/koko-0.5/_build/src/github.com/vishvananda/netlink
Cloning into '/builddir/build/BUILD/koko-0.5/_build/src/github.com/vishvananda/netlink'...
fatal: unable to access 'https://github.com/vishvananda/netlink/': Could not resolve host: github.com
package github.com/vishvananda/netlink: exit status 128
package golang.org/x/net/context: unrecognized import path "golang.org/x/net/context" (https fetch: Get https://golang.org/x/net/context?go-get=1: http: error connecting to proxy http://192.168.1.20:3128: dial tcp 192.168.1.20:3128: connect: network is unreachable)


RPM build errors:
error: Bad exit status from /var/tmp/rpm-tmp.DiTGyX (%build)
    Bad exit status from /var/tmp/rpm-tmp.DiTGyX (%build)
ERROR: Exception(/tmp/koko-0.5-1.src.rpm) Config(fedora-25-x86_64) 8 minutes 52 seconds
INFO: Results and/or logs in: /var/lib/mock/fedora-25-x86_64/result
ERROR: Command failed: 
 # /usr/bin/systemd-nspawn -q -M c63079181aa34148b2ac4a6345bb189c -D /var/lib/mock/fedora-25-x86_64/root -a --private-network --setenv=TERM=vt100 --setenv=HOSTNAME=mock --setenv=SHELL=/bin/bash --setenv=LANG=en_US.UTF-8 --setenv=PATH=/usr/bin:/bin:/usr/sbin:/sbin --setenv=http_proxy=http://192.168.1.20:3128 --setenv=PS1=<mock-chroot> \s-\v\$  --setenv=HOME=/builddir --setenv=PROMPT_COMMAND=printf "\033]0;<mock-chroot>\007" -u mockbuild bash --login -c /usr/bin/rpmbuild -bb --target x86_64 --nodeps /builddir/build/SPECS/koko.spec


Also rpmlint output:

$ rpmlint /tmp/koko-0.5-1.src.rpm 
koko.src: W: summary-not-capitalized C koko, container connector
koko.src: W: name-repeated-in-summary C koko
koko.src: W: spelling-error %description -l en_US netns -> nets, net's
koko.src: W: spelling-error %description -l en_US veth -> vet, vetch, vets
koko.src: W: spelling-error %description -l en_US vxlan -> vanilla
koko.src: E: unknown-key RSA#2ec405fb (MD5
1 packages and 0 specfiles checked; 1 errors, 5 warnings.

First two could be fixed easily by omitting the name in summary, and capitalizing the first character in sentence. For the 3rd, 4th and 5th please double check the spellings.
Comment 16 s1061123 2017-08-16 07:16:57 EDT
Samuel,

Thank you for your review! Regarding code download from internet, I will take care of it soon.
But I'm wondering that lint error because all terms are correct words, not wrong.
1st warning can be workaround with removal name, koko, however, removing the name from summary, it is not good.

Could you please let me know how to skip these warning?
Comment 17 srakitnican 2017-08-16 07:38:12 EDT
(In reply to s1061123 from comment #16)
> Samuel,
> 
> Thank you for your review! Regarding code download from internet, I will
> take care of it soon.
> But I'm wondering that lint error because all terms are correct words, not
> wrong.
> 1st warning can be workaround with removal name, koko, however, removing the
> name from summary, it is not good.
> 
> Could you please let me know how to skip these warning?

They are just warnings, automated checks. It is not always right but nevertheless worth double checking.

For the summary, how about rephrasing it a bit or using some other description like: "Connect containers as point-to-point connection".


Also for the sources you could use slightly different URL to give more concise filename at the end instead of just a version. See https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL#Git_Tags
Comment 18 s1061123 2017-08-16 10:34:42 EDT
Samuel, thank you for your review! I've taken care of it and upload it.

Spec URL: http://s1061123.net/koko/koko_v051.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/s1061123/koko/fedora-rawhide-x86_64/00591013-koko/koko-0.51-1.src.rpm
copr build URL: https://copr.fedorainfracloud.org/coprs/s1061123/koko/build/591013/

W.r.t github tarball location, I don't change URL because it is default setting of github.
You can get tarball from URL written in this spec, hence it should be OK.
I also tried to get tarball from "https://github.com/OWNER/%{name}/archive/GIT-TAG/%{name}-%{version}.tar.gz" but it failed. Just FYI.
Comment 19 s1061123 2017-08-16 11:52:52 EDT
(In reply to s1061123 from comment #18)
> Samuel, thank you for your review! I've taken care of it and upload it.
(snip)
> W.r.t github tarball location, I don't change URL because it is default
> setting of github.
> You can get tarball from URL written in this spec, hence it should be OK.
> I also tried to get tarball from
> "https://github.com/OWNER/%{name}/archive/GIT-TAG/%{name}-%{version}.tar.gz"
> but it failed. Just FYI.

I changed above fedora's style of github URL in spec file, so all comments are taken care of.
Please take a look into it?
Comment 20 srakitnican 2017-08-16 12:05:01 EDT
This looks better, thank you.

Now it just needs someone who works with Go packages to go through and review all the bundled libraries, these should be de-bundled as much as possible, but as I understand this is not entirely doable with Go. This is the part where I am clueless about.

https://fedoraproject.org/wiki/PackagingDrafts/Go#Bundled_or_de-bundled
Comment 21 s1061123 2017-08-16 13:02:54 EDT
(In reply to srakitnican from comment #20)
> This looks better, thank you.
> 
> Now it just needs someone who works with Go packages to go through and
> review all the bundled libraries, these should be de-bundled as much as
> possible, but as I understand this is not entirely doable with Go. This is
> the part where I am clueless about.
> 
> https://fedoraproject.org/wiki/PackagingDrafts/Go#Bundled_or_de-bundled

My project uses up-to-date packages and some of them is not updated in Fedora, hence I would like to control it otherwise I cannot bring latest binary into Fedora.

BTW, who is the best person for verify? Could you please let me know who should be?
Comment 22 Athos Ribeiro 2017-08-16 14:12:31 EDT
Hello,

- You may want to check [1] for some guidelines on packaging go programs.

In special, you want to use ExclusiveArch:  %{go_arches} to make sure it only builds for the arches supported by Fedora and BuildRequires:  compiler(go-compilers). Then you can also use the %gobuild and %gotest macros.

- You are not supposed to download anything during your build (golang is not an exception).

- I'd recommend using gofed [1] to generate a first version of your package and start improving/cleaning from there.

- About bundling: I see different opinions on bundling project dependencies for golang projects, and I do understand how bad it is to handle all those dependencies on specific revisions. Fedora packaging guidelines are clear on that matter though: we should avoid it at all costs. Remember now we use pagure to maintain our spec files and you can always send pull requests to package maintainers to update those. Maybe some more experienced golang packager could give some input on this matter here, Jan, any inputs?

[1] https://fedoraproject.org/wiki/PackagingDrafts/Go
[2] https://github.com/ingvagabund/gofed
Comment 23 s1061123 2017-08-17 02:19:06 EDT
Hi Athos,

Thank you for your review and valuable comments!
I've update spec files as following. This is made by gofed from scratch.

Spec URL: http://s1061123.net/koko/koko_v051a.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/s1061123/koko/fedora-rawhide-x86_64/00591177-koko/koko-0.51-1.fc27.src.rpm
copr build URL: https://copr.fedorainfracloud.org/coprs/s1061123/koko/build/591177/

Could you please take a look into it?
Comment 24 Jan Chaloupka 2017-08-24 10:03:11 EDT
There is unwritten rule. If a project depends on Docker or Kubernetes,  docker/kubernetes dependency from the bundled dependencies. Currently, both Docker and Kubernetes are built from bundled deps due to rapid changes in their dependencies. I hope it will change in the future and we will be able to provide at least a subset of Docker, resp. Kubernetes source code rpms that can be [B]Red in spec files.

Another unwritten rule is: if you can debundle a dependency, do it.

In your case it will be a combination of both.
From bundled deps:
- github.com/docker/docker/client will be 
From Fedora packages:
- github.com/MakeNowJust/heredoc
- github.com/mattn/go-getopt
- github.com/containernetworking/plugins/pkg/ns
- github.com/vishvananda/netlink
- golang.org/x/net/context

- github.com/MakeNowJust/heredoc updated here: https://koji.fedoraproject.org/koji/buildinfo?buildID=960919
- github.com/mattn/go-getopt needs new spec file: https://bugzilla.redhat.com/1484795
- github.com/containernetworking/plugins PR here: https://src.fedoraproject.org/rpms/containernetworking-cni/pull-request/1
- github.com/vishvananda/netlink updated here: https://koji.fedoraproject.org/koji/packageinfo?packageID=19455
- golang.org/x/net/context updated here: https://koji.fedoraproject.org/koji/buildinfo?buildID=961022
Comment 25 srakitnican 2017-08-28 07:15:43 EDT
(In reply to s1061123 from comment #21)
> (In reply to srakitnican from comment #20)
> > This looks better, thank you.
> > 
> > Now it just needs someone who works with Go packages to go through and
> > review all the bundled libraries, these should be de-bundled as much as
> > possible, but as I understand this is not entirely doable with Go. This is
> > the part where I am clueless about.
> > 
> > https://fedoraproject.org/wiki/PackagingDrafts/Go#Bundled_or_de-bundled
> 
> My project uses up-to-date packages and some of them is not updated in
> Fedora, hence I would like to control it otherwise I cannot bring latest
> binary into Fedora.
> 
> BTW, who is the best person for verify? Could you please let me know who
> should be?

Thank you Athos and Jan. Seems you've got your answers, thus clearing needinfo request from me.
Comment 26 s1061123 2017-08-29 12:43:56 EDT
Thank you Jan for your comments and explanation about undocumented guideline because I can only read documented rules (I hope it will be documented soon).

Koko uses vendoring (i.e. bundling dependent packages) to avoid API mismatch between current package code in github and koko's code and no plan to remove it otherwise old code may failed to build in usual 'go get/build' command ('go get .' pulls latest code in github), for non-Fedora's go environment.

Hence I decide to wait:
 + all dependent packages are RPMed in Fedora and
 + all dependent packages are up-to-date enough to bulid koko, or
 + golang provides the way to remove vendoring safely with backward compatibility.

Without RPM package, user can easily install koko as 'go get' command hence
it does not block user to use koko, I hope.

Thank you, Jan, Samuel and Athos, again.

So I keep it and I am going to update it when it is ready.