Spec URL: https://github.com/Chapien/mygui-copr/raw/refs/heads/main/sources/mygui.spec SRPM URL: https://github.com/Chapien/mygui-copr/raw/refs/heads/main/mygui-3.4.3-1.fc42.src.rpm Description: Upstream: https://github.com/MyGUI/mygui This package was maintained until around 2020, at which point it was retired. I am attempting to package openMW, which relies on MyGUI being available. MyGUI has received a few updates since it was retired, so I updated the spec file accordingly. It now uses OpenGL as a backend instead of OGRE. I've removed the demos from the repo for now, as I am unsure if they work without using OGRE, but I've tested the tools and they work as expected. I am unsure of all the build dependencies for openGL projects, so some of them may be unnecessary -- please let me know. This is my first review request, so I am very much open to feedback -- but I've run fedora-review, and the issues it has seem like false positives. From fedora-review: Issues: ======= - Header files in -devel subpackage, if present. Note: mygui-tools : /usr/share/MYGUI/Media/Tools/LayoutEditor/CodeTemplates/BaseLayoutTemplate.h See: https://docs.fedoraproject.org/en-US/packaging- guidelines/#_devel_packages - If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. Note: License file _licensing.html is not marked as %license See: https://docs.fedoraproject.org/en-US/packaging- guidelines/LicensingGuidelines/#_license_text - Package does not use a name that already exists. Note: A package with this name already exists. Please check https://src.fedoraproject.org/rpms/mygui See: https://docs.fedoraproject.org/en-US/packaging- guidelines/Naming/#_conflicting_package_names For the first, the header file is part of the sample code, and as such I don't believe it should be in the includedir. _licensing.html is from the doxygen docs; COPYING.MIT is the actual license and is placed appropriately. Finally, the last one is obvious; I am attempting to unretire an existing package, so the name does of course exist. Fedora Account System Username: chapien Since it might be relevant, here is the copr repo link: https://copr.fedorainfracloud.org/coprs/chapien/mygui/
Just a few quick nitpicks before doing a proper review: > Source0: https://github.com/MyGUI/mygui/archive/MyGUI3.4.3/mygui-MyGUI%{version}.tar.gz This uses the %{version} macro in one place, but not in the other. Could probably be replaced with: https://github.com/MyGUI/mygui/archive/MyGUI%{version}/mygui-MyGUI%{version}.tar.gz > %if 0%{?fedora} > 41 > Requires: sdl2-compat > %else > Requires: SDL2 > %endif This is unnecessary; sdl2-compat has "Provides: SDL2". > %package tools > Summary: MyGUI tools > Requires: %{name} = %{version}-%{release} This should probably be an archful dependency: "%{name}%{?_isa} = %{version}-%{release}". https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_package (Same deal for the devel package.) > %build > [...] > cd redhat-linux-build It might be safer to use %{_vpath_builddir} here. > %install > rm -rf %{buildroot} "The contents of the buildroot SHOULD NOT be removed in the first line of %install." https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections > install -D %{_buildrootdir}/mygui-MyGUI%{version}/redhat-linux-build/Docs/html/* %{buildroot}%{_datadir}/doc/mygui-devel-doc/html You don't need to specify the full path to the source file, using "redhat-linux-build/Docs/html*" should work fine. (Or, as noted above, "%{_vpath_builddir}/Docs/html/*".
Thank you for the feedback. I made the requested changes.
Copr build: https://copr.fedorainfracloud.org/coprs/build/9150228 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2371328-mygui/fedora-rawhide-x86_64/09150228-mygui/fedora-review/review.txt Found issues: - mygui-tools : /usr/share/MYGUI/Media/Tools/LayoutEditor/CodeTemplates/BaseLayoutTemplate.h Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages - License file _licensing.html is not marked as %license Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text - A package with this name already exists. Please check https://src.fedoraproject.org/rpms/mygui Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
(In reply to Fedora Review Service from comment #3) > Copr build: > https://copr.fedorainfracloud.org/coprs/build/9150228 > (succeeded) > > Review template: > https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora- > review-2371328-mygui/fedora-rawhide-x86_64/09150228-mygui/fedora-review/ > review.txt > > Found issues: > > - mygui-tools : > /usr/share/MYGUI/Media/Tools/LayoutEditor/CodeTemplates/BaseLayoutTemplate.h > Read more: > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages > - License file _licensing.html is not marked as %license > Read more: > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > LicensingGuidelines/#_license_text > - A package with this name already exists. Please check > https://src.fedoraproject.org/rpms/mygui > Read more: > https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/ > #_conflicting_package_names > > Please know that there can be false-positives. > > --- > This comment was created by the fedora-review-service > https://github.com/FrostyX/fedora-review-service > > If you want to trigger a new Copr build, add a comment containing new > Spec and SRPM URLs or [fedora-review-service-build] string. As described in my initial post, I believe these are all false positives.
Copr build: https://copr.fedorainfracloud.org/coprs/build/9150333 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2371328-mygui/fedora-rawhide-x86_64/09150333-mygui/fedora-review/review.txt Found issues: - mygui-tools : /usr/share/MYGUI/Media/Tools/LayoutEditor/CodeTemplates/BaseLayoutTemplate.h Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages - License file _licensing.html is not marked as %license Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text - A package with this name already exists. Please check https://src.fedoraproject.org/rpms/mygui Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
As a heads up, I made a few changes. I added an icon for mygui-layouteditor.desktop. Will likely use it for other mygui tools. Updated spec file to use mygui.png as a source, which is then installed to /usr/share/MYGUI/mygui.png. Will create other desktop files after review most likely, so as not to change things too much.
(In reply to Claire Robsahm from comment #6) > As a heads up, I made a few changes. I added an icon for > mygui-layouteditor.desktop. Will likely use it for other mygui tools. Where does that icon come from? > Updated spec file to use mygui.png as a source, which is then installed to > /usr/share/MYGUI/mygui.png. Will create other desktop files after review > most likely, so as not to change things too much. Please post a new set of SPEC and SRPM URLs each time you change it (and bump the Release: field).
(In reply to Dominik 'Rathann' Mierzejewski from comment #7) > Where does that icon come from? I took the icon from the github repo of MyGUI: https://github.com/MyGUI/mygui > Please post a new set of SPEC and SRPM URLs each time you change it > (and bump the Release: field). The link is the same as it's just in my github repo, but I understand that the SRPM should be changing each time. I am using %{autorelease} on the recommendation of other reviewers, but I don't believe it works when just making edits to the SPEC like I am (at least not on github), so I can switch to manually bumping the build number.
(In reply to Claire Robsahm from comment #8) > (In reply to Dominik 'Rathann' Mierzejewski from comment #7) > > Where does that icon come from? > > I took the icon from the github repo of MyGUI: https://github.com/MyGUI/mygui Please refer to the source location in the spec. > > Please post a new set of SPEC and SRPM URLs each time you change it > > (and bump the Release: field). > > The link is the same as it's just in my github repo, but I understand that > the SRPM should be changing each time. I am using %{autorelease} on the > recommendation of other reviewers, but I don't believe it works when just > making edits to the SPEC like I am (at least not on github), so I can switch > to manually bumping the build number. Even if the spec file URL is unchanged, you must still post it for Fedora Review Service to pick it up and do an automated COPR build. Either that, or at least put in a comment with [fedora-review-service-build]
Created attachment 2093779 [details] The .spec file difference from Copr build 9150333 to 9157212
Copr build: https://copr.fedorainfracloud.org/coprs/build/9157212 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2371328-mygui/fedora-rawhide-x86_64/09157212-mygui/fedora-review/review.txt Found issues: - mygui-tools : /usr/share/MYGUI/Media/Tools/LayoutEditor/CodeTemplates/BaseLayoutTemplate.h Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages - License file _licensing.html is not marked as %license Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text - A package with this name already exists. Please check https://src.fedoraproject.org/rpms/mygui Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
> # Icon png taken from MyGUI's website for Tools desktop files > Source4: mygui.png This seems unnecessary, there are icons included in the source tarball at Media/Common/Sources/Icons, e.g. MyGUI_Icon_LE_256x256.png. > # Put the mygui.png icon in MyGUI's datadir. > install %{SOURCE4} %{buildroot}%{_datadir}/MYGUI/mygui.png Icons usually go into %{_datadir}/pixmaps (or under %{_iconsdir}/hicolor/). Or is this path hard-coded somewhere? > Icon=/usr/share/MYGUI/mygui.png The preferred way is to have no path, just the file name without the extension. If you use Icon=mygui_fe and put the icons in subdirectories like this: for size in 16 24 32 48 96 256 ; do install -Dpm644 Media/Common/Sources/Icons/MyGUI_Icon_FE_${size}x${size}.png %{buildroot}%{_iconsdir}/hicolor/${size}x${size}/apps/mygui_fe.png done then the desktop environment will pick the closest icon size automatically when displaying it. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_desktop_files .