| View Issue Details [ Jump to Notes ] | [ Issue History ] [ Print ] | ||||||||||||
| ID | Project | Category | View Status | Date Submitted | Last Update | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 0005058 | Spring engine | Linux | public | 2016-02-07 04:39 | 2017-07-07 21:40 | ||||||||
| Reporter | Kip | ||||||||||||
| Assigned To | abma | ||||||||||||
| Priority | normal | Severity | feature | Reproducibility | always | ||||||||
| Status | assigned | Resolution | open | ||||||||||
| Product Version | 100.0+git | ||||||||||||
| Target Version | Fixed in Version | ||||||||||||
| Summary | 0005058: libstreflop-dev now available for testing on Ubuntu | ||||||||||||
| Description | Hey everyone, I noticed the streflop library is used by the Spring RTS engine. Several other projects also use it. The problem is that for Debian based distributions, such as Ubuntu, developers have had to rely on building streflop locally within their source tree because it has not been packaged properly for them. I am happy to share with you my PPA for streflop which is currently under testing. Please consider testing building against this library in the usual way with pkg-config. With enough feedback and successful usage, I will have it become an official Debian package. https://launchpad.net/~kip/+archive/ubuntu/streflop To test, run the following on Ubuntu (wily only at this time): $ sudo add-apt-repository ppa:kip/streflop $ sudo apt-get update $ sudo apt-get install libstreflop-dev What I suggest you do is test to see if the library is already available on the user's system via pkg-config during build time, and if so, use the system installation. If not, then revert back to building your local version. | ||||||||||||
| Tags | No tags attached. | ||||||||||||
| Checked infolog.txt for Errors | |||||||||||||
| Attached Files |
| ||||||||||||
 Relationships |
|
 Relationships |
