Discussion:
Bug#948760: berusky2: Compile without warnings
Add Reply
Asher Gordon
2020-01-13 03:00:02 UTC
Reply
Permalink
Package: berusky2
Version: 0.10+git20170630-3
Severity: normal
Tags: patch

Dear Maintainer,

Currently when Berusky2 is compiled it generates a *lot* of compile
warnings, some of which seem serious. I've fixed all these warnings and
have attached a patch (I've attached it as an attachment rather than
inline because it is quite large (3651 lines) and I don't want to
clutter up the bug report log).

I've set the severity to "normal" since there are so many warnings and I
suspect that my patch may fix some crashes. But feel free to downgrade
as you see fit.

Here is a short summary (not meant to be comprehensive) of the changes I
have made:

* Some functions (such as chdir() and getcwd()) warn if their return
values are not checked. I have added checks to all such functions
which did not already have checks.

* I've added a few assertions where needed to avoid warnings.

* I've changed many calls to sprintf() which may overflow to calls to
snprintf(). I also check the return value of the snprintf() call
since the output may be truncated.

* I've replaced the deprecated ALUT functions alutLoadWAVFile() and
alutUnloadWAV() with local implementations (s/alut/adas/).

* Apparently G++ doesn't like calls to memset() on a non-trivial type
(all of these were structs). So I've casted the pointer to (void *)
before passing it to memset(). Is there a better way to do this? I
don't know C++ very well (the warning said to "use assignment or
value-initialization instead").

* Numerous other miscellaneous fixes.

The coding style of Berusky2 is inconsistent, so I just tried to use the
local style in whatever file (or part of file!) I was editing.

I finally (sort of) figured out how to use quilt properly, and I've
added a DEP-3 compliant header to my patch. Please remember to update
the Bug-Debian (this bug), Reviewed-by (you), and Last-Update headers
before you add the patch.

Since Berusky2 now compiles without warnings, I recommend adding -Werror
to the C{,XX}FLAGS. I guess this would be done by adding it to
DEB_CXXFLAGS_MAINT_APPEND (like you did in 89e7190) and
DEB_CFLAGS_MAINT_APPEND.

I tested running Berusky2 periodically while writing the patch to make
sure that I didn't introduce any bugs. I also tested it after I finished
writing the patch. But I did not test very thoroughly (I just started it
and made a few moves in a level). However, the only part that seems
likely to introduce new bugs is the replacement of the deprecated ALUT
functions, and sound still works fine. So I'm pretty sure I didn't
introduce any new bugs.

I have attached the patch after the message.

Thanks,
Asher
--
...very few phenomena can pull someone out of Deep Hack Mode, with two
noted exceptions: being struck by lightning, or worse, your *computer*
being struck by lightning.
-- Matt Welsh

GPG fingerprint: 38F3 975C D173 4037 B397 8095 D4C9 C4FC 5460 8E68


-- System Information:
Debian Release: bullseye/sid
APT prefers testing-debug
APT policy: (500, 'testing-debug'), (500, 'testing')
Architecture: amd64 (x86_64)

Kernel: Linux 5.4.0-2-amd64 (SMP w/2 CPU cores)
Kernel taint flags: TAINT_FIRMWARE_WORKAROUND
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8), LANGUAGE=en_US.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled

Versions of packages berusky2 depends on:
ii berusky2-data 0.9-2
ii libalut0 1.1.0-6
ii libc6 2.29-7
ii libgcc1 1:9.2.1-22
ii libgl1 1.1.0-1+b1
ii libglu1-mesa [libglu1] 9.0.1-1
ii libopenal1 1:1.19.1-1+b1
ii libsdl-image1.2 1.2.12-12
ii libsdl1.2debian 1.2.15+dfsg2-5
ii libstdc++6 9.2.1-22
ii libvorbisfile3 1.3.6-2
ii libx11-6 2:1.6.8-1
ii zlib1g 1:1.2.11.dfsg-1+b1

berusky2 recommends no packages.

berusky2 suggests no packages.

