Bug 1290513 - Review Request: playonlinux - Front-end application for the wine
Review Request: playonlinux - Front-end application for the wine
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Zbigniew Jędrzejewski-Szmek
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2015-12-10 12:25 EST by Jiri Konecny
Modified: 2016-03-05 01:21 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-03-05 01:21:19 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
zbyszek: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Jiri Konecny 2015-12-10 12:25:40 EST
Spec URL: https://jkonecny.fedorapeople.org/packages/playonlinux/playonlinux.spec
SRPM URL: https://jkonecny.fedorapeople.org/packages/playonlinux/playonlinux-4.2.9-1.fc23.src.rpm
Description: PlayOnLinux better management for your wine.
Fedora Account System Username: jkonecny@redhat.com
Comment 1 Jiri Konecny 2015-12-10 12:29:31 EST
Hi people,

I want to get PlayOnLinux to Fedora main repository if it's not against the rules.

This is my first package so I need a maintainer here. I'm doing this in my free time and it's not so easy to find some, you know it ;).

Here is the koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=12139716

Here is the copr repository:
https://copr.fedoraproject.org/coprs/jkonecny/PlayOnLinux/

Thank you for patience with me.
Comment 2 Upstream Release Monitoring 2015-12-10 13:53:26 EST
jkonecny's scratch build of playonlinux-4.2.9-2.fc23.src.rpm for f24 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12140459
Comment 3 Jiri Konecny 2015-12-10 14:41:24 EST
I found that this was broken version because I unintentionally exclude lang files from the package files.

New:
SRPM URL: https://jkonecny.fedorapeople.org/packages/playonlinux/playonlinux-4.2.9-2.fc23.src.rpm

Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=12140737

New copr build:
https://copr.fedoraproject.org/coprs/jkonecny/PlayOnLinux/build/146885/
Comment 4 Johnny Robeson 2015-12-11 04:53:28 EST
can you avoid the gstreamer 0.10 dependency? Fedora's wine removed the gstreamer 0.10 dependency during Fedora 22.
Comment 5 Jiri Konecny 2015-12-11 05:57:58 EST
Hello Johnny,

it seems that the gstreamer is there because of wxPython. I really don't know how or why they are using it. I will try and see.
Comment 6 Jiri Konecny 2015-12-11 06:00:13 EST
I looked on source code in repository and it seems they heavily depends on wxPython so sorry I can't remove it.

gstreamer seems as mandatory dependency.
Comment 7 Johnny Robeson 2015-12-11 06:21:43 EST
if it could build against gstreamer 1.x, then it'd be fine. It doesn't look like it does though, so I guess I'm out for being able to test it. :(
Comment 8 Neal Gompa 2016-01-04 05:57:57 EST
@Jiri, could you please change your python Requires to "python2"? It makes it more clear that it has a hard requirement on Python 2, especially once "python" is switched to Python 3 in the future.

Additionally, please use the macros in the %install phase.

For example, "/usr" would be replaced with %{_prefix}, "/usr/share" would be replaced with %{_datadir}, and so on.

Also, unless you're targeting older than EL7, you can choose to use %autosetup or %setup and %autopatch.

For example, with %autosetup:
%autosetup -n %{BUILD_DIR} -p1

Or, with %setup and %autopatch:
%setup -qn %{BUILD_DIR}
%autopatch -p1
Comment 9 Upstream Release Monitoring 2016-01-05 13:03:27 EST
jkonecny's scratch build of playonlinux-4.2.10-1.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12421878
Comment 10 Jiri Konecny 2016-01-05 14:08:16 EST
Hello Neal,

I did the changes you want, hope I didn't missed something. If there is something I missed please tell me.

New changes to spec:
- New version of PlayOnLinux 4.2.10-1
- I'm now using more macros in %install section
- Using %autosetup now (thank you I had hard times with using it properly but your simple command worked like a charm)
- Add 2 new dependencies I missed before (icoutils and p7zip-plugins). PlayOnLinux works even without them but when it's started it show dialog complaining about missing tools.

New koji and copr builds are done. New spec is on the same address.

Thank you very much for helping me.
Comment 11 Neal Gompa 2016-01-06 20:15:05 EST
Jiri,

Since it appears you're the author of the Makefile being used, could you make it compatible with our macros %make_build and %make_install?

%make_build expands out to: make %{?_smp_mflags}

%make_install expands out to: make install DESTDIR=%{?buildroot}

Most likely, %make_build will work fine, since the "all" target in your makefile goes to "build".

However, your install target doesn't handle DESTDIR, it seems. If your makefile handled it correctly, you could use %make_install PREFIX=%{_prefix} and it would work, because it would internally combine DESTDIR (set to %{buildroot}) with PREFIX to create the install path. Or it could default to /usr as the prefix and not require it to be set unless otherwise being changed.

Also, why are Python scripts in the datadir being set with the executable bit? Stuff there shouldn't generally be executable.
Comment 12 Miroslav Suchý 2016-01-07 08:34:53 EST
Also note that it is good habit once you do some changes to post here link to new spec file (even if the url did not changed) and to new srpm.
Comment 13 Upstream Release Monitoring 2016-01-07 16:56:40 EST
jkonecny's scratch build of playonlinux-4.2.10-2.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12455943
Comment 14 Jiri Konecny 2016-01-07 17:15:17 EST
Hello Neal,

I changed the patches for Makefile and also updated the PR in upstream. New version of package is in my copr repository and new koji scratch build was done.

Package is now version 4.2.10-2 and it's using %make_install and %make_build macros.

New spec file: https://jkonecny.fedorapeople.org/packages/playonlinux/playonlinux.spec
New srpms file: https://jkonecny.fedorapeople.org/packages/playonlinux/playonlinux-4.2.10-2.fc23.src.rpm

Without the executable bits rpmlint starts complaining with these errors so the chmod commands are still there:

playonlinux.x86_64: E: non-executable-script /usr/share/playonlinux/tests/python/test_versionlower.py 644 /usr/bin/python
playonlinux.x86_64: E: non-executable-script /usr/share/playonlinux/tests/bash/test-versionlower 644 /bin/bash
playonlinux.x86_64: E: non-executable-script /usr/share/playonlinux/python/gui_server.py 644 /usr/bin/python
2 packages and 1 specfiles checked; 3 errors, 0 warnings.

thank you for helping me with this.


Hello Miroslav,

I will do it for future, thank you for telling me this. I didn't know that.
Comment 15 Eduardo Mayorga 2016-01-07 18:19:36 EST
(In reply to Jiri Konecny from comment #14)
> Without the executable bits rpmlint starts complaining with these errors so
> the chmod commands are still there:
> 
> playonlinux.x86_64: E: non-executable-script
> /usr/share/playonlinux/tests/python/test_versionlower.py 644 /usr/bin/python
> playonlinux.x86_64: E: non-executable-script
> /usr/share/playonlinux/tests/bash/test-versionlower 644 /bin/bash
> playonlinux.x86_64: E: non-executable-script
> /usr/share/playonlinux/python/gui_server.py 644 /usr/bin/python
> 2 packages and 1 specfiles checked; 3 errors, 0 warnings.

Removing the shebang will fix this error. See: https://fedoraproject.org/wiki/Packaging_tricks#Remove_shebang_from_files.
Comment 16 Miroslav Suchý 2016-01-08 07:49:51 EST
(In reply to Jiri Konecny from comment #14)
> Without the executable bits rpmlint starts complaining with these errors so
> the chmod commands are still there:

Python files are either executable and have executable bit set on and shebang at the top. Or it is just library and should have neither the bit set on nor the shebang.

If file have x bit set on and not shebang or have shebang and not x bit set on, then it is something in between, and weirdo. So we do not allow it.

Files in /usr/share are usually libraries so should not have x bit set on and should not have shebang. Unless you have good reason for the opposite.
Comment 17 Zbigniew Jędrzejewski-Szmek 2016-01-08 09:04:06 EST
Both Summary and %description are pretty awkward. There should be no "the" article before "wine", it is a name. Also "application" is redundant, we don't say "text editor application" about emacs, just "text editor". And %description should be expanded so that people who do not know what p.o.l. is already can see what it is about.

It would be really great to add appdata file (https://fedoraproject.org/wiki/Packaging:AppData). This will make p.o.l. show up in searches in gnome-software and in the gnome overview, which is especially useful for non-powerusers. Usually the screenshots are the most work, in this case there are some screenshots on the website: https://www.playonlinux.com/images/playonlinux_screenshots/install_wizard.png. They are not directly usable, because they are too small, include too much background, and localized in French.
Comment 18 Upstream Release Monitoring 2016-01-13 14:35:03 EST
jkonecny's scratch build of playonlinux-4.2.10-3.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12537434
Comment 19 Jiri Konecny 2016-01-13 14:51:07 EST
Hello all,

new versions of files are here:
New spec file: https://jkonecny.fedorapeople.org/packages/playonlinux/playonlinux.spec
https://jkonecny.fedorapeople.org/packages/playonlinux/playonlinux-4.2.10-3.fc23.src.rpm

Eduardo:
I have removed the chmod from spec file and removed from the files shebang instead. Thank you for the link you sent. 
I don't know if it will be better to remove executable from all files in /usr/share/playonlinux and if that could be done at all. Please tell me if this should be OK or I need to try to remove others.

Miroslav:
Thank you for nice explanation. For this application it's harder because the program is done for use case: download -> unpack -> run. So I don't know if it will run when all executable bits will be removed so to change them to "libraries" and how much I can split the files to other folders. I do know I can't move the main executable script to bin folder because of local paths.

Zbigniew:
I created the appdata.xml file and did a pull request to the upstream. English isn't something I do best so if you see something which can be improved write me. I will gladly take any help on this. The same apply for new description and summary of the package.

Pull request on appdata.xml: https://github.com/PlayOnLinux/POL-POM-4/pull/36
Comment 20 Zbigniew Jędrzejewski-Szmek 2016-01-13 20:38:17 EST
There is a rule to preserve timestamp of *unmodified* files. Once you run sed on a file, it's doesn't apply anymore. People do this thing with .orig file, but it's just a waste of time IMHO.

Another thing that people do is to use %{__sed} and %{__rm}, %{__make}, etc. But %{__sed} will always be sed, etc. It's not like %{__python} which can at some point to /usr/bin/python3. There is a rule to use macros for *directories* (and some specific applications as python), which can change over time. Doing it for standard unix programs just decreases readability.

OTOH, let's say that upstream changes those files not to have the line you want to delete. You want to guard against that.

So you can just do:
sed -i '1{/^#!\//d}' %{BUILD_DIR}/python/gui_server.py \
          %{BUILD_DIR}/tests/python/test_versionlower.py \
          %{BUILD_DIR}/tests/bash/test-versionlower

Safer and simpler ;)


Thank you for the appdata file. It needs some love though. Usually you want to use "appstream-util validate" locally (to catch as many errors as possible), and put "appstream-util validate-relax" in the spec file (so the build does not break on minor things when new versions of appstream-util are released). Running appstream-util validate PlayOnLinux.appdata.xml:

PlayOnLinux.appdata.xml: FAILED:
• tag-invalid           : <project_license> is not valid [GPLv3]SPDX ID 'GPLv3' unknown
• attribute-invalid     : <screenshot> width too small [https://jkonecny.fedorapeople.org/images/Screenshot%20from%202016-01-12%2019-14-40.png]
Validation of files failed

(BTW, shutter is pretty good for screenshots, and gives nice names automatically).

Both seem to be valid complaints. Small width causes the screenshots to be surrounded by gray bars in gnome-software. The main window is resizeable, but the installation window has fixed size here. It seems to be some kind of bug, because I cannot even display most of the contents of the window. Upstream bugs like that do not block the review, but you might want to investigate, and/or notify upstream. (Oh, I looked at the sources now. Not a pleasant view.)

Appdata file should go into %{_datadir}/appdata/, not /usr/share/applications/PlayOnLinux.appdata.xml.
It also appears in /usr/share/playonlinux/etc/PlayOnLinux.appdata.xml, which seems to be some mistake.

In the text of the appdata file, and also in %description, I'm missing some sentence or two which say how this applications helps (I'm *guessing* it has a database of commonly used programs, and can list and download them automatically, and is able to configure wine specifically for each program. The list of programs and configuration settings is periodically updated over the web.)


rpmbuild complains:
warning: File listed twice: /usr/share/playonlinux/lang/locale/ar/LC_MESSAGES/pol.mo
warning: File listed twice: /usr/share/playonlinux/lang/locale/ast/LC_MESSAGES/pol.mo
warning: File listed twice: /usr/share/playonlinux/lang/locale/bg/LC_MESSAGES/pol.mo
warning: File listed twice: /usr/share/playonlinux/lang/locale/bn/LC_MESSAGES/pol.mo
...
warning: File listed twice: /usr/share/playonlinux/lang/locale/zh_TW/LC_MESSAGES/pol.mo

This is because lang files are stored in a directory which is also listed in %files. Normally lang files would be stored underneath /usr/share/locale. I'm not aware of any issues which would be caused by keeping lang files in current location, so the simplest solutions seems to add: %exclude %{_datadir}/%{name}/lang/locale/bn/LC_MESSAGES/*.mo to %files to avoid the warning.


OK, the program runs, displays stuff, and seems to be generally functional.
The installation does not work for me, because it has a small fixed size
and the buttons at the bottom are cut off. Maybe this is only under wayland?
Under normal X the buttons are also cut off, but just a bit.

So we're at least 95% of the way there with the package. As far as sponsorship goes, please do two or three reviews of packages from http://fedoraproject.org/PackageReviewStatus/NEW.html. Running fedora-review is a good first step, but please note that the automatically generated template needs to be filled in in various places, and trimmed in others. Not everything the tools say is always correct. Sometimes they are outdated, sometimes they are plain wrong. It's always best to link to the relevant part of the guidelines. Please pick packages that are in the area you are interested in, so that you can finalize the review after you get the packager bit. If you have any questions or issues, I'd always be happy to help (zbyszek at in waw pl, zbyszek on #fedora-devel).
Comment 21 Jiri Konecny 2016-01-20 12:58:18 EST
Hello Zbigniew,

sorry it took me so long time to reply. New version is on the move :). Thank you for your valuable input. I already changed the sed command, added %exclude macro and fixed the screenshots (btw thank you for the Shutter it's much easier).

I have changed the appdata but I don't quite like it so I need to give it some more time.

Hopefully I will finish it this week.

BTW I will look on the bug later.
Comment 22 Zbigniew Jędrzejewski-Szmek 2016-01-20 16:11:44 EST
(In reply to Jiri Konecny from comment #21)
> BTW I will look on the bug later.
If you have trouble reproducing it, I can provide some screenshots.
Comment 23 Upstream Release Monitoring 2016-01-22 16:16:17 EST
jkonecny's scratch build of playonlinux-4.2.10-5.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12653018
Comment 24 Jiri Konecny 2016-01-22 16:28:37 EST
Hello,

so finally I finished work on the package.

New spec file: 
https://jkonecny.fedorapeople.org/packages/playonlinux/playonlinux.spec
New src rpm:
https://jkonecny.fedorapeople.org/packages/playonlinux/playonlinux.spec

At the end I fixed the lang files on their side. I have PR there so we will see if they will like it or not. If they won't I found some other way or I left them in application folder as you wrote before. In short now the locale files are installed to /usr/share/locale by Makefile and there was required to set variable to load the files from there.

Now I have in my TODO to look on the review of some packages. I will try to do it soon. 

Zbigniew I have the same issue as you on my VM when I use wayland so seems as wayland specific. So or so the positioning there is broken. I will look on that in the future when I found some spare time but now I'm going to file a bug on this in their bug tracker.
Comment 25 Zbigniew Jędrzejewski-Szmek 2016-01-22 20:29:08 EST
Package looks good, no issues.
Comment 26 Zbigniew Jędrzejewski-Szmek 2016-02-20 17:31:59 EST
Hej, any news with the other reviews?
Comment 27 Jiri Konecny 2016-02-22 03:53:38 EST
Hello,

sorry with the DevConf and all other things I didn't found quite much time. Another issue I'm trying to find something what suites me :).

Good new is I have taken this one so far

https://bugzilla.redhat.com/show_bug.cgi?id=1286885

I will try to look for another but it would be really nice if you help me (correct me) with this one.

Thank you for your patience Zbigniew.
Comment 28 Jiri Konecny 2016-02-23 13:53:41 EST
Hello Zbigniew,

I chose another review and it's this one https://bugzilla.redhat.com/show_bug.cgi?id=1305154 but I didn't find anything against this package.

If you please can give me the packager status I will confirm review for 

orthorobot:
https://bugzilla.redhat.com/show_bug.cgi?id=1286885

and for the python-notario library (link above).

All these packages seems fine to me (and to the fedora-review tool too). I build them, installed them and run them without problem. I think they should be in Fedora repo but I will be glad if you look on them and said if you find something there.

I'm not assigned to python-notario because I don't have anything to say there and I don't want to block that review when it seems ok to me. I want to approve the package when I get the status.
Comment 29 Zbigniew Jędrzejewski-Szmek 2016-03-01 20:09:54 EST
Jiří, I've added you to the packagers group now. Use your powers for the good!

I think your comments on the two reviews are very reasonable.
I'll be happy to answer any question or help otherwise.
Comment 30 Jiri Konecny 2016-03-02 01:53:19 EST
Thank you very much Zbigniew.

I'm going to finish the reviews now, to spread the good news :).
Comment 31 Gwyn Ciesla 2016-03-02 09:02:15 EST
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/playonlinux
Comment 32 Fedora Update System 2016-03-02 12:48:20 EST
playonlinux-4.2.10-5.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-1430c7fcdb
Comment 33 Fedora Update System 2016-03-03 16:58:20 EST
playonlinux-4.2.10-5.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-1430c7fcdb
Comment 34 Fedora Update System 2016-03-05 01:21:16 EST
playonlinux-4.2.10-5.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.

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