https://bugzilla.redhat.com/show_bug.cgi?id=1519785 Ben Rosser <rosser.bjr@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |rosser.bjr@xxxxxxxxx --- Comment #2 from Ben Rosser <rosser.bjr@xxxxxxxxx> --- This looks like a nice piece of software! I'd be happy to review it and sponsor you. It doesn't look like you've introduced yourself on the Fedora devel mailing list. I'd encourage you to do so. (https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Introduce_yourself). I did a first pass of the spec, but this is not necessarily exhaustive. > Spec URL: https://github.com/jdeluyck/notepadqq-packaging/blob/master/Fedora/notepadqq.spec In order to aid the automated review tools, it's better to provide a "raw" github link, like: > Spec URL: https://raw.githubusercontent.com/jdeluyck/notepadqq-packaging/master/Fedora/notepadqq.spec Once you do so, you'd be able to run the "fedora-review" tool over a bugzilla ticket by, say: > dnf install fedora-review > fedora-review -b 1519785 -m fedora-rawhide-x86_64 This will do some additional checks beyond what rpmlint does. > Patch0: node-path.patch > Patch1: add-node.patch > Patch2: bash-path.patch I took a look at the patches and they all seem reasonable. It'd be good to include comments directly in the spec explaining what they are for, though. > Source1: https://github.com/codemirror/CodeMirror/archive/5.32.0.tar.gz I need to look into this in more detail, but it looks like you're bundling codemirror. Is this because the version of nodejs-codemirror (4.8) in Fedora is too old? Either way, if you need to bundle a package you must follow the bundling policy, for this and any other bundled nodejs libraries. https://fedoraproject.org/wiki/Bundled_Libraries?rd=Packaging:Bundled_Libraries I'd encourage you to attempt to unbundle nodejs libraries by symbolically linking to system copies instead, where possible. > %setup -q You can avoid the patch directives by using the "%autosetup -p1" macro instead (no -q is needed). It will automatically apply the patches. > %{_datarootdir}/%{name} You can use %{_datadir} instead of %{_datarootdir} (it's a bit shorter to type). https://fedoraproject.org/wiki/Packaging:RPMMacros > %doc %{_mandir}/man1/%{name}.1.gz Manpages are automatically marked as %doc; you don't need to do it manually. Also, the packaging guidelines say that you should use a wildcard here (e.g. ".1*"). That way if the default compression format ever changes specs don't need updating. https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages > cp $(find | grep desktop$) %{buildroot}/%{_datarootdir}/applications It looks like you're using this to install the desktop file. Desktop files need to be installed using the "desktop-file-install" command, or validated with "desktop-file-validate". This will ensure the file is valid. Take a look at this section of the guidelines for more details: https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage Also, please consider writing an AppStream metadata file and submitting it upstream. https://fedoraproject.org/wiki/Packaging:AppData > I'm awaiting some comments before I further push the spec file upstream to the author. While you're welcome to do so, this isn't necessary. The authoritative source of a Fedora package's spec is our own dist-git repositories hosted on https://src.fedoraproject.org/. Specs don't need to be included in upstream 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 To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx
