Port utils::convert_text() and UTF-8 <-> locale converters to Rust #1455
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I don't see a way to make the testing any more thorough, as we're basically testing the platform's support for transliteration — there is nothing our tests can do to enable or disable that support.
They all pass, even with a dummy implementation that just returns `tocode`. I'll ameliorate that in the next commit.
This test currently fails, because Rust has a dummy implementation. As a reminder: we can't rely on conventional tests here because translit() is platform-independent, and there's no fixed known-good output for any given input. But we can compare two implementations to see if they behave the same — which is exactly what the test is doing.
The "comparison test" which compares results of C++ and Rust implementations now pass, and will be removed in the next commit. This commit passed the CI, i.e. it works not just on my Linux machine, but also on Ubuntu Linux with various compilers, two macOS versions, and two FreeBSD versions.
The inner call to iconv_open() creates a separate iconv_t value which is never deallocated. The impact of this is minuscule, since Newsboat runs through this code path at most once, but it's still nice to plug this little leak.
Unlike C++ tests, Rust tests don't show output if they pass. As a result, the user won't see warnings about the tests that are skipped due to missing locales. The next commit will fix that.
As mentioned in the previous commit, Rust doesn't display stdout of tests which passed. As a result, we can't print a warning if we conditionally skip a test. An interested user can run `cargo test -- --show-output` and see if there are any warnings. But this is even worse than our C++ approach: in C++ it's sufficient to *look* at the output, but with Rust we'd have to *explicitly ask* for that output. So this commit moves the decision up, to the *invocation of Make*: `make check` (and `make ci-check`) runs all the tests *except* the ones that need locales, while `make NEWSBOAT_RUN_IGNORED_TESTS=1 check` runs *everything* (ditto for `ci-check`). This way, users and package builders can continue to run `make check` even if they don't have all the locales, and our CI can run a more strict version that checks everything.
It's not a user-facing documentation.
Most of it is (rightfully) marked as outdated, but the section on tests explains the purpose of `NEWSBOAT_RUN_IGNORED_TESTS` variable that I introduced a few commits ago.
Minoru
force-pushed
the
feature/rust-convert_text
branch
from
ccc521a
to
4555cc2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is another stepping stone to #1344.
I had to add my own bindings for
iconv()because my PR to the libc crate is stuck due to linking issues of macOS. In Newsboat, I have more control over the build process than I have in the libc crate, so I managed to overcome those issues here; but we should switch to libc bindings once that PR is merged.There are also some trouble with tests. #1443 added tests that require some locales to be present. Users and distro's build machines probably don't have those, so I put in checks to skip those tests. I couldn't transfer this trick to Rust directly, so Rust uses a slightly different scheme. More on that in the commit message to 7ec21f9
A commit message is not a good place for docs, so I also brushed up the hacker's guide a bit: marked most sections as outdated, but updated the one on tests. I don't yet have a plan on how to fully restore the hacker's guide, and what I actually want out of that document, but it's a start.
Reviews are welcome! Don't be afraid of large number of changes — it's the usual "rewrite in C++, port tests" stuff, nothing hardcore. I'll merge this in three days if no issues are raised.