bundled physfs version

[GoogleCodeExporter]

Hedgewars bundles a copy of physfs with almost no information on
a) which exact version/revision (is it 2.0.3 or a specific mercurial revision?)
b) what changes have been applied that are _not_ upstream (those should be 
available in plain patch-form as well)
c) link to a bug at upstream tracker which requests those changes to be merged

Let me explain why bundling libraries is a QA issue.
d) security: bundled libraries are somehow out of the scope of packagers. As an 
example: zlib had many severe vulnerabilities and if packages bundle copies of 
zlib, then every single one of it needs to be fixed _manually_. If packages 
link to it, we just fix up the system lib and are done.
These things are difficult to track and will cause security issues on the long 
run, cause developers/packagers and users might not even be aware that they are 
affected.
e) maintenance: for you and for us. You modify it, we have to track down 
security issues, QA issues, build time issues and plain bugs not just for the 
system lib, but also for every modified copy in every possible package. So that 
you not think I'm talking stuff... this already happens for us, e.g. with 
Irrlicht or premake or lua which are often bundled. I also have to report bugs 
in several places instead of one, because I cannot be sure if you merge fixes 
regularly from upstream.
f) support: experience shows that people who bundle stuff don't accept bug 
reports for their bundled libraries although they might have modified them 
heavily (popular example is freeorion which "forked" gigi, but does not support 
it at all). Take it up with upstream they say. When I do it, upstream will tell 
me: erm, we don't support random forks. Maybe your version even introduces new 
bugs. No one will be responsible. This decreases support.
g) size: increased size, both in memory and on the disk, obvious
h) breakage: it happens from time to time that some packagers carelessly 
unbundle libraries without contacting upstream first and thus (partly) breaking 
the application for the end-user (this happened with supertuxkart on some 
distros afair). This is also partly upstreams fault, especially when there is 
no information on a,b,c thus indicating that it is just bundled for convenience.

Bundled libraries are no blockers that prevent stuff to get into distros (at 
least I can say this for gentoo), but it's a bug (most of the time).

Original issue reported on code.google.com by julian.o...@googlemail.com on 20 Feb 2013 at 7:41

• Blocked on: #532

[GoogleCodeExporter]

Speaking for unc0rr here, but bundling is not something we are a fan of, which 
is why we try to use system liblua if it exists.

But, I believe at the time he began adding physfs, he required a feature that 
was not yet in release.

Original comment by kyberneticist@gmail.com on 20 Feb 2013 at 7:47

[GoogleCodeExporter]

I am just wondering if there was a feature request already about that at physfs 
upstream.

At least I don't see a bug report
https://bugzilla.icculus.org/describecomponents.cgi?product=PhysicsFS

Original comment by julian.o...@googlemail.com on 20 Feb 2013 at 7:49

[GoogleCodeExporter]

... I mean. not in the released version of physfs.  just beta.

I'll let him speak to it tho.

Original comment by kyberneticist@gmail.com on 20 Feb 2013 at 7:50

[GoogleCodeExporter]

if other packagers come across this:

a) https://hg.icculus.org/icculus/physfs/rev/e7dad5b51954
b) no changes, so it's perfectly safe to unbundle
c) -

In order to resolve this bug I think a cmake option to switch between 
bundled/system physfs would be the right way to go. I advise against automagic 
detection, because it decreases the level of control over the build process and 
can have unexpected side effects depending on the distribution. Make the 
default the bundled version for now, since that is not yet released.

Also, put a README inside misc/ folder explaining the situation of each bundled 
lib. That makes it easier for packagers and users to deal with it.

I will try to provide a patch for this when I have time for testing, but this 
also correlates with bug 532.

Original comment by julian.o...@googlemail.com on 20 Feb 2013 at 10:14

[GoogleCodeExporter]

actually comment #4 is not entirely correct

there are custom files in "misc/physfs/extras" which makes separation difficult

Original comment by julian.o...@googlemail.com on 21 Feb 2013 at 3:37

[GoogleCodeExporter]

This fixes it, tested on linux only. (not entirely sure about the ANDROID logic)

Patch is applied on top of patch from bug 532

Original comment by julian.o...@googlemail.com on 21 Feb 2013 at 4:27

