Bug 1615641 - Review Request: compliance-masonry - Security Documentation Builder
Summary: Review Request: compliance-masonry - Security Documentation Builder
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 1609038 (view as bug list)
Depends On: 1614929 1615976 1615979 1615981 1616106
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2018-08-13 23:52 UTC by redhatrises
Modified: 2021-06-07 00:45 UTC (History)
5 users (show)

Fixed In Version:
Clone Of: 1609038
Environment:
Last Closed: 2021-06-07 00:45:18 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description redhatrises 2018-08-13 23:52:55 UTC
+++ This bug was initially created as a clone of Bug #1609038 +++

Spec URL: https://copr-be.cloud.fedoraproject.org/results/rga/compliance-masonry/fedora-rawhide-x86_64/00781200-compliance-masonry/compliance-masonry.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/rga/compliance-masonry/fedora-rawhide-x86_64/00781200-compliance-masonry/compliance-masonry-1.1.4-2.src.rpm

Koji Scratch build: https://koji.fedoraproject.org/koji/tasks?state=closed&owner=rga&view=tree&method=all&order=-id
Copr Builds: https://copr.fedorainfracloud.org/coprs/rga/compliance-masonry/monitor/

Description: Compliance Masonry is a command-line interface (CLI) that allows users to construct certification documentation using the OpenControl Schema.

Fedora Account System Username: rga

--- Additional comment from  on 2018-07-26 15:27:30 EDT ---

This is my first package.

I would like to add it to EPEL7, F27, F28, F29, and Rawhide.

--- Additional comment from Robert-André Mauchin on 2018-07-26 16:23:57 EDT ---

 - Use a more meaningful name for your archive:

Source0:        https://%{provider_prefix}/archive/v%{version}/%{name}-%{version}.tar.gz

 - Add a comment above the patch describing why it is needed.

 - You should unbundle the dependencies and remove the vendor directory in %prep (might take a while). That implies packaging any missing dependency.

 - It is not ok to apply a patch on some architecture only. The arch detection if needed should be in the patched code itself.

 - with_bundled isn't defined anywhere.


There's a new way to package the Go libraries, see https://fedoraproject.org/wiki/More_Go_packaging and samples: https://eclipseo.fedorapeople.org/golang/

It would be great to convert to the new style for F27-Rawhide and keep the old style for EPEL7.

--- Additional comment from  on 2018-07-26 18:59:36 EDT ---

> - Use a more meaningful name for your archive: 
>
> Source0:        https://%{provider_prefix}/archive/v%{version}/%{name}-% {version}.tar.gz

That results in the source url no longer being valid and doesn't that go against the Fedora url guidelines which wants the actual source url?

> - Add a comment above the patch describing why it is needed.

Will fix

> - You should unbundle the dependencies and remove the vendor directory in %prep (might take a while). That implies packaging any missing dependency.

This was recommended to me by one of the package wranglers as well as the cri-o people until the GO packaging guidelines are finalized

> - It is not ok to apply a patch on some architecture only. The arch detection if needed should be in the patched code itself.

Will fix

> - with_bundled isn't defined anywhere.

Will fix

> There's a new way to package the Go libraries, see https://fedoraproject.org/wiki/More_Go_packaging and samples: https://eclipseo.fedorapeople.org/golang/
>
> It would be great to convert to the new style for F27-Rawhide and keep the old style for EPEL7.

That looks to be a proposal. Does the draft not have precedence?

--- Additional comment from Robert-André Mauchin on 2018-07-26 19:13:34 EDT ---

