Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1974377 - Review Request: rust-clang-ast - Data structures for processing Clang's `-ast-dump=json` format
Summary: Review Request: rust-clang-ast - Data structures for processing Clang's `-ast...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Fabio Valentini
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1974172 1974529
TreeView+ depends on / blocked
 
Reported: 2021-06-21 13:49 UTC by Jan Staněk
Modified: 2021-06-23 13:02 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-06-23 13:02:04 UTC
Type: ---
Embargoed:
decathorpe: fedora-review+

Attachments (Terms of Use)

Description Jan Staněk 2021-06-21 13:49:54 UTC 
Spec URL: https://download.copr.fedorainfracloud.org/results/jstanek/rust-clang-ast/fedora-rawhide-x86_64/02285123-rust-clang-ast/rust-clang-ast.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/jstanek/rust-clang-ast/fedora-rawhide-x86_64/02285123-rust-clang-ast/rust-clang-ast-0.1.6-1.fc35.src.rpm
Description: Data structures for processing Clang's `-ast-dump=json` format. Transitive build-time dependency of the newsboat package.
Fedora Account System Username: jstanek
Comment 1 Fabio Valentini 2021-06-22 11:23:35 UTC 
I'll take on this review.

If you want to review another Rust package in turn: 
https://bugzilla.redhat.com/show_bug.cgi?id=1973218
Comment 2 Fabio Valentini 2021-06-22 11:39:28 UTC 
Minor issues:

0) remove markdown markup from Summary

Please remove markdown markup characters (`) from the Summary tag.

1) "# check has missing dependencies"

Please list which dependencies are missing.

2) %description

Looks like you changed "Clang's" to "Clang" in the generated description. Why?

3) %license in %files

To be consistent with ~all~ other Rust packages (and to make version rebases easier for me), change to this please:

%files          devel
%license LICENSE-APACHE LICENSE-MIT
%doc README.md

4) changelog date format

Please remove the time and timezone from the date in the changelog entry.
This broken format is only used by rust2rpm any longer and I hope to be able to remove it there with the next version as well.


---


All those are very minor or cosmetic issues. Other than those:

Package was generated with rust2rpm, simplifying the review process.

- package conforms to Rust packaging Guidelines ✅
- Latest version is packaged ✅
- License correct, acceptable, and LICENSE files shipped with %license ✅
- package builds and installs without errors in mock / rawhide ✅

Package APPROVED.
Please make the small adaptations listed above before importing the package to Fedora.
Comment 3 Jan Staněk 2021-06-22 11:55:52 UTC 
(In reply to Fabio Valentini from comment #2)

> 0) remove markdown markup from Summary
✔

> 1) "# check has missing dependencies"
listed clang-ast-test-suite ✔

> 2) %description
> Looks like you changed "Clang's" to "Clang" in the generated description.
> Why?

It messes my syntax higlighting in vim, and I forgot to undo the change once I was done with the spec. Apostrophe added back 🙂

> 3) %license in %files

Changed as requested. ✔

> 4) changelog date format

Time and timezone removed. ✔

---

Thanks for the review! The one in kind should be approved now.
Comment 4 Fabio Valentini 2021-06-22 12:16:54 UTC 
Thanks!

Don't forget to request the f34 branch as well:
fedpkg request-branch --repo rust-clang-ast f34

(You can run this even if the repo does not exist yet, since those tickets are processed chronologically.)
Comment 5 Jan Staněk 2021-06-22 12:35:27 UTC 
(In reply to Fabio Valentini from comment #4)
> (You can run this even if the repo does not exist yet, since those tickets
> are processed chronologically.)

That is good to know – the guidelines state this should be run *after* the repo is created. This is much more convenient 😀

https://pagure.io/releng/fedora-scm-requests/issue/35189 ← the repo
https://pagure.io/releng/fedora-scm-requests/issue/35192 ← the branch
Comment 6 Gwyn Ciesla 2021-06-22 13:31:13 UTC 
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-clang-ast
Comment 7 Jan Staněk 2021-06-23 13:02:04 UTC 
Successfully built in rawhide: https://bodhi.fedoraproject.org/updates/FEDORA-2021-ff9c7e3722
Note You need to log in before you can comment on or make changes to this bug.