Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

6 python tests failing in 3.3.0 #2302

Open
bremner opened this issue Nov 25, 2023 · 36 comments
Open

6 python tests failing in 3.3.0 #2302

bremner opened this issue Nov 25, 2023 · 36 comments
Assignees
@afh
Labels
P2 Second highest priority tests Related to ledger's test suite

Comments

@bremner
Copy link
Contributor

bremner commented Nov 25, 2023

Ledger is currently scheduled to be removed from the next stable release of Debian due to the following failing tests in ledger 3.3.0. Full log is at 1, extract containing tests is at 2.

> The following tests FAILED:
> 	  3 - demo (Failed)
> 	307 - RegressTest_4D9288AE_py (Failed)
> 	344 - RegressTest_78AB4B87_py (Failed)
> 	357 - RegressTest_9188F587_py (Failed)
> 	374 - RegressTest_B21BF389_py (Failed)
> 	417 - RegressTest_xact_code_py (Failed)
> Errors while running CTest
@bremner bremner changed the title 5 python tests failing in 3.3.0 6 python tests failing in 3.3.0 Nov 25, 2023
@tbm
Copy link
Contributor

tbm commented Nov 27, 2023

This is something for @afh although I'm not sure if he's around at the moment. @afh ping?

I wonder why this hasn't been flagged before. I thought GitHub CI was testing Python 3, so not sure.

@tbm
Copy link
Contributor

tbm commented Nov 27, 2023 •
edited

The first is:

