Bug 489633 - Review Request: mingw32-physfs - MinGW Windows port of the PhysicsFS library
Review Request: mingw32-physfs - MinGW Windows port of the PhysicsFS library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Richard W.M. Jones
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-10 21:15 EDT by Michael Ploujnikov
Modified: 2009-06-16 03:57 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-06-16 03:57:34 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rjones: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Michael Ploujnikov 2009-03-10 21:15:58 EDT
Spec URL: http://plouj.com/rpmbuild/SPECS/mingw32-physfs.spec
SRPM URL: http://plouj.com/rpmbuild/SRPMS/mingw32-physfs-1.0.1-11.fc10.src.rpm

Hi! I just finished packaging physfs for the Fedora/mingw32 project (http://fedoraproject.org/wiki/SIGs/MinGW). This is my first package and I need a sponsor.

Description: 
PhysicsFS is a library to provide abstract access to various archives. It is
intended for use in video games, and the design was somewhat inspired by Quake
3's file subsystem. The programmer defines a "write directory" on the physical
filesystem. No file writing done through the PhysicsFS API can leave that
write directory, for security. For example, an embedded scripting language
cannot write outside of this path if it uses PhysFS for all of its I/O, which
means that untrusted scripts can run more safely. Symbolic links can be
disabled as well, for added safety. For file reading, the programmer lists
directories and archives that form a "search path". Once the search path is
defined, it becomes a single, transparent hierarchical filesystem. This makes
for easy access to ZIP files in the same way as you access a file directly on
the disk, and it makes it easy to ship a new archive that will override a
previous archive on a per-file basis. Finally, PhysicsFS gives you
platform-abstracted means to determine if CD-ROMs are available, the user's
home directory, where in the real filesystem your program is running, etc.
Comment 1 Richard W.M. Jones 2009-03-13 11:54:55 EDT
Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1240160
Comment 2 Richard W.M. Jones 2009-03-13 12:05:38 EDT
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.
Comment 3 Richard W.M. Jones 2009-03-13 12:18:32 EDT
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).
Comment 4 Richard W.M. Jones 2009-03-13 12:20:48 EDT
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
Comment 5 Richard W.M. Jones 2009-03-13 12:30:04 EDT
- 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?
Comment 6 Michael Ploujnikov 2009-03-14 13:18:05 EDT
(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.
Comment 7 Richard W.M. Jones 2009-03-16 08:01:51 EDT
Issues in comment 2 are FIXED.

Issues in comment 3 are FIXED.

Issue in comment 4 is FIXED.

---------------------------------
This package is APPROVED by rjones
---------------------------------
Comment 8 Richard W.M. Jones 2009-03-16 08:03:23 EDT
Plouj, you should continue the process with
http://fedoraproject.org/wiki/CVSAdminProcedure
Comment 9 Itamar Reis Peixoto 2009-03-16 08:19:59 EDT
(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 :-)
Comment 10 Richard W.M. Jones 2009-03-16 08:41:17 EDT
I'm quite happy with sponsoring him.  TBH I have no idea how
the sponsorship process works though ...
Comment 11 Michael Ploujnikov 2009-03-16 08:49:09 EDT
(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).
Comment 12 Tom "spot" Callaway 2009-03-16 10:39:02 EDT
(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.
Comment 13 Tom "spot" Callaway 2009-03-16 10:41:49 EDT
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.
Comment 14 Itamar Reis Peixoto 2009-03-16 10:53:57 EDT
Michael you should continue the process with

https://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure#New_Packages
Comment 15 Michael Ploujnikov 2009-03-16 13:28:06 EDT
New Package CVS Request
=======================
Package Name: mingw32-physfs
Short Description: MinGW Windows port of the PhysicsFS library
Owners: plouj rjones
Branches: F-10
InitialCC:
Comment 16 Kevin Fenzi 2009-03-17 01:10:29 EDT
cvs done.
Comment 17 Richard W.M. Jones 2009-03-17 04:19:54 EDT
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
Comment 18 Fedora Update System 2009-03-17 22:34:38 EDT
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
Comment 19 Fedora Update System 2009-03-18 15:13:11 EDT
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
Comment 20 Kevin Kofler 2009-03-18 16:50:27 EDT
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
Comment 21 Fedora Update System 2009-03-23 21:07:57 EDT
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
Comment 22 Fedora Update System 2009-04-21 20:59:03 EDT
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.
Comment 23 Richard W.M. Jones 2009-06-06 11:26:41 EDT
This bug doesn't need to be in NEEDINFO any more.
Comment 24 Peter Lemenkov 2009-06-16 03:57:34 EDT
It seems that we should close this ticket (mingw32-physfs was built and pushed to updates).

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