-- no debconf information
Markus Koschany
2020-01-13 18:20:02 UTC
Reply
Permalink
Post by Asher Gordon
Package: berusky2
Version: 0.10+git20170630-3
Severity: normal
Tags: patch
Dear Maintainer,
Currently when Berusky2 is compiled it generates a *lot* of compile
warnings, some of which seem serious. I've fixed all these warnings and
have attached a patch (I've attached it as an attachment rather than
inline because it is quite large (3651 lines) and I don't want to
clutter up the bug report log).
I've set the severity to "normal" since there are so many warnings and I
suspect that my patch may fix some crashes. But feel free to downgrade
as you see fit.
Hello,

thanks for the report. Have you considered to forward these patches
upstream and help upstream to implement them? I'm asking because a lot
of users won't benefit from bug fixes if they are only fixed in Debian.
Post by Asher Gordon
Since Berusky2 now compiles without warnings, I recommend adding -Werror
to the C{,XX}FLAGS. I guess this would be done by adding it to
DEB_CXXFLAGS_MAINT_APPEND (like you did in 89e7190) and
DEB_CFLAGS_MAINT_APPEND.
I think -Werror is never a good idea for Debian packages. Even minor
warnings would cause a build failure. Ideally -Werror is used in
development to detect and fix warnings but not in production. Again this
is something to consider for upstream.
Post by Asher Gordon
I tested running Berusky2 periodically while writing the patch to make
sure that I didn't introduce any bugs. I also tested it after I finished
writing the patch. But I did not test very thoroughly (I just started it
and made a few moves in a level). However, the only part that seems
likely to introduce new bugs is the replacement of the deprecated ALUT
functions, and sound still works fine. So I'm pretty sure I didn't
introduce any new bugs.
I have attached the patch after the message.
I would feel more comfortable if upstream included and reviewed the
patch. It may take a while to find the time to review it myself.

Regards,

Markus
Bernhard Übelacker
2020-01-13 21:40:02 UTC
Reply
Permalink
Hello Asher,
maybe you want to incorporate the changes given here:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=944431#31
Unfortunately I was too late there.

Then the call to e.g. SetThreadPriority would not needed to get
commented out, and the "-O0" fix in #944431 could be removed
again, I guess.

Kind regards,
Bernhard
Markus Koschany
2020-01-13 22:30:01 UTC
Reply
Permalink
Post by Bernhard Übelacker
Hello Asher,
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=944431#31
Unfortunately I was too late there.
Then the call to e.g. SetThreadPriority would not needed to get
commented out, and the "-O0" fix in #944431 could be removed
again, I guess.
I had incorporated and applied your patch and removed the -O0 fix again.
I haven't checked though how all this is related to Asher's patch.

Regards,

Markus
Asher Gordon
2020-01-14 02:30:01 UTC
Reply
Permalink
Hello Markus and Bernhard,
Post by Markus Koschany
Post by Bernhard Übelacker
Hello Asher,
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=944431#31
Unfortunately I was too late there.
Then the call to e.g. SetThreadPriority would not needed to get
commented out, and the "-O0" fix in #944431 could be removed
again, I guess.
I had incorporated and applied your patch and removed the -O0 fix again.
I haven't checked though how all this is related to Asher's patch.
As Markus says, he's already added Bernhard's fixes. I've written my
patch on top of those fixes.
Post by Markus Koschany
thanks for the report. Have you considered to forward these patches
upstream and help upstream to implement them? I'm asking because a lot
of users won't benefit from bug fixes if they are only fixed in Debian.
I've forked the upstream repository here[1] (because I don't want to use
GitHub) and I've sent Martin Stransky an email to let him know. In my
repository, I've added Markus's patch for GCC 6 and Bernhard's patch for
the 'no return statement in function returning non-void' warning as well
as my patches. I've added both of these as separate commits where I've
credited you in the commit message. But if you like, I can change the
Author to your name instead.

Hopefully, these will be accepted upstream and we will be able to drop
all patches except the Debian-specific ones (data.patch and docs.patch).

Asher


Footnotes:
[1] https://notabug.org/AsDaGo/berusky2
--
If at first you don't succeed, redefine success.

GPG fingerprint: 38F3 975C D173 4037 B397 8095 D4C9 C4FC 5460 8E68
Bernhard Übelacker
2020-01-14 08:30:02 UTC
Reply
Permalink
Hello Asher, hello Markus,
sorry, I wasn't aware of that patch being applied at Salsa as
there was no more activity in 944431, so I thought it went
through unnoticed.

Sorry for the noise.

Kind regards,
Bernhard

Loading...