Bug 1426844 - Review Request: notepadqq - An advanced text editor for developers
Summary: Review Request: notepadqq - An advanced text editor for developers
Keywords:
Status: CLOSED DUPLICATE of bug 1519785
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: William Moreno
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2017-02-25 15:37 UTC by kevin01010
Modified: 2017-12-07 18:10 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-11-30 21:48:30 UTC
Type: ---
Embargoed:
rdieter: fedora-review-


Attachments (Terms of Use)

Description kevin01010 2017-02-25 15:37:04 UTC
Spec URL: https://mirror.jkanetwork.com/Fedora/SOURCES/notepadqq.spec
SRPM URL: https://mirror.jkanetwork.com/Fedora/SOURCES/notepadqq-1.0.1-4.fc25.src.rpm
Description: A text editor for developers, similar to notepad++ in Windows.
Fedora Account System Username: kprkpr

Hi, I packed notepadqq for Fedora
Is my first contribution to Fedora and I need a sponsor. I'm not the creator of Notepadqq, but I talked with him and updated their .spec files and patches

Koji success build: https://koji.fedoraproject.org/koji/taskinfo?taskID=18054539

rpmlint
For .spec:
notepadqq.spec:39: W: rpm-buildroot-usage %build %configure --qmake=qmake-qt5 --prefix %{buildroot}/usr --lrelease /usr/bin/lrelease-qt5
notepadqq.spec:79: W: macro-in-comment %{buildroot}
notepadqq.spec:79: W: macro-in-comment %{_datadir}
notepadqq.spec:79: W: macro-in-comment %{nam

Line 39 is because I used it to change build prefix in make
Line 79 is not a comment, but I don't know why it detects as it. Its in a sed changes for env (That in fedora it can't be)

For srpm:
notepadqq.src: W: strange-permission v1.0.1.tar.gz 777
notepadqq.src: W: strange-permission 5.18.2-nqq.tar.gz 777
notepadqq.src:39: W: rpm-buildroot-usage %build %configure --qmake=qmake-qt5 --prefix %{buildroot}/usr --lrelease /usr/bin/lrelease-qt5
notepadqq.src:79: W: macro-in-comment %{buildroot}
notepadqq.src:79: W: macro-in-comment %{_datadir}
notepadqq.src:79: W: macro-in-comment %{name}
1 packages and 0 specfiles checked; 0 errors, 6 warnings.

Same warnings, and two more from permissions. I think is not important as is the .tar.gz

Comment 1 Nemanja Milosevic 2017-02-25 17:40:24 UTC
Until someone more experienced reviews this package, here are my thoughts:

> Summary:		A Linux clone of Notepad++

Refrain from using constructs like this. Notepad++ is proprietary software if I'm correct, and we shouldn't use their name without permission. Why not just qt based advanced text editor. 

> Source0:		https://github.com/notepadqq/notepadqq/archive/v%{version}.tar.gz

Can also be:

> Source0:		%{url}/archive/v%{version}.tar.gz

You can also add #/%{name}-%{version}.tar.gz to the end to have spectool download the file with a pretty file name. 

> Source1:		https://github.com/notepadqq/CodeMirror/archive/5.18.2-nqq.tar.gz

Why is this needed, and what is this? If it's a needed library why not package it separately?

In %description, avoid bullet points.

No need to reference builddir in %setup that's the current directory. You can use tar xf %SOURCE1 to achieve the same. However, you shouldn't be doing this at all. (as stated above)

Make doesn't respect RPM_OPT_FLAGS. Consider using %make_build.

No need to remove RPM buildroot directory. 

Why aren't you using %make_install?

You are patching files with sed after compilation, which seems like a bad practice to me. You should patch before build using %patch. If that's impossible, maybe contact upstream to make the build process more smooth.

You are mentioning node, which isn't listed as a requirement? Does it require a newer version?

------

That's it for me, hopefully someone more experienced can also provide feedback. :)

Cheers,
Nemanja