| Notes |
|
|
jK (developer) 2016-02-07 20:21 |
I am unsure if that works, cause streflop has multiple compile time flags, in particular: STREFLOP_SOFT STREFLOP_X87 STREFLOP_SSE (that's what spring uses) Also other compile time flags matter (i.e. if you enable sse2 or not). |
|
Kip (reporter) 2016-02-07 20:23 |
Hey jK. There are separate pkg-config files provided for streflop-x87, streflop-x87-nd, streflop-sse, streflop-sse-nd, and streflop-soft, so no problem. |
|
abma (administrator) 2016-02-07 21:17 |
is there an upstream (distribution independent) project for feedback/patches? the original project didn't touch the code for ~10 years it seems. we changed a lot of stuff: https://github.com/spring/spring/commits/develop/rts/lib/streflop so very likely changes are needed. |
|
Kip (reporter) 2016-02-07 21:20 |
Hey abma. The upstream project is maintained by Nicolas Brodu here: http://nicolas.brodu.net/en/programmation/streflop/ He doesn't have much time to spend on it these days, so if you can, please send me a unified diff of your changes against his last stable release (0.3) and I can liaise with him as I did this morning and try to have them merged. Nicolas's email address, if you prefer to contact him directly, is nicolas at brodu dot net. |
|
abma (administrator) 2016-02-07 21:41 |
1. i don't see someone merging these changes to streflop: https://github.com/abma/streflop/compare/master...spring :-| 2. especially this made "our" streflop incompatible: https://github.com/spring/spring/commit/e333f248398af03a55b6f956efefa062aff8a577 and i don't see us reverting it... |
|
jK (developer) 2016-02-07 21:43 |
"we changed a lot of stuff" Not really, most things affect streflop_cond.h which is a spring wrapper one, and so doesn't belong to streflop. Streflop itself is nearly untouched (a few structs were renamed to fix conflicts). |
|
abma (administrator) 2016-02-07 21:51 |
oh, indeed so, should be doable. streflop_cond.h isn't in streflop.h, so should be no problem. |
|
Kip (reporter) 2016-02-07 21:53 |
Hey abma. I'm with jK on this. Most of what you changed just affects the build system for streflop which becomes irrelevant when using pkg-config on a proper distro package. Some of the changes made are actually not good, such as removing the starting of a to be completed later unit testing which pbuilder will need. |
|
hokomoko (developer) 2016-02-08 01:11 |
I'm not sure if I agree with using external streflop, as the main point of it is being synced. Someone who compiled their own mangled streflop for some other project may then compile spring and fail to sync with no real warning as to the reason. |
|
Kip (reporter) 2016-02-08 01:14 |
That argument applies to every library you are dependent on. You should always use the distribution's official package whenever possible. If you can test the one in my PPA, we can have it nominated as an official Debian package (and by extension all of its derivatives, such as Ubuntu). There are five different libraries that are included in it and all of them are available via XDG's standard pkg-config interface for whatever build environment you want to use, such as CMake, GNU Autotools, vanilla makefile, or what have you. |
|
hokomoko (developer) 2016-02-08 09:54 Last edited: 2016-02-08 10:41 |
"That argument applies to every library you are dependent on. You should always use the distribution's official package whenever possible." I highly disagree. Most errors that can happen by using a wrong library version will either happen during compilation (such as using boost with a wrong exception model) or be relatively visible (such as libil failing to load textures) However, with streflop you can test locally successfully and not even know something is wrong before connecting to a game which desyncs halfway through for no apparent reason. Even worse is the fact that we don't know in desync which player is at fault, so add that to the mix. tl;dr: while in other libs correctness is more important than consistency, due to streflop's unique role, consistency is more important here than correctness. |
|
abma (administrator) 2016-02-08 14:09 |
"tl;dr: while in other libs correctness is more important than consistency, due to streflop's unique role, consistency is more important here than correctness." add a small runtime test? only problem is: we need examples which fail when i.e. a wrong compile flag was used. |
|
hokomoko (developer) 2016-02-08 14:25 |
It'll be a solution to known hazards, as opposed to enforcing a streflop version which is a solution to most unknown hazards as well. What do we gain from allowing linking with an external streflop? |
|
Kloot (developer) 2016-02-08 14:42 Last edited: 2016-02-08 14:52 |
Very little if only because streflop would still need to be built internally for windows. You really want to eliminate every possible variable when dealing with FP math across platforms. I also feel like we have this discussion each time a package maintainer comes by... |
|
Kip (reporter) 2016-02-08 19:20 |
hokomoko & abma: The unit testing is done automatically via dh_auto_test. See DPM ยง 4. The point hokomoko raised is really not applicable on a modern GNU distribution with properly debianized packages. |
|
hokomoko (developer) 2016-02-08 19:35 |
"The point hokomoko raised is really not applicable on a modern GNU distribution with properly debianized packages." We're cross platform, cross distro, and cross hardware. If we were all using the same configuration streflop wasn't required in the first place. The entire concept of using an external library to ensure consistency is baffling (no wonder it didn't have a package). Can you please explain how the advantages of linking through pkg-config (whatever these are) outweigh the risks? Will you volunteer investigating desync reports yourself to mitigate said risks? (Unless Kip gives us a good reason or someone has a huge objection, this is going to be a wontfix) |
|
Kip (reporter) 2016-02-08 19:42 |
"We're cross platform, cross distro, and cross hardware." That isn't the point. The point if the package is available from a native platform's package management system, then the build environment should first validate it is sufficient (e.g. XDG's standard pkg-config mechanism). This works on all platforms, including Windows. "The entire concept of using an external library to ensure consistency is baffling (no wonder it didn't have a package)." Just because it is "internal" doesn't mean it will be built consistently either. As for it not being packaged, that has nothing to do with it. Upstream's tarball had never been nomimated. That's all. Regarding the merits of pkg-config, see the following: https://wiki.freedesktop.org/www/Software/pkg-config/ https://people.freedesktop.org/~dbn/pkg-config-guide.html https://en.wikipedia.org/wiki/Pkg-config "Will you volunteer investigating desync reports yourself to mitigate said risks?" The whole reason why I brought the PPA to your attention was so that it's build and packaging could be validated. Please give it some testing. All you need to do is verify via pkg-config it is present and new enough, and then use it's include and linker flags. If it isn't, then fall back to your local build. That is the standard way most upstream projects handle scenarios like this. |
|
hokomoko (developer) 2016-02-08 19:52 |
"The whole reason why I brought the PPA to your attention was so that it's build and packaging could be validated" Obviously. That's the entire point, once this is tested, you go your way, and we'll have to deal with the fallout. "Just because it is "internal" doesn't mean it will be built consistently either" What this sentence means is that we shouldn't be using streflop at all as it doesn't offer consistency (its only use). I highly doubt that is the case since we have been using it successfully for a few years. "Regarding the merits of pkg-config, see the following:" I've used pkg-config before, I know what it does. My question was very specific: Can you please explain how the advantages of linking through pkg-config (whatever these are) outweigh the risks? |
|
Kip (reporter) 2016-02-08 19:56 |
"I've used pkg-config before, I know what it does. My question was very specific: Can you please explain how the advantages of linking through pkg-config (whatever these are) outweigh the risks?" I'm sorry hokomoko, but that is a contradiction. Best of luck with your project. Over and out. |
|
abma (administrator) 2016-02-08 22:34 |
@hokomoko: sorry, i still disagree @Kip the streflop you provide has unresolved symbols: g++ test.cpp $(pkg-config streflop-sse --cflags --libs) /usr/lib/x86_64-linux-gnu//libstreflop-sse.so: undefined reference to `streflop_libm::SimplePositiveInfinity' /usr/lib/x86_64-linux-gnu//libstreflop-sse.so: undefined reference to `streflop_libm::DoublePositiveInfinity' /usr/lib/x86_64-linux-gnu//libstreflop-sse.so: undefined reference to `fastiroot(double)' test.cpp: #include "streflop/Math.h" #include <stdio.h> int main() { printf("%f", streflop::sin(1.0f)); return 0; } also it seems there is no 32 bit version available?! can you fix these or point me to the error i made and report back? |
|
Kip (reporter) 2016-02-08 23:31 |
Hey abma. Yes, you are correct. Those symbols appear to be missing. I discovered that last night. The strange thing is the test cases appear to compile and link successfully. I'm working with upstream to get that resolved and will report back to you when I've pushed the new package to the build server. As for the 32 bit version, there is suppose to be one. Funny enough though it actually ended up crashing g++. I discovered what appears to be a bug in all released versions of it. I'm working with upstream on that now. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69715 |
|
Kip (reporter) 2016-02-14 01:20 |
So I've almost narrowed down the problem. The test cases (randomTest and arithmeticTest) compile and link successfully because they are linked against the static lib (.a) version. If I try your minimal and link against the static lib, it compiles and links successfully. If I try the same again, but link against the shared object version I can replicate the above linker errors. I've been trying to figure it out all morning and I can't see what's making this happen. The creation of the static library for SSE through the makefile is simply... $ ar r streflop.a $(LIBM_OBJECTS) Math.o Random.o The creation of the shared library version is... g++ -o libstreflop-sse.so.0.0.0 -shared -Wl,-soname=libstreflop-sse.so.0 -Wl,--as-needed $(LIBM_OBJECTS) Math.o Random.o ${USE_SOFT_BINARY} This consequently works... $ g++ test.cpp -DSTREFLOP_SSE=1 -o test streflop.a && ./test ...and this does not... $ g++ test.cpp -DSTREFLOP_SSE=1 -o test libstreflop-sse.so.0.0.0 && ./test |
|
abma (administrator) 2016-02-14 21:03 Last edited: 2016-02-14 21:04 |
very likely the linker just ignores the missing symbols when static linking, but it can't ignore it when linking to a shared lib. the link error message just says that the vars are declared, but not implemented. missing vars: streflop_libm::SimplePositiveInfinity streflop_libm::DoublePositiveInfinity missing function: fastiroot(double) |
|
Kip (reporter) 2016-02-15 04:42 |
The latter missing function was an easy fix: --- a/libm/dbl-64/mpsqrt.cpp +++ b/libm/dbl-64/mpsqrt.cpp @@ -42,9 +42,8 @@ /* p as integer. Routine computes sqrt(*x) and stores result in *y */ /****************************************************************************/ -Double fastiroot(Double); - namespace streflop_libm { +Double fastiroot(Double); void __mpsqrt(mp_no *x, mp_no *y, int p) { #include "mpsqrt.h" But the former two linker errors are very elusive. The two symbols appear to be declared within Math.h, but within streflop:: namespace, not streflop_libm:: namespace. But it gets weirder. Inside of Math.cpp both symbols appear to be defined (within streflop::). I wish I knew how to get the linker to give me more information on what code is trying to reference those two symbols within streflop_libm namespace. |
|
Kip (reporter) 2016-02-22 06:43 |
abma, I've just pushed a newer source package to the PPA build server. The linker errors appear to be solved. If after you try the new package (streflop 0.3-kip6) and it works for you, can you provide me with a minimal I can integrate into a unit test on the build server? I would have used upstream's arithmeticTest.cpp, but it's not clear to me what the correct output was suppose to be. Lastly, the i386 package will probably fail to build again for the reason already mentioned. There is a bug in GCC upstream has patched, but it may take some time for the fix to migrate downstream to the build servers. Thank you for your help. |
|
abma (administrator) 2016-02-29 02:09 |
> I would have used upstream's arithmeticTest.cpp, but it's not clear to me what the correct output was suppose to be. the test seems to do some math and write it to result files. so, to test, you have to create these files and store it somewhere and to verify you need to rerun the test again and see if the created files matches. so adding these files to a repo makes sense as it allows to verify if the results are the same when run on a different machine / with different compiler settings. |
|
abma (administrator) 2016-02-29 02:16 |
so basicly: ./arithmeticTest test1 ./arithmeticTest test2 for i in double simple; do for j in basic lib nan; do cmp test1_${i}_${j}.bin test2_${i}_${j}.bin; done done |
|
Kip (reporter) 2016-02-29 02:26 |
Hey abma. Yes, that is one way to do it. But the problem is I still won't know if the results generated as the ground truth are correct either on my machine prior to the unit test comparing whatever happened under sbuild or for some other user. |
|
abma (administrator) 2016-02-29 03:22 |
as the lib works for spring, just define "your results" as correct and see what happens. |
|
Kip (reporter) 2016-03-04 05:05 |
Hey abma. I've integrated unit testing for arithmeticTest. Same results on both my machine and Launchpad's sbuild bot. Verification is done using md5 checksums of the generated basic, lib, and nan bin files against the ones I generated for each of the five different types of library configurations. Give it a try. |
|
abma (administrator) 2016-07-07 19:19 |
started a new branch: https://github.com/spring/spring/tree/system-streflop |
|
abma (administrator) 2016-07-07 19:23 Last edited: 2016-07-07 19:27 |
needs to be done: - split the files created by us/for spring into a dedicated dir (i.e. streflop_wrapper) - log to infolog.txt that system installed steflop is used - ... |
|
abma (administrator) 2016-07-07 19:30 |
@Kip: can you build packages for trusty? https://travis-ci.org/spring/spring/builds/143110032 |
|
Kip (reporter) 2016-07-08 03:55 |
@abma: Injected into sbuilder, but can't build due to some needed dependencies, pkg-config (>= 0.28), libstdc++6 (>= 5.2.1). Do you actually need for Trusty? It builds fine under Wily, Xenial, and probably later. If you really need it for Trusty I will investigate whether the version numbers had any reasoning behind them. |
|
abma (administrator) 2016-07-08 12:14 |
yes: trusty (sadly) is the most recent version available for travis-ci which we use for automatic compile testing. :-| |
|
Kip (reporter) 2016-07-08 22:30 Last edited: 2016-07-08 22:31 |
@abma: Package built successfully for amd64, but i386 fails due to a bug in GCC causing the latter to crash. This has since been fixed upstream (GCC), but the version of GCC used with the trusty chroot sbuilder setup is old. The amd64 package should be automatically injected shortly. |
|
abma (administrator) 2016-07-09 11:45 |
thanks, seems to basicly work now. but still some parts of the integrated lib are used, this needs some cleanup/more work. |
|
Kip (reporter) 2016-07-09 18:48 |
Glad to hear abma. |
|
Kip (reporter) 2016-10-13 01:47 |
Pushed to Yakkety successfully. |
| Notes |
| Issue History |
|||
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2016-02-07 04:39 | Kip | New Issue | |
| 2016-02-07 20:21 | jK | Note Added: 0015641 | |
| 2016-02-07 20:23 | Kip | Note Added: 0015642 | |
| 2016-02-07 21:17 | abma | Note Added: 0015645 | |
| 2016-02-07 21:20 | Kip | Note Added: 0015646 | |
| 2016-02-07 21:41 | abma | Note Added: 0015647 | |
| 2016-02-07 21:43 | jK | Note Added: 0015648 | |
| 2016-02-07 21:51 | abma | Note Added: 0015649 | |
| 2016-02-07 21:53 | Kip | Note Added: 0015650 | |
| 2016-02-08 01:11 | hokomoko | Note Added: 0015652 | |
| 2016-02-08 01:14 | Kip | Note Added: 0015653 | |
| 2016-02-08 09:54 | hokomoko | Note Added: 0015654 | |
| 2016-02-08 10:41 | hokomoko | Note Edited: 0015654 | View Revisions |
| 2016-02-08 14:09 | abma | Note Added: 0015655 | |
| 2016-02-08 14:25 | hokomoko | Note Added: 0015656 | |
| 2016-02-08 14:42 | Kloot | Note Added: 0015657 | |
| 2016-02-08 14:52 | Kloot | Note Edited: 0015657 | View Revisions |
| 2016-02-08 19:20 | Kip | Note Added: 0015674 | |
| 2016-02-08 19:35 | hokomoko | Note Added: 0015676 | |
| 2016-02-08 19:42 | Kip | Note Added: 0015679 | |
| 2016-02-08 19:52 | hokomoko | Note Added: 0015681 | |
| 2016-02-08 19:56 | Kip | Note Added: 0015684 | |
| 2016-02-08 19:56 | hokomoko | Status | new => closed |
| 2016-02-08 19:56 | hokomoko | Assigned To | => hokomoko |
| 2016-02-08 19:56 | hokomoko | Resolution | open => won't fix |
| 2016-02-08 22:34 | abma | Note Added: 0015692 | |
| 2016-02-08 22:34 | abma | Assigned To | hokomoko => abma |
| 2016-02-08 22:34 | abma | Status | closed => feedback |
| 2016-02-08 22:34 | abma | Resolution | won't fix => open |
| 2016-02-08 23:31 | Kip | Note Added: 0015695 | |
| 2016-02-08 23:31 | Kip | Status | feedback => assigned |
| 2016-02-09 00:17 | abma | Status | assigned => feedback |
| 2016-02-14 01:20 | Kip | Note Added: 0015766 | |
| 2016-02-14 01:20 | Kip | Status | feedback => assigned |
| 2016-02-14 21:03 | abma | Note Added: 0015782 | |
| 2016-02-14 21:04 | abma | Note Edited: 0015782 | View Revisions |
| 2016-02-15 04:42 | Kip | Note Added: 0015787 | |
| 2016-02-22 06:43 | Kip | Note Added: 0015840 | |
| 2016-02-29 02:09 | abma | Note Added: 0015942 | |
| 2016-02-29 02:16 | abma | Note Added: 0015943 | |
| 2016-02-29 02:26 | Kip | Note Added: 0015944 | |
| 2016-02-29 03:22 | abma | Note Added: 0015945 | |
| 2016-03-04 05:05 | Kip | Note Added: 0015965 | |
| 2016-07-06 20:59 | abma | Target Version | => 103.0 |
| 2016-07-07 19:19 | abma | Note Added: 0016475 | |
| 2016-07-07 19:23 | abma | Note Added: 0016476 | |
| 2016-07-07 19:27 | abma | Note Edited: 0016476 | View Revisions |
| 2016-07-07 19:30 | abma | Note Added: 0016477 | |
| 2016-07-08 03:55 | Kip | Note Added: 0016479 | |
| 2016-07-08 12:14 | abma | Note Added: 0016480 | |
| 2016-07-08 22:30 | Kip | Note Added: 0016481 | |
| 2016-07-08 22:31 | Kip | Note Edited: 0016481 | View Revisions |
| 2016-07-09 11:45 | abma | Note Added: 0016482 | |
| 2016-07-09 18:48 | Kip | Note Added: 0016486 | |
| 2016-07-19 15:28 | abma | Target Version | 103.0 => 104.0 |
| 2016-10-13 01:47 | Kip | Note Added: 0016795 | |
| 2017-07-07 21:40 | abma | Target Version | 104.0 => |
| Issue History |