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 .
I found the icons in the tar.gz, and have installed them appropriately. I also added cmake tests at the end of the spec file, though it seems like MyGUI has no cmake tests predefined. Regardless, here's the newest version of the spec file. SPEC: https://github.com/Chapien/mygui-copr/raw/refs/heads/main/sources/mygui.spec Let me know if there's anything else. [fedora-review-service-build]
Created attachment 2093918 [details] The .spec file difference from Copr build 9157212 to 9160748
Copr build: https://copr.fedorainfracloud.org/coprs/build/9160748 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2371328-mygui/fedora-rawhide-x86_64/09160748-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.
I mislabeled the Image Editor as the Skin Editor in the .desktop file. Fixed that just now. No changes to the spec file, but will trigger a rebuild regardless. SPEC: https://github.com/Chapien/mygui-copr/blob/main/sources/mygui.spec [fedora-review-service-build]
Copr build: https://copr.fedorainfracloud.org/coprs/build/9168862 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2371328-mygui/fedora-rawhide-x86_64/09168862-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.
Please sort BuildRequires: alphabetically. > Requires: mesa-libGL Can you justify this? rpmbuild already generates: Requires: libOpenGL.so.0()(64bit) > Requires: sdl2-compat This is redundant, because rpmbuild already generates: Requires: libSDL2-2.0.so.0()(64bit) > install -m 0755 %{SOURCE2} %{buildroot}%{_bindir}/MyGUI-Tools The above should include the -p argument to preserve the timestamp of the source file for reproducibility. > Requires: pkgconfig, ois-devel, mesa-libGL-devel Please split into separate lines and sort alphabetically. > %cmake -G Ninja \ > -DMYGUI_INSTALL_PDB=FALSE -DMYGUI_BUILD_DEMOS=FALSE -DMYGUI_BUILD_PLUGINS=OFF \ > -DMYGUI_DONT_USE_OBSOLETE=ON -DMYGUI_BUILD_TOOLS=TRUE -DMYGUI_USE_SYSTEM_GLEW=TRUE \ > -DMYGUI_RENDERSYSTEM=4 -DMYGUI_INSTALL_TOOLS=TRUE -DMYGUI_BUILD_DOCS=TRUE -DMYGUI_INSTALL_DOCS=TRUE \ > -DMYGUI_INSTALL_DEMOS=FALSE I recommend putting each CMake option in a separate line and sorting alphabetically for readability. > cd %{_vpath_builddir} > pushd Docs > doxygen > popd Is that the recommended upstream way of building docs? Docs/CMakeLists.txt seems to have "api-docs" target, so you should be able to run it. > %{_libdir}/*.so.* This must be explicitly listed to avoid accidental SONAME bumps. > %files devel > %{_includedir}/* > %{_libdir}/*.so > %{_libdir}/pkgconfig/*.pc These should be listed explicitly as well. Please avoid using wildcards as much as possible. > %{_iconsdir}/hicolor Same here. Please use something like: %{_iconsdir}/hicolor/*/apps/mygui_*.png
(In reply to Dominik 'Rathann' Mierzejewski from comment #18) > Please sort BuildRequires: alphabetically. > > > Requires: mesa-libGL > > Can you justify this? I switched from building with Ogre to building with openGL. I'm not sure if openGL comes with Fedora, or if this is the right package to require for openGL compatibility. Let me know. > rpmbuild already generates: > Requires: libOpenGL.so.0()(64bit) > > > Requires: sdl2-compat > > This is redundant, because rpmbuild already generates: > Requires: libSDL2-2.0.so.0()(64bit) Will remove it. > > install -m 0755 %{SOURCE2} %{buildroot}%{_bindir}/MyGUI-Tools > > The above should include the -p argument to preserve the timestamp of the > source file for reproducibility. Understood. > > Requires: pkgconfig, ois-devel, mesa-libGL-devel > > Please split into separate lines and sort alphabetically. Will do. > > %cmake -G Ninja \ > > -DMYGUI_INSTALL_PDB=FALSE -DMYGUI_BUILD_DEMOS=FALSE -DMYGUI_BUILD_PLUGINS=OFF \ > > -DMYGUI_DONT_USE_OBSOLETE=ON -DMYGUI_BUILD_TOOLS=TRUE -DMYGUI_USE_SYSTEM_GLEW=TRUE \ > > -DMYGUI_RENDERSYSTEM=4 -DMYGUI_INSTALL_TOOLS=TRUE -DMYGUI_BUILD_DOCS=TRUE -DMYGUI_INSTALL_DOCS=TRUE \ > > -DMYGUI_INSTALL_DEMOS=FALSE > > I recommend putting each CMake option in a separate line and sorting > alphabetically for readability. Makes sense. > > > cd %{_vpath_builddir} > > pushd Docs > > doxygen > > popd > > Is that the recommended upstream way of building docs? Docs/CMakeLists.txt > seems to have "api-docs" target, so you should be able to run it. This is how the old package did it, so I was just reimplementing that because I couldn't get the docs to build properly using the CMAKE target. > > %{_libdir}/*.so.* > > This must be explicitly listed to avoid accidental SONAME bumps. Will do. > > %files devel > > %{_includedir}/* > > %{_libdir}/*.so > > %{_libdir}/pkgconfig/*.pc > > These should be listed explicitly as well. Please avoid using wildcards as > much as possible. It's a lot of files to list, but I'll go through them all appropriately. > > %{_iconsdir}/hicolor > > Same here. Please use something like: > %{_iconsdir}/hicolor/*/apps/mygui_*.png Thank you for all of the feedback. I can't quite make changes right now, but I wanted to update you that I had seen it (and ask my openGL question!). I'll probably make the necessary changes tonight or tomorrow -- I have a dayjob that's requiring a bit of my attention recently, but I promise to get to it ASAP!
Alright, I've made the necessary changes that you've requested, except for the docs build process due to CMAKE issues. Hopefully all looks good now. SPEC: https://github.com/Chapien/mygui-copr/raw/refs/heads/main/sources/mygui.spec SRPM: https://github.com/Chapien/mygui-copr/raw/refs/heads/main/mygui-3.4.3-4.fc42.src.rpm
Broke the spec file by accident (extra space meant CMAKE flags were missed). Should be fixed now. SPEC: https://github.com/Chapien/mygui-copr/raw/refs/heads/main/sources/mygui.spec SRPM: https://github.com/Chapien/mygui-copr/raw/refs/heads/main/mygui-3.4.3-4.fc42.src.rpm
[fedora-review-service-build]
(In reply to Dominik 'Rathann' Mierzejewski from comment #22) > [fedora-review-service-build] Is the review service broken?
> BuildRequires: libGLEW Unnecessary. BR: glew-devel should pull it in. > %{_libdir}/libMyGUICommon.so.* > %{_libdir}/libMyGUIEngine.so.* This is still insufficient. You need explicit: %{_libdir}/libMyGUICommon.so.%{version} %{_libdir}/libMyGUIEngine.so.%{version} Otherwise you risk unnoticed SONAME changes which will break dependent packages. It looks like the SONAME will change even with micro version bump. Does upstream make any promises regarding ABI compatibility?
(In reply to Dominik 'Rathann' Mierzejewski from comment #24) > > BuildRequires: libGLEW > > Unnecessary. BR: glew-devel should pull it in. Fixed. > > %{_libdir}/libMyGUICommon.so.* > > %{_libdir}/libMyGUIEngine.so.* > > This is still insufficient. You need explicit: > %{_libdir}/libMyGUICommon.so.%{version} > %{_libdir}/libMyGUIEngine.so.%{version} > > Otherwise you risk unnoticed SONAME changes which will break dependent > packages. > > It looks like the SONAME will change even with micro version bump. > Does upstream make any promises regarding ABI compatibility? Fixed. MyGUI does not update very often, so I don't think there will be many micro version bumps. Upstream is somewhat slow to reply from what I've been told, but I should be able to reach out. That said, it seems that it has not been a problem before; MyGUI was abandoned by Fedora before it even added OpenGL support, and the burden of building with OGRE seems to be one of the major reasons that it was dropped as opposed to any compatibility issues. SPEC: https://github.com/Chapien/mygui-copr/raw/refs/heads/main/sources/mygui.spec SRPM: https://github.com/Chapien/mygui-copr/raw/refs/heads/main/mygui-3.4.3-5.fc42.src.rpm
LGTM now, package approved. You might want to consider working with upstream to migrate off GLEW to GLAD or libepoxy. See this thread (once Fedora services are back after DC move): https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/AS2LPDXVE4ZQGGENAN4W6ST42NAPEOKG/ Some relevant links from that e-mail: https://github.com/Dav1dde/glad https://gitlab.com/kicad/code/kicad/-/issues/20647 https://github.com/prusa3d/PrusaSlicer/pull/14440 https://github.com/flightlessmango/MangoHud/blob/d416a8314f87910f3c37f15347ebfa5918bac9c2/src/meson.build#L107
Claire, you are now sponsored. Please go ahead and follow the rest of the unretirement process: https://docs.fedoraproject.org/en-US/package-maintainers/Package_Retirement_Process/#claiming .
FEDORA-2025-d80a82fc61 (mygui) has been submitted as an update to Fedora 42. https://bodhi.fedoraproject.org/updates/FEDORA-2025-d80a82fc61
FEDORA-2025-f92881e560 (mygui) has been submitted as an update to Fedora 41. https://bodhi.fedoraproject.org/updates/FEDORA-2025-f92881e560
FEDORA-2025-d80a82fc61 has been pushed to the Fedora 42 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2025-d80a82fc61 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2025-d80a82fc61 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2025-f92881e560 has been pushed to the Fedora 41 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2025-f92881e560 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2025-f92881e560 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2025-d80a82fc61 (mygui) has been pushed to the Fedora 42 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2025-f92881e560 (mygui) has been pushed to the Fedora 41 stable repository. If problem still persists, please make note of it in this bug report.