Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#4192 closed enhancement (wontfix)

[PATCH] Allow build with system tinygettext

Reported by: susnux Owned by:
Priority: Nice to Have Milestone:
Component: Build & Packages Keywords: patch
Cc: leper Patch:

Description

On some systems, at least openSUSE, tinygettext is available as a shared library through the package manager. So this will allow to link against the system library instead of the bundled one (will decrease binary size).

Attachments (1)

system-tinygettext.patch (3.3 KB ) - added by susnux 8 years ago.

Download all attachments as: .zip

Change History (6)

by susnux, 8 years ago

Attachment: system-tinygettext.patch added

comment:1 by Itms, 8 years ago

Cc: leper added
Milestone: Alpha 21Alpha 22

Thanks for the patch!

I don't know whether this is desirable, since we compile a very slightly modified version of tinygettext directly inside our engine.

I'm adding leper to the CC list of this ticket, if he has time to take a look he would have a far better informed opinion on the subject.

In any case, we are close to a new alpha release of the game, so such changes to the build process are a bit too radical right now. I'm pushing this ticket to the next milestone.

comment:2 by leper, 8 years ago

Keywords: rfc removed
Milestone: Alpha 22
Resolution: wontfix
Status: newclosed

tinygettext is meant to be included as source and built with the same compilation flags as the program (given that it uses STL types in the API not using the same flags is likely to lead to breakage in the ABI; also allocation of types on both sides of the program-lib barrier being passed to the other side). So this patch is just asking for trouble at some point down the line which will be bugs that are hard to debug.

The included version isn't really modified from upstream, apart from providing some function for Windows (not that we actually need that part, given we don't use the related part of the code), and adding required headers that ensure building with the same settings on Windows. The included version is probably a few commits behind upstream, but none of those are changing or fixing things we care about or likely to break our usage, so no need to update.

Given that the patch touches a few lines that disable a few warnings I'd prefer to remove those and actually fix them.

(Speaking as both downstream and upstream.)

in reply to:  2 comment:3 by Itms, 8 years ago

Thanks for the input!

Replying to leper:

Given that the patch touches a few lines that disable a few warnings I'd prefer to remove those and actually fix them.

When fiddling around with VS2015 support I noticed that most of those warnings don't actually appear. When I have my changes ready I'll remove the unnecessary disablings and I'll make sure to report the remaining warnings.

comment:5 by Itms, 8 years ago

Yes :)

Note: See TracTickets for help on using tickets.