2019-12-12 01:41 CET

View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0005058Spring engineLinuxpublic2017-07-07 21:40
ReporterKip 
Assigned Toabma 
PrioritynormalSeverityfeatureReproducibilityalways
StatusassignedResolutionopen 
Product Version100.0+git 
Target VersionFixed in Version 
Summary0005058: libstreflop-dev now available for testing on Ubuntu
DescriptionHey 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.
TagsNo tags attached.
Checked infolog.txt for Errors
Attached Files

-Relationships
+Relationships

-Notes

~0015641

jK (developer)

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).

~0015642

Kip (reporter)

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.

~0015645

abma (administrator)

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.

~0015646

Kip (reporter)

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.

~0015647

abma (administrator)

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...

~0015648

jK (developer)

"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).

~0015649

abma (administrator)

oh, indeed so, should be doable. streflop_cond.h isn't in streflop.h, so should be no problem.

~0015650

Kip (reporter)

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.

~0015652

hokomoko (developer)

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.

~0015653

Kip (reporter)

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.

~0015654

hokomoko (developer)

Last edited: 2016-02-08 10:41

View 2 revisions

"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.

~0015655

abma (administrator)

"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.

~0015656

hokomoko (developer)

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?

~0015657

Kloot (developer)

Last edited: 2016-02-08 14:52

View 2 revisions

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...

~0015674

Kip (reporter)

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.

~0015676

hokomoko (developer)

"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)

~0015679

Kip (reporter)

"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.

~0015681

hokomoko (developer)

"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?

~0015684

Kip (reporter)

"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.

~0015692

abma (administrator)

@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?

~0015695

Kip (reporter)

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

~0015766

Kip (reporter)

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

~0015782

abma (administrator)

Last edited: 2016-02-14 21:04

View 2 revisions

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)

~0015787

Kip (reporter)

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.

~0015840

Kip (reporter)

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.

~0015942

abma (administrator)

> 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.

~0015943

abma (administrator)

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

~0015944

Kip (reporter)

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.

~0015945

abma (administrator)

as the lib works for spring, just define "your results" as correct and see what happens.

~0015965

Kip (reporter)

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.

~0016475

abma (administrator)

started a new branch:

https://github.com/spring/spring/tree/system-streflop

~0016476

abma (administrator)

Last edited: 2016-07-07 19:27

View 2 revisions

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
- ...

~0016477

abma (administrator)

@Kip:
can you build packages for trusty?
https://travis-ci.org/spring/spring/builds/143110032

~0016479

Kip (reporter)

@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.

~0016480

abma (administrator)

yes: trusty (sadly) is the most recent version available for travis-ci which we use for automatic compile testing. :-|

~0016481

Kip (reporter)

Last edited: 2016-07-08 22:31

View 2 revisions

@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.

~0016482

abma (administrator)

thanks, seems to basicly work now. but still some parts of the integrated lib are used, this needs some cleanup/more work.

~0016486

Kip (reporter)

Glad to hear abma.

~0016795

Kip (reporter)

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