Bug 1463492

I would be happy to sponsor Tomo and this package.

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...

(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!

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

(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.

You're welcome.

Also think about adding a man page for your command, see https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages

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.

> 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

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

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 .

(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/

Samuel, any chance you could look at the latest files?  Is there anything left to do on this review?  Thanks!

(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.

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/

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.

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?

(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

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.

(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?

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

(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?

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

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?

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

(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.

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.

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.

Hi, What is the state on this one?

It has been a while. Are you still interested in packaging koko?

Anthos,

Thank you to ping me again! Currently user seems to use download binary directly without rpm and I feel it could be natural in case of go-tool's user.
So I'm closing the bz.

Thank you again.