[Bug 1255179] Review Request: runc - CLI for running Open Containers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1255179



--- Comment #17 from Nalin Dahyabhai <nalin@xxxxxxxxxx> ---
(In reply to Jan Chaloupka from comment #15)
> > [!]: Package requires other packages for directories it uses.
> > Subdirectories under /usr/share/gocode that aren't owned by golang or the
> > opencontainer-specs package should be owned by this package so that its 
> > removal doesn't leave empties around.
> 
> Spec file updated. All directories are owned by devel subpackage. As user
> could update a devel subpackage without updating unit-test, both packages
> could end up owning the same directory. So no directories owned by unit-test.

Thanks!

> > [ ]: %build honors applicable compiler flags or justifies otherwise.
> >  Might want to add a comment about the -B flag here, for people like me who 
> > didn't know that it tells the go linker to embed a build ID note in the
> > binary.
> 
> Described in Packaging Draft [1].

Ah, okay, works for me.

> > [ ]: Changelog in prescribed format.
> >  It's customary to use a person's full name where the changelog currently
> > lists a Fedora account name.  It's not a blocker, but my guess is it's an
> > oversight.
> 
> Updated.

Great!

> > [ ]: Requires correct, justified where necessary.
> >  The runc-devel package appears to be missing a requires on 
> > "golang(github.com/opencontainers/specs)", which is imported by multiple
> > parts of libcontainer.
> 
> $ gofed ggi --all-occurrences --show-occurrence | grep
> github.com/opencontainers/specs
> github.com/opencontainers/specs (spec.go:main, restore.go:main,
> utils.go:main, run.go:main)
> 
> github.com/opencontainers/specs is used only in main packages. These are not
> to be imported by other packages.

Whoops, you're right - no idea what made me think that was used internally.

> > [ ]: Package complies to the Packaging Guidelines
> > The runc command should (eventually) have a manual page.
> 
> Definitely. 
> 
> > The package description should be more than just a copy of the summary.
> 
> At the moment I don't see any description that would be explaining enough
> what runc is. If you have an idea what description is suitable, please. I
> will update the spec file accordingly. 

Possibly "The runc command can be used to start containers which are packaged
in accordance with the Open Container Initiative's specifications, and to
manage containers running under runc.", or something that includes more of the
details from the web site or runc's --help output.

> > [ ]: Package successfully compiles and builds into binary rpms on at least
> >     one supported primary architecture.
> > Missing BuildRequires on "golang(github.com/opencontainers/specs)" (from bug
> > #1255370) and "golang(github.com/codegangsta/cli)".
> 
> Updated. Thanks. This is not included in the spec file generator. I will
> update the generator as well.

Great!

> > [ ]: Fully versioned dependency in subpackages if applicable.
> > Whether runc-devel or runc-unit-test should depend on the same version of
> > runc isn't really clear to me, since there aren't any dangling symlinks if
> > they don't.
> >     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in 
> >     runc-devel, runc-unit-test
> 
> Neither devel nor unit-test depends on runc.

Okay.  

> > [!]: Latest version is packaged.
> 
> By the time of generating the spec file, the commit was the latest one.

Understood.

> > [ ]: Spec use %global instead of %define unless justified.
> >  Why is copying() defined using %define rather than %global?
> 
> Because copying is a parametric macro which needs to be evaluated in the
> time of use. As a global, it would be evaluated in the time of definition
> end up with empty %license macro.

Are you sure that's necessary here?  When I try changing it, the files show up
in the right place with the right file flags in the binary packages.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review




[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]