Comment 2 kevin01010 2017-02-26 00:18:15 UTC
(In reply to Nemanja Milosevic from comment #1)


Hi and thanks for explaining all
I will answer some of them and fix problems

> Refrain from using constructs like this. Notepad++ is proprietary software if I'm correct, and we shouldn't use their name without permission. Why not just qt based advanced text editor. 

Notepad++ is GPL software, although I will change the name in description for you say

> Source0: ... 
Fixed
About the name, I changed as you write, sounds a bit strange but works

> Source1:		https://github.com/notepadqq/CodeMirror/archive/5.18.2-nqq.tar.gz
>Why is this needed, and what is this? If it's a needed library why not package it separately?

Its a submodule of notepadqq git, github doesnt download it when it downloads the .tar.gz . I didnt think of it as a library because is in it and its like a js file but a lot of it, as an addon. (And without it it doesnt builds)

Corrected description

About the patchs..
The launcher one I sent a better patch to upstream and its merged, but is not released yet (https://github.com/notepadqq/notepadqq/commit/13dec7d5e1008c0d14de56c739d45ec1477f4557 )

The rest, I put it in the prep section, I'm sorry but I dont understand how %patch works

> Make doesn't respect RPM_OPT_FLAGS. Consider using %make_build.
> Why aren't you using %make_install?
I didnt use %make_build and %make_install because I read first time wiki in spanish (I'm Spanish) and it doesnt mention it..
I put %make_build, but it seems that make install doesnt change nothing

> No need to remove RPM buildroot directory.
I think is because same as above, removed line

> You are mentioning node, which isn't listed as a requirement? Does it require a newer version?
It's because for 99% of users notepadqq doesnt need it, and I dont know if its a way to put "Optional" depends

Uploaded changes:
Spec URL: https://mirror.jkanetwork.com/Fedora/SOURCES/notepadqq.spec
SRPM URL: https://mirror.jkanetwork.com/Fedora/SOURCES/notepadqq-1.0.1-5.fc25.src.rpm

Thanks

Comment 3 kevin01010 2017-02-28 22:28:14 UTC
I reuploaded notepadqq spec and srpm from changes at github from developer at same location

url for more info: https://github.com/notepadqq/notepadqq-packaging/pull/58

Comment 4 William Moreno 2017-03-13 19:49:00 UTC
Nice to see you working with upstream, I will run the review, it will be nice if you do some informal reviews.

Regards

Comment 5 kevin01010 2017-03-14 15:14:22 UTC
(In reply to William Moreno from comment #4)
> Nice to see you working with upstream, I will run the review, it will be
> nice if you do some informal reviews.
> 
> Regards

Great :)
I will see it because I don't know much about rpm and reviews, but I will try to test or do things if I can

Comment 6 William Moreno 2017-10-01 16:45:15 UTC
Hello kevin, do you still want to continúe here?

Comment 7 Raphael Groner 2017-10-25 18:10:11 UTC
another friendly ping. I could continue as requester if interest is lost.

Comment 8 Rex Dieter 2017-10-25 18:10:58 UTC
I can help sponsor-wise, assuming submitter is available and interested in continuing this review (per comment #6)

Comment 9 kevin01010 2017-10-25 18:50:05 UTC
Hi, I'm sorry for not answering before, mail of #6 got lost in my inbox
I'm not doing anymore that sorry, anyone that wants to continue is welcome :)

Comment 10 Fedora End Of Life 2017-11-16 19:18:37 UTC
This message is a reminder that Fedora 25 is nearing its end of life.
Approximately 4 (four) weeks from now Fedora will stop maintaining
and issuing updates for Fedora 25. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as EOL if it remains open with a Fedora  'version'
of '25'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version'
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not
able to fix it before Fedora 25 is end of life. If you would still like
to see this bug fixed and are able to reproduce it against a later version
of Fedora, you are encouraged  change the 'version' to a later Fedora
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's
lifetime, sometimes those efforts are overtaken by events. Often a
more recent Fedora release includes newer upstream software that fixes
bugs or makes them obsolete.

Comment 11 jan 2017-11-30 20:46:55 UTC
Hello,

I'd like to take over to get this package included with Fedora28. (Sooner would be great, but well.)

Comment 12 Rex Dieter 2017-11-30 21:48:30 UTC
Given comment #9, this is a dead review.

If you want to pick things up, go ahead an open a new one.

Comment 13 Raphael Groner 2017-12-07 18:10:19 UTC

*** This bug has been marked as a duplicate of bug 1519785 ***


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