Attachments:

• hedgewars-9999-unbundle-physfs.patch

[GoogleCodeExporter]

Why is automagic detection a bad idea?
Right now in lua we do this:

1- you pass the LUA library path
2- if not present, detection starts
3- if not found, bundled lua gets build

so far worked quite good, esp on system that do not have packaging system 
(win/osx)
maybe we could do something similar for physfs but we need at least new 
FindPhysfs.cmake file

Original comment by vittorio...@gmail.com on 21 Feb 2013 at 8:11

[GoogleCodeExporter]

a) problems get hidden if the external library is not found for whatever reason 
(e.g. a bug in the FindFoo.cmake file). The user will not get the result he 
expects.
I want the build to fail if I say "use my external library" and the build 
system is unable to find it.
b) there may be cases where the external library is available, but broken, not 
just because of version mismatch, but also because of different build-time 
configuration, so a user might want to use the internal library (that happens 
often with lua/luajit and special preprocessor flags), but the build system 
will not allow this
c) automagic stuff is almost always an annoyance when packaging. In many cases 
we have to patch it (e.g. options that are not toggable, but are just enabled 
if certain libraries are found, but that's not the case here).
I prefer build systems that try not to be too smart and just do what I tell 
them and with sane defaults.

Btw. if you just bundle libraries for convenience and without technical reasons 
like modifications or special build time configuration, then you could also 
provide a tarball like: hedgewars-nodeps-0.9.19.tar.gz
which is stripped of all that stuff.

Original comment by julian.o...@googlemail.com on 21 Feb 2013 at 2:26

[GoogleCodeExporter]

I'd say the main reason is that while it is *unlikely* they would change 
something in an unreleased version without bumping the 2.1.0 version, it can't 
be ruled out.

So, it is hard to know which version of physfs would be safe at this point.

Original comment by kyberneticist@gmail.com on 21 Feb 2013 at 4:44

[GoogleCodeExporter]

By the way, could we shed some light on why we require 2.1.0?

Original comment by vittorio...@gmail.com on 21 Feb 2013 at 4:55

[GoogleCodeExporter]

hasufell, can you try the libphysfs branch?

It's doesn't yet fix the version check or the presence of an older physfs but 
it should correctly link an external libphyfs library and not interfere with 
our extra functions.

Original comment by vittorio...@gmail.com on 22 Feb 2013 at 9:15

• Changed state: Started

[GoogleCodeExporter]

I guess you mean physfslayer branch and that still fails to a) solve bug 532 
and b) does not let me choose which lib to pick, so does not fix this bug 
either.

Original comment by julian.o...@googlemail.com on 22 Feb 2013 at 5:04

[GoogleCodeExporter]

Original comment by vittorio...@gmail.com on 22 Feb 2013 at 5:09

• Now blocked on: #532

[GoogleCodeExporter]

Ok I think rd610e692e2f6 addresses this:

- cmake looks for a PhysFS library
* if you don't like automagic, you can pass PHYSFS_LIBRARY and 
PHYSFS_INCLUDE_DIR to skip it
- when the library is found, cmake performs a version checking
- if the lib is not new enough cmake builds the bundled version

For reference of packagers stumbling into this:
a) right now there is no code modification in our bundled physfs;
b) there is no patch against stable physfs because you can just pull the latest 
tip from physfs repository;
c) it's not an optional change, as in 2.1.0 a few slimmer APIs were introduced 
and our wrappers have been designed around that;
d) minimum required physfs version has always been outlined in INSTALL file; 
I've made this more evident adding a few cmake output lines and editing the 
aforementioned file (in r0d3f4a731233).

If no objections arise, I'd like to close this bug.
Testing is more that welcome.

Original comment by vittorio...@gmail.com on 24 Feb 2013 at 10:21

[GoogleCodeExporter]

Please also see raf104e59ea8e in which we add an explicit flag for this.

Original comment by vittorio...@gmail.com on 24 Feb 2013 at 11:26

[GoogleCodeExporter]

the branch got merged, so this is fixed in raf104e59ea8e

Original comment by vittorio...@gmail.com on 27 Feb 2013 at 10:02

• Changed state: Fixed