Bug 489633
Summary: | Review Request: mingw32-physfs - MinGW Windows port of the PhysicsFS library | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Michael Ploujnikov <ploujj> |
Component: | Package Review | Assignee: | Richard W.M. Jones <rjones> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | low | ||
Version: | rawhide | CC: | fedora-mingw, fedora-package-review, itamar, lemenkov, notting, rjones, tcallawa |
Target Milestone: | --- | Flags: | rjones:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2009-06-16 07:57:34 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Michael Ploujnikov
2009-03-11 01:15:58 UTC
Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1240160 The %{_mingw32_configure} line needs to be moved down so it is in the %build section. Also you don't need %pre and %post, because ldconfig only cares about native Fedora binaries, and won't know what to do with *.DLLs. Just remove the %pre and %post sections completely. I will continue the review assuming that you have done the above. auto-buildrequires suggests the following extra BuildRequires lines: BuildRequires: mingw32-dlfcn BuildRequires: mingw32-readline BuildRequires: mingw32-zlib (A build might work without those, but configure might disable features if those packages aren't present. On the other hand, maybe those are false dependencies since auto-buildrequires isn't perfect). rpmlint output: mingw32-physfs.src:66: E: files-attr-not-set mingw32-physfs.src:67: E: files-attr-not-set mingw32-physfs.src:68: E: files-attr-not-set mingw32-physfs.src:69: E: files-attr-not-set mingw32-physfs.src:70: E: files-attr-not-set You are missing a %defattr line in the specfile mingw32-physfs.x86_64: W: devel-file-in-non-devel-package /usr/i686-pc-mingw32/sys-root/mingw/lib/libphysfs.dll.a mingw32-physfs.x86_64: W: spurious-executable-perm /usr/i686-pc-mingw32/sys-root/mingw/lib/libphysfs.dll.a mingw32-physfs.x86_64: W: devel-file-in-non-devel-package /usr/i686-pc-mingw32/sys-root/mingw/include/physfs.h mingw32-physfs.x86_64: W: binaryinfo-readelf-failed readelf: Error: Not an ELF file - it has the wrong magic bytes at the start mingw32-physfs.x86_64: W: non-standard-dir-in-usr i686-pc-mingw32 You can ignore these, as per http://fedoraproject.org/wiki/MinGW/Rpmlint - rpmlint output see comment 4 + package name satisfies the packaging naming guidelines + specfile name matches the package base name - package should satisfy packaging guidelines problems, see comment 2, comment 3, comment 4 + license meets guidelines and is acceptable to Fedora zlib + license matches the actual package license + %doc includes license file + spec file written in American English + spec file is legible ? upstream sources match sources in the srpm 9bab1ed6383958b8bc7db792c6989b01 663342 (upstream website was being very slow - I will check this later) + package successfully builds on at least one architecture n/a ExcludeArch bugs filed ? BuildRequires list all build dependencies see comment 3 n/a %find_lang instead of %{_datadir}/locale/* + binary RPM with shared library files must call ldconfig in %post and %postun this doesn't apply for mingw packages, see comment 2 + does not use Prefix: /usr + package owns all directories it creates + no duplicate files in %files - %defattr line see comment 4 + %clean contains rm -rf $RPM_BUILD_ROOT + consistent use of macros + package must contain code or permissible content n/a large documentation files should go in -doc subpackage + files marked %doc should not affect package n/a header files should be in -devel doesn't apply to mingw packages n/a static libraries should be in -static could consider creating a -static subpackage, but not vital n/a packages containing pkgconfig (.pc) files need 'Requires: pkgconfig' n/a libfoo.so must go in -devel n/a -devel must require the fully versioned base n/a packages should not contain libtool .la files n/a packages containing GUI apps must include %{name}.desktop file n/a packages must not own files or directories owned by other packages + %install must start with rm -rf %{buildroot} etc. + filenames must be valid UTF-8 Optional: n/a if there is no license file, packager should query upstream n/a translations of description and summary for non-English languages, if available + reviewer should build the package in mock + the package should build into binary RPMs on all supported architectures - review should test the package functions as described n/a scriptlets should be sane n/a pkgconfig files should go in -devel n/a shouldn't have file dependencies outside /etc /bin /sbin /usr/bin or /usr/sbin ------------------------ Please see comment 2, comment 3, comment 4. Also why are we not building the latest upstream version, which is 1.1.1? (In reply to comment #2) > The %{_mingw32_configure} line needs to be moved down so it > is in the %build section. > > Also you don't need %pre and %post, because ldconfig only cares > about native Fedora binaries, and won't know what to do with > *.DLLs. Just remove the %pre and %post sections completely. > > I will continue the review assuming that you have done the above. Done (In reply to comment #3) > auto-buildrequires suggests the following extra > BuildRequires lines: > > BuildRequires: mingw32-dlfcn This is definitely needed since physfs can use dlopen(). > BuildRequires: mingw32-readline This is only required for the test program > BuildRequires: mingw32-zlib Physfs can use an internal zlib if this isn't available. I suspect it would be better for security updates to have physfs use the system's zlib rather than its own. (In reply to comment #4) > rpmlint output: > > mingw32-physfs.src:66: E: files-attr-not-set > mingw32-physfs.src:67: E: files-attr-not-set > mingw32-physfs.src:68: E: files-attr-not-set > mingw32-physfs.src:69: E: files-attr-not-set > mingw32-physfs.src:70: E: files-attr-not-set > > You are missing a %defattr line in the specfile Fixed, although my rpmlint (0.85) didn't mention this. (In reply to comment #5) > Also why are we not building the latest upstream > version, which is 1.1.1? Version 1.1.1 uses cmake, which I haven't figured out how to use with mingw32 yet. Version 1.1.0 still uses autotools so I'll try creating a .spec file for that one, later. The reasons I chose 1.0.1 originally is because the latest Fedora development version still uses 1.0.1: http://cvs.fedoraproject.org/viewvc/devel/physfs/physfs.spec?revision=1.9&view=markup . I've updated http://plouj.com/rpmbuild/SPECS/mingw32-physfs.spec and http://plouj.com/rpmbuild/SRPMS/mingw32-physfs-1.0.1-11.fc10.src.rpm with all of the above fixes. Issues in comment 2 are FIXED. Issues in comment 3 are FIXED. Issue in comment 4 is FIXED. --------------------------------- This package is APPROVED by rjones --------------------------------- Plouj, you should continue the process with http://fedoraproject.org/wiki/CVSAdminProcedure (In reply to comment #8) Richard. have you sponsored the guy ? --------------------------------------------------- Michael Do you have a fedora account ? what's your FAS username ? if you don't have one please get one at the flowing link https://fedoraproject.org/wiki/PackageMaintainers/Join#Get_a_Fedora_Account after you get a FAS account please join the fedora packager group and post your username here, so Richard will sponsor you :-) I'm quite happy with sponsoring him. TBH I have no idea how the sponsorship process works though ... (In reply to comment #9) > Michael > > Do you have a fedora account ? what's your FAS username ? "plouj" > after you get a FAS account please join the fedora packager group and post your > username here, so Richard will sponsor you :-) I've just applied for the Fedora Packager CVS Commit Group (packager). (In reply to comment #11) > I've just applied for the Fedora Packager CVS Commit Group (packager). Michael, I've sponsored you based on Richard's recommendation. If you have any questions about the process or future packages, please do not hesitate to contact me. Also, as the physfs maintainer, we haven't bumped to the higher level releases because it is an API break and none of the dependent programs have moved to the newer physfs API yet. That said, if you need the newer one for your mingw work, I have no problem with it. Michael you should continue the process with https://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure#New_Packages New Package CVS Request ======================= Package Name: mingw32-physfs Short Description: MinGW Windows port of the PhysicsFS library Owners: plouj rjones Branches: F-10 InitialCC: cvs done. Plouj, next step is to import the package into Fedora's CVS, both the devel and F-10 branches. https://fedoraproject.org/wiki/PackageMaintainers/Join#Install_the_Client_Tools_.28Koji.29 Roughly speaking the commands are something like this. Please check them carefully. cd /tmp fedora-cvs mingw32-physfs cd mingw32-physfs/devel ../common/cvs-import name-of-the-SRPM cvs up make build cd ../F-10 ../common/cvs-import name-of-the-SRPM cvs up make build make update mingw32-physfs-1.0.1-11.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/mingw32-physfs-1.0.1-11.fc10 mingw32-physfs-1.0.1-11.fc10 has been pushed to the Fedora 10 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update mingw32-physfs'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-2840 For CMake issues (i.e. for the latest version), these should help: http://www.cmake.org/Wiki/CMake_Cross_Compiling http://techbase.kde.org/Getting_Started/Build/KDE4/Windows/Cross-Compiling mingw32-physfs-1.0.1-12.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/mingw32-physfs-1.0.1-12.fc10 mingw32-physfs-1.0.1-12.fc10 has been pushed to the Fedora 10 stable repository. If problems still persist, please make note of it in this bug report. This bug doesn't need to be in NEEDINFO any more. It seems that we should close this ticket (mingw32-physfs was built and pushed to updates). |