https://bugzilla.redhat.com/show_bug.cgi?id=1519785
Ben Rosser rosser.bjr@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |rosser.bjr@gmail.com
--- Comment #2 from Ben Rosser rosser.bjr@gmail.com --- 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#Intro...).
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...
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...
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_Librar...
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_usa...
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.