> 3: Welcome to the Ledger.Python demo!
> 3: Traceback (most recent call last):
> 3:   File "/<<PKGBUILDDIR>>/python/demo.py", line 83, in <module>
> 3:     assertEqual([u'', u'$', u'%', u'XCD', u'h', u'm', u's', u'¤', u'€'],
> 3:   File "/<<PKGBUILDDIR>>/python/demo.py", line 19, in assertEqual
> 3:     raise Exception("FAILED: %s != %s" % (pat, candidate))
> 3: Exception: FAILED: ['', '$', '%', 'XCD', 'h', 'm', 's', '¤', '€'] != ['', '%', 'XC', 'h', 'm', 's']

['', '$', '%', 'XCD', 'h', 'm', 's', '¤', '€'] != ['', '%', 'XC', 'h', 'm', 's']

So some commodities are different. XDC becomes XC. And the Unicode characters are missing. (The code says Tests currency symbols encoded using UCS. For details see #2132.).

Hmm, the errors are all different. Another one is:

> 307:   +RuntimeError: Cannot read journal file "/<<PKGBUILDDIR>>/test/regress/4D9288AE.da"
> 307: 
> 307:   +Error: Failed to execute Python module

It doesn't find the name with the ending .da with the code says .dat and the file exists.

Ah, same here:

> 344:   +RuntimeError: Cannot read journal file "/<<PKGBUILDDIR>>/test/regress/78AB4B87.da"

I wonder if the $PATH is very long in the build environment and the filename gets cut off.

And another one:

> 374:   +RuntimeError: Cannot read journal file "/<<PKGBUILDDIR>>/test/regress/B21BF389_py.tes"

and one more:

> 417:   +RuntimeError: Cannot read journal file "/<<PKGBUILDDIR>>/test/regress/xact_code.da"

So, in summary, there appear to be two distinct issues:

  • Commodities different/missing
  • Test .dat file not found, possibly due to filename cut off by one. The filenames have different lengths, so it can't be due to a string length limitation; somehow it's a off-by-one, I think.

@tbm
Copy link
Contributor

tbm commented Nov 27, 2023

Debian unstable has Python 3.11.4. I wonder what the GitHub CI is using...

@tbm
Copy link
Contributor

tbm commented Nov 27, 2023

Hm, possibly 3.11.5 on macOS.

@afh are you around?

@afh
Copy link
Member

afh commented Nov 28, 2023

Thanks for raising this @bremner and thanks for the preliminary investigation @tbm, much appreciated.

@tbm did you have a look at a publicly available output of a test run or reproduce locally?

@tbm
Copy link
Contributor

tbm commented Nov 28, 2023

@afh I looked at the log @bremner mentioned:

http://qa-logs.debian.net/2023/10/27/ledger_3.3.0-4_unstable.log

@afh
Copy link
Member

afh commented Nov 28, 2023

ℹ️ @tbm GitHub CI seems to be testing with Python 3.10.12 on ubuntu-latest, i.e Ubuntu 22.04 (see runner-images). According to the full log Cmake reports Python version 3.11.6 being found.

ℹ️ The full log informs "I: NOTICE: Log filtering will replace 'build/ledger-LYcYiL/ledger-3.3.0' with '<>'", which seems to be a path of reasonable length.

❓ @bremner Are the test failures on Debian un/stable also observed with ledger 3.3.2?

❓ Is it possible to get an archive of the build directory (e.g. build/ledger-LYcYiL/ledger-3.3.0) for closer inspection?

❓ Is a build history for ledger on Debian available from which one can see when tests started to fail and what may have changed in ledger or the test environment?

@bremner
Copy link
Contributor Author

bremner commented Nov 28, 2023 via email

@afh
Copy link
Member

afh commented Nov 30, 2023

Yes, I just duplicated the failures with 3.3.2

😢 This might hint at an issue with the test environment. Anything Debian-specific or unusual that comes to your mind, @bremner?

Sure, but it's likely to be pretty large (and contained dynamically
linked executables). How would you like to receive it?

Depending on the size via e-mail (< ~50MB) or a public download link from a server you have access to or a file-sharing service that does not require sign-in.

I just have the one previous successful build of 3.3.0

🙏 Taking a look…

@bremner
Copy link
Contributor Author

bremner commented Nov 30, 2023 via email

@afh
Copy link
Member

afh commented Nov 30, 2023

Thanks, @bremner, I have retrieved it.

@afh afh mentioned this issue Dec 5, 2023
Open
@afh afh self-assigned this Dec 5, 2023
@afh
Copy link
Member

afh commented Dec 5, 2023

After digging through Ledger's tests I've understood what parts are involved to run the tests and I assume the error may surface in the Python interface, though I'm not sure and not sure how to best verify this.

In the following snippet the filepath argument to the ledger.read_journal call is complete, so maybe the last character of the path is lost when handing over the path from Python to the native C++ code, e.g. in the Python wrapper for read_journal (see py_session.cc:83f and py_session.cc:45f).

Could the used locale (C.UTF-8) and its encoding affect strings in this regard?
What could other potential causes for this issue be? Any ideas are welcome.

307:   +Traceback (most recent call last):
307: 
307:   +  File "/<<PKGBUILDDIR>>/test/regress/4D9288AE.py", line 3, in <module>
307: 
307:   +    for post in ledger.read_journal("test/regress/4D9288AE.dat").query("^expenses:"):
307: 
307:   +                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
307: 
307:   +RuntimeError: Cannot read journal file "/<<PKGBUILDDIR>>/test/regress/4D9288AE.da"
307: 
307:   +Error: Failed to execute Python module

At @bremner do you have access to a Debian Unstable system and would be able to compile ledger from source and run the following from a ledger repo clone?

python test/RegressTests.py $PATH_TO_BUILD/ledger . ./test/regress/4D9288AE_py.test

/cc @jwiegley

@bremner
Copy link
Contributor Author

bremner commented Dec 5, 2023 via email

@afh
Copy link
Member

afh commented Dec 6, 2023

Thanks @bremner, that's helpful. I've installed Debian 12 (stable) in a virtual machine and am unable to reproduce the issue so far.

I'll look to switching to debian unstable tomorrow. Any other info that you could provide that could help me get an environment up and running with which the issue can be reproduced would be very much appreciated.

@afh
Copy link
Member

afh commented Dec 6, 2023 •
edited

After an update to Debian unstable all ledger tests continue to succeed for me and I am unable to reproduce the reported issue.

Here is how I set up my testing/debugging environment:

sudo sed -i'' -e 's/bookworm /unstable /g' /etc/apt/sources.list
  • Upgrade to Debian Unstable
sudo apt update && sudo apt -y full-upgrade

full-upgrade ran into errors while processing linux-image-6.5.0-5-arm64 and linux-image-arm64

  • Reboot
  • Install ledger dependencies:
sudo apt-get -y install git g++ cmake libboost-dev libmpfr-dev libgmp-dev libicu-dev
  • Clone ledger repository
git clone https://github.com/ledger/ledger.git
  • Change working directory to local ledger clone
cd ledger
  • Configure build
cmake -Bbuild -S. -DUSE_PYTHON:BOOL=ON
  • Build ledger
make -j4 -Cbuild
  • Run tests
make -j4 -Cbuild test
…
100% tests passed, 0 tests failed out of 424
  • Run exemplary failing test:
python3 test/RegressTests.py -l ./build/ledger -s . ./test/regress/4D9288AE_py.test
.
OK (1)

Note

The latest changes on ledger master branch changed the test scripts (inlcuding RegressTests.py) to use command-line options (-l and -s) instead of positional arguments to align with the CheckManpage.py and CheckTexinfo.py scripts. To test using a previous versions of ledgers test suite run python3 test/RegressTests.py ./build/ledger . ./test/regress/4D9288AE_py.test

@bremner How does the above setup differ from your or the Debian testing chroot setup and how can I mimick it?

@afh
Copy link
Member

afh commented Dec 6, 2023

ℹ️ Tested with clang instead of g++ and got the same results as above ☝️

@jwiegley
Copy link
Member

jwiegley commented Dec 6, 2023

My output is:

ledger-py3:[johnw@Vulcan:~/src/ledger/master]$ python3 test/RegressTests.py ~/Products/master/debug/ledger . ./test/regress/4D9288AE_py.test
FAILURE in output from ./test/regress/4D9288AE_py.test:
--
$ledger -f "/Users/johnw/src/ledger/master/test/regress/4D9288AE_py.test" python test/regress/4D9288AE.py

--
  @@ -1 +0,0 @@

  -None

FAILURE in error output from ./test/regress/4D9288AE_py.test:
--
$ledger -f "/Users/johnw/src/ledger/master/test/regress/4D9288AE_py.test" python test/regress/4D9288AE.py

--
  @@ -0,0 +1 @@

  +Error: Unrecognized command 'python'

FAILURE in exit code (0 != 1) from ./test/regress/4D9288AE_py.test:
--
$ledger -f "/Users/johnw/src/ledger/master/test/regress/4D9288AE_py.test" python test/regress/4D9288AE.py

--
E[4D9288AE_py.test]
FAILED (1)

@bremner
Copy link
Contributor Author

bremner commented Dec 6, 2023 via email

@bremner
Copy link
Contributor Author

bremner commented Dec 7, 2023 via email

@bremner
Copy link
Contributor Author

bremner commented Dec 7, 2023 via email

@afh
Copy link
Member

afh commented Dec 7, 2023

Thanks for confirming, @bremner. I am now able to reproduce the issue in my Debian unstable VM 🎉

I'll have a closer look at newer versions of utfcpp and what changed between them. Ledger contains the 3.2.2 source, Debian unstable offers 3.2.5 and the latest release is 4.0.3.

Off-topic: In case it has slipped your attention I'd appreciate your thoughts on #2313, which adds support for installing Debian Trixie dependencies via ledger's acprep.
Apologies for this nudge if you did see the message and did not get to it yet or decided to not comment.

@afh
Copy link
Member

afh commented Dec 7, 2023 •
edited

If I've tested properly the issue first appears when using utfcpp v3.2.5 and disappears again when using utfcpp v4.0.1.

Anyone see what could be the cause for the issue when looking at

What's the best way forward on this?

  • Is it a bug that exists in utfcpp 3.2.5 until 4.0.1 and ledger can decide to declare these versions as incompatible?
  • Can debian update utfcpp to at least 4.0.1? Will this be enough for current stable and testing?
  • Can we find a quick and maintainable fix?
  • Should ledger drop support for third-party packaged utfcpp and simply maintain its own version from here on?

What are your thoughts and ideas @bremner, @jwiegley, @tbm?

Update: I've raised this upstream

This was referenced Dec 7, 2023
Closed
Merged
@bremner
Copy link
Contributor Author

bremner commented Dec 7, 2023 via email

@afh
Copy link
Member

afh commented Dec 7, 2023

That's good news, @bremner and thanks for asking the Debian maintainer of utfcpp. Any specific reason why the request is to update utfcpp in Debian to 4.0.1 instead of the current latest version 4.0.3?

Your voice is appreciated and heard regarding ledger supporting third-party utfcpp. Just to understand the motivation for not embedding, what are the benefits from a distribution's point of view?

ℹ️ Checking the utfcpp release notes the following two issues are mentioned and seem related to the observed misbehaviour:

@afh
Copy link
Member

afh commented Dec 7, 2023

The author of nemtrif/utfcpp suggests:

Instead of using utf8::unchecked::utf16to8(), use utf8::utf16to8(). It is safer anyway and just a bit slower.

@jwiegley what was the initial motivation for using the unchecked variant?

@bremner
Copy link
Contributor Author

bremner commented Dec 7, 2023 via email

@afh
Copy link
Member

afh commented Dec 7, 2023

@bremner if I remember correctly Debian does maintain its own patches for some of its packages. Is this still the case and would it be feasible for Debian to apply the following patch (fix_utfcpp_3.2.5_regression.patch) to ledger as long as Debian ships with utfcpp 3.2.5?

And would this avoid ledger being dropped from testing?

@afh afh added P2 Second highest priority tests Related to ledger's test suite labels Dec 7, 2023
@hosiet
Copy link

hosiet commented Dec 8, 2023

Debian utfcpp packager here. While the best way might be upgrading Debian's utfcpp to 4.0.x, some other Debian's software will encounter build failures when using utfcpp 4.0.x so that won't happen immediately.

I plan to downgrade Debian's utfcpp to 3.2.4 for now, and later eventually upgrade Debian's utfcpp to 4.x series when all software are compatible with utfcpp 4.0.x. I believe further coordination is definitely needed; just let me know if I can further help.

@afh
Copy link
Member

afh commented Dec 8, 2023

Hi @hosiet, welcome to the conversation and thanks for chiming in 👋🙂

Downgrading Debian's utfcpp 3.2.4 for testing and adressing any build failures with utfcpp 4.0.x later down the line seems very reasonable to me.

I'd appreciate if you could leave a quick note here once the downgrade has happened.

Out of curiosity which other Debian packages make use of utfcpp and which ones break using utfcpp 4.0.x?

FYI: Ledger is upgrading to utfcpp 4.0.3 (see #2315) with the next release and is already compatible with it.

@hosiet
Copy link

hosiet commented Dec 8, 2023 •
edited

Thanks. The downgrade to utfcpp 3.2.4 has now happened. For more information, please see https://tracker.debian.org/pkg/utfcpp .

Just in case you are curious, the other software that breaks with utfcpp 4.0.x in Debian is apertium/apertium#193 .

@afh
Copy link
Member

afh commented Dec 8, 2023

That's great to hear, @hosiet. Thanks for moving so quickly on this and raising awareness for the issue with apertium.

@afh
Copy link
Member

afh commented Dec 8, 2023

@bremner thanks for the context on why distros prefer programs to link to libutfcpp, that makes a lot of sense.

A few things that are on my mind:

  1. Where can I follow Debian ledger builds with the downgraded utfcpp?
  2. What needs to happen, so that the Debian bug #1054728 and this bug can be closed?
  3. Are there plans to update Debian ledger to 3.3.2?
  4. Is there anything else that is open and that I could help with?

@bremner
Copy link
Contributor Author

bremner commented Dec 11, 2023 via email

@tbm
Copy link
Contributor

tbm commented Dec 11, 2023

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1039736
I haven't had a chance to check the existing issues for duplication.

There are many open bugs about print dropping information and creating invalid syntax. I'll check tomorrow if one matches the Debian report (#1768 is very close).

@afh
Copy link
Member

afh commented Dec 11, 2023

Thanks, @bremner for the update and already closing the Debian bug #1054728. I'm looking forward to seeing ledger 3.3.2 in Debian in the future :)

The link to the Debian ledger builds you shared mentions a cmake_rpath_contains_build_path_issue, is this something that you feel should be addressed here?

Another thank you for mentioning Debian Bug #1039736; a quick search shows issue #604 could be related, would you agree?

@jwiegley
Copy link
Member

The author of nemtrif/utfcpp suggests:

Instead of using utf8::unchecked::utf16to8(), use utf8::utf16to8(). It is safer anyway and just a bit slower.

@jwiegley what was the initial motivation for using the unchecked variant?

I really don't remember, maybe just speed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@afh afh

Labels
P2 Second highest priority tests Related to ledger's test suite
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@jwiegley @afh @bremner @tbm @hosiet