1.9.0: UnitTestsDSP fails in modified arch build

[dvzrv]

Bug Description:
When running the unit tests for 1.9.0 the UnitTestsDSP fails.

Surge Version
This information is found on the About screen, which you get to from the bottom right menu

• Version: 1.9.0
• Plugin Type: build
• Bitness: 64-bit

Reproduction Steps:
Steps to reproduce the behavior:

1. Build tests

# set datapath to local dir for testing
sed -e 's|/usr/share/Surge|resources/data|' -i src/common/SurgeStorage.cpp
# build surge-headless (test-suite)
cmake -DCMAKE_INSTALL_PREFIX='/usr' \
           -DCMAKE_BUILD_TYPE='None' \
           -W no-dev \
           -B build-test \
           -S .
make VERBOSE=1 -C build-test

2. Run build-test/surge-headless:

# surge-headless: 1.9.0 built: 0 0

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
surge-headless is a Catch v2.9.2 host application.
Run with -? for options

-------------------------------------------------------------------------------
Every Oscillator Plays
  Oscillator type Wavetable
-------------------------------------------------------------------------------
/build/surge/src/surge-1.9.0/src/headless/UnitTestsDSP.cpp:781
...............................................................................

/build/surge/src/surge-1.9.0/src/headless/UnitTestsDSP.cpp:806: FAILED:
  REQUIRE( sumAbsOut > 1 )
with expansion:
  0.0f > 1

===============================================================================
test cases:      59 |      58 passed | 1 failed
assertions: 4643968 | 4643967 passed | 1 failed

Expected Behavior:
All unit tests pass

Screenshots:
n/a

Computer Information (please complete the following information):

• OS: Arch Linux
• Host: n/a
• Version: n/a

Additional Information:

• gcc 10.2.0

[baconpaul]

Hmm well we run all the tests on every build so def something about your setup
That test will fail if the wavetable oscillator can’t find wavetables. You sure that see is working?
I our next release I have en environment var to override the data path which will make things much easier.

[dvzrv]

Thanks for the hint!
Maybe the hack for the local test is not enough and I need to modify more changes than the sed -e 's|/usr/share/Surge|resources/data|' -i src/common/SurgeStorage.cpp

Do note, that this affects only a test binary, not the packaged binary! I keep these two separated :)

[baconpaul]

If you look in the azure pipelines file you can see how we solve this in ci. Basically we stage the resources to a tmp dir and point xdghome at it. Maybe same would work?

In our xt branch there’s a variable for direct override but not in 19

[mkruselj]

Not a bug, then?

[dvzrv]

@baconpaul okay, using XDG_DATA_HOME is of course much more straight forward for setting up the tests. Unfortunately I still get the same error:

# surge-headless: 1.9.0 built: 0 0

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
surge-headless is a Catch v2.9.2 host application.
Run with -? for options

-------------------------------------------------------------------------------
Every Oscillator Plays
  Oscillator type Wavetable
-------------------------------------------------------------------------------
/build/surge/src/surge-1.9.0/src/headless/UnitTestsDSP.cpp:781
...............................................................................

/build/surge/src/surge-1.9.0/src/headless/UnitTestsDSP.cpp:806: FAILED:
  REQUIRE( sumAbsOut > 1 )
with expansion:
  0.0f > 1

===============================================================================
test cases:      59 |      58 passed | 1 failed
assertions: 4641905 | 4641904 passed | 1 failed

[baconpaul]

Well that is perplexing

We don’t get that break on ubuntu

One thought is that will use a default wavetable and I wonder if that default could from a fs scan order or some such. That’s a sloppy test of course - we should force a repeatable wavetable. But may be it. If I share a little micro patch here would you be able to try it easily? But probably not today.

thanks for your patience and testing!

[dvzrv]

If I share a little micro patch here would you be able to try it easily? But probably not today.

Yes, sure! :)

[baconpaul]

diff --git a/src/headless/UnitTestsDSP.cpp b/src/headless/UnitTestsDSP.cpp
index b87ceb4f..72f2fb83 100644
--- a/src/headless/UnitTestsDSP.cpp
+++ b/src/headless/UnitTestsDSP.cpp
@@ -792,6 +792,25 @@ TEST_CASE("Every Oscillator Plays", "[dsp]")
 
             for (int q = 0; q < 10; ++q)
                 surge->process();
+
+            int idx = 0;
+            bool got = false;
+            for (auto q : surge->storage.wt_list)
+            {
+                if (q.name == "Sine Power HQ")
+                {
+                    got = true;
+                    surge->storage.getPatch().scene[0].osc[0].wt.queue_id = idx;
+                }
+                idx++;
+            }
+            REQUIRE(got);
+            for (int q = 0; q < 10; ++q)
+                surge->process();
+
+            REQUIRE(std::string(surge->storage.getPatch().scene[0].osc[0].wavetable_display_name) ==
+                    "Sine Power HQ");
+
             float sumAbsOut = 0;
             surge->playNote(0, 60, 127, 0);
             for (int q = 0; q < 100; ++q)

If you apply that patch, does it work? That forces you to load one of the factor wavetables, makes sure it is loaded

My theory is we choose the first wavetable by scan by default. Something about your install is picking one which is silent in the generator at default (either it is a growing wavetable or a one shot or some such). This will force you to something which, in my system, makes sound.

I plan to merge this in any case - makes the test far better - but would love to know if it fixes your problem first.

Thanks!

[baconpaul]

So I pushed that diff just now to our next-release branch (xt-alpha) and all our CI tests passed. Curious if it fixes the problem for you also.

If not, then next step is to bring up surge and see if the wavetable oscillator actually makes noise in your system!

[dvzrv]

That does indeed fix the test. Thanks!

I am suspecting, that if this is a sorting issue, it is probably introduced by the filesystem.
I have e.g. run into issues with games on Linux (Civilization V has this problem), that were created to implicitly rely on a filesystem such as NTFS to sort files for loading in a very specific order. This leads e.g. steam to only support this game when run from an ext4 partition (as with that filesystem they can emulate the behavior), but e.g. btrfs is not supported, as the game will just crash as soon as it loads files in the wrong order.

I guess the take away is: It is good to be explicit! :)

Thanks again for looking into fixing this!

[baconpaul]

Yeah we don’t rely on fs order per se but the default wt is the first in fs order so we got unlucky here. I’ll merge the fix into both branches today. Thanks!

[baconpaul]

Oh hey @dvzrv I was just looking at https://github.com/archlinux/svntogit-community/blob/packages/surge/trunk/PKGBUILD quickly. A couple of minor things

1. Can you grab the patch from surge-synthesizer not baconpaul repo? The baconpaul repo is just my fork. Line 23.
2. I know you are zeroing out some things for repro, but the version, hash, etc... will be stable for a given version and drives the about screen our users use to report errors. Around line 38 or so if you could not nuke the RELEASE_ and FULL_ version things and just the BUILD ones, that would mean that if someone uses your package and reports a problem we know what software they are running.
3. On binaries and plugins have executable stack by default #1701 I thought I'd asked some question about performance and executable stack - is that still live?

Thanks!

[dvzrv]

hey @baconpaul

1. I can use the now merged commit from the surge main repo
2. Of course!
3. To my knowledge there is no performance penalty when circumventing executable stack and of the > 600 packages I maintain, none has executable stack (as it is considered a vulnerability).

Thanks for the drive-by review! :D

[baconpaul]

Awesome thanks - I just moved 1701 into our next milestone and will add that flag. Appreciated!