Bug 2371328
| Summary: | Review Request: mygui - Unretire mygui with newest version for f41+ | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Claire Robsahm <inquiries> | ||||||
| Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> | ||||||
| Status: | NEW --- | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||||
| Severity: | medium | Docs Contact: | |||||||
| Priority: | unspecified | ||||||||
| Version: | rawhide | CC: | dominik, fedora, package-review | ||||||
| Target Milestone: | --- | ||||||||
| Target Release: | --- | ||||||||
| Hardware: | All | ||||||||
| OS: | Linux | ||||||||
| URL: | http://mygui.info/ | ||||||||
| Whiteboard: | |||||||||
| Fixed In Version: | Doc Type: | --- | |||||||
| Doc Text: | Story Points: | --- | |||||||
| Clone Of: | Environment: | ||||||||
| Last Closed: | Type: | --- | |||||||
| Regression: | --- | Mount Type: | --- | ||||||
| Documentation: | --- | CRM: | |||||||
| Verified Versions: | Category: | --- | |||||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||||
| Embargoed: | |||||||||
| Bug Depends On: | |||||||||
| Bug Blocks: | 177841, 1364745 | ||||||||
| Attachments: |
|
||||||||
|
Description
Claire Robsahm
2025-06-09 23:26:20 UTC
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. |