(In reply to ralford from comment #3)
> > - Use a more meaningful name for your archive: 
> >
> > Source0:        https://%{provider_prefix}/archive/v%{version}/%{name}-% {version}.tar.gz
> 
> That results in the source url no longer being valid and doesn't that go
> against the Fedora url guidelines which wants the actual source url?
> 
The source URL I provided is valid, check again.


> > - Add a comment above the patch describing why it is needed.
> 
> Will fix
> 
> > - You should unbundle the dependencies and remove the vendor directory in %prep (might take a while). That implies packaging any missing dependency.
> 
> This was recommended to me by one of the package wranglers as well as the
> cri-o people until the GO packaging guidelines are finalized
> 
Could take months, packages are already being unbundled.

> > - It is not ok to apply a patch on some architecture only. The arch detection if needed should be in the patched code itself.
> 
> Will fix
> 
> > - with_bundled isn't defined anywhere.
> 
> Will fix
> 
> > There's a new way to package the Go libraries, see https://fedoraproject.org/wiki/More_Go_packaging and samples: https://eclipseo.fedorapeople.org/golang/
> >
> > It would be great to convert to the new style for F27-Rawhide and keep the old style for EPEL7.
> 
> That looks to be a proposal. Does the draft not have precedence?

Most Go packages have already been converted to the new style this past year.

--- Additional comment from  on 2018-07-26 20:32:59 EDT ---

> The source URL I provided is valid, check again.

Doh! My bad. Typed it in wrong.

> Could take months, packages are already being unbundled.

Okay. Thanks for the review and answering questions. Will work through your comments.

Comment 1 ralford 2018-08-14 00:06:43 UTC
*** Bug 1609038 has been marked as a duplicate of this bug. ***

Comment 3 redhatrises 2018-09-17 18:01:46 UTC
This package is ready for review. All dependencies are now in Rawhide.

Comment 4 Hirotaka Wakabayashi 2018-12-09 22:23:02 UTC
Hi, this is an unofficial review. Please read it for your reference.

Summary:
- No rpmlint errors and warnings
- Koji build success
- 2 suggestions on packaging

1. Rpmlint found no 0 errors, 0 warnings!
$ curl -O https://copr-be.cloud.fedoraproject.org/results/rga/compliance-masonry/fedora-28-x86_64/00788804-compliance-masonry/compliance-masonry.s\
pec
$ md5sum compliance-masonry.spec
1f6aca8dcd935fc0cdef6f55940c3a02  compliance-masonry.spec
$ rpmlint -i compliance-masonry.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

2. Scratch Build by Koji is success.
https://koji.fedoraproject.org/koji/taskinfo?taskID=31363528

3. Others
3.1. vendor folder
I found the vendor folder in koji build log but is it required?
https://kojipkgs.fedoraproject.org//work/tasks/3529/31363529/build.log

I found a same golang rpm package that contains BuildRequires directive as your
package and it successfully built without vendor folder. I think it means you 
could remove the vendor folder too if you have no special reasons.
https://bugzilla.redhat.com/show_bug.cgi?id=1655785
https://kojipkgs.fedoraproject.org//packages/golang-github-ovh/0/0.2.20181207gitba5adb4.fc30/data/logs/x86_64/build.log

Reference:
The packaging guideline says
```
Software which downloads code bundles from the internet in order to be
functional or useful is not acceptable for inclusion in Fedora (regardless of
whether the downloaded code would be acceptable to be packaged in Fedora as a
proper dependency).
```
https://docs.fedoraproject.org/en-US/packaging-guidelines/what-can-be-packaged/#_packages_which_are_not_useful_without_external_code

3.2. Naming
compliance-masonry should be golang-github-compliance-masonry. 
I think you need not to define %goname in your spec file because the %gometa
macro defines it.
https://fedoraproject.org/wiki/More_Go_packaging#Spec_preamble:_.25.7Bgoipath.7D.2C_.25.7Bforgeurl.7D_and_.25gometa

Reference:
The golang naming guidlines which is still in WIP provides the package naming guideline.
https://fedoraproject.org/wiki/PackagingDrafts/Go#Package_Names

gofed is another solution that helps you to generate your spec file.
https://github.com/gofed/gofed#spec-file-generator

Thanks in advance,
Hirotaka Wakabayashi

Comment 5 Hirotaka Wakabayashi 2018-12-16 21:45:32 UTC
Ignore my comment about Naming because the following guideline says a binary package do not need a "golang" prefix and I think your package is a binary package.
https://fedoraproject.org/wiki/PackagingDrafts/Go#Packaging_Binaries

Thanks,
Hirotaka Wakabayashi

Comment 7 Package Review 2021-05-08 00:45:13 UTC
This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time. We're sorry
it is taking so long. If you're still interested in packaging this software
into Fedora repositories, please respond to this comment clearing the
NEEDINFO flag.

You may want to update the specfile and the src.rpm to the latest version
available and to propose a review swap on Fedora devel mailing list to increase
chances to have your package reviewed. If this is your first package and you
need a sponsor, you may want to post some informal reviews. Read more at
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.

Without any reply, this request will shortly be considered abandoned
and will be closed.
Thank you for your patience.

Comment 8 Package Review 2021-06-07 00:45:18 UTC
This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.


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