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.

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

Text wrapping problem #19

Closed
thoni56 opened this issue Oct 12, 2020 · 18 comments
Closed

Text wrapping problem #19

thoni56 opened this issue Oct 12, 2020 · 18 comments

Comments

@thoni56
Copy link
Collaborator

thoni56 commented Oct 12, 2020

In an issue in the alan-doc project Tristano mentioned a possible problem with the line-wrapping calculation:

After some fiddling around with the WIDTH option in the source adventures, I realized that the main problem is that the value is not honored correctly in the generated transcript, which tend to wrap before the expected column.

If set it to 80 it seems to fail to wrap at the longest possible line that doesn't go beyond that value. E.g. it would wrap at 72 even though the following word is 4 character only. The same applies to any other value, it tend to wrap earlier than expected.

Here's a real example generated from the StdLib repo, using the default setting of 80 (adding it explicitly yelded the same results):

The air in this gloomy tunnel is filled with foul smelling fumes. Your
eyes are itching and your lungs burning.

The output is being wrapped at 71, but the correct wrapping for the value 80 should have been:

The air in this gloomy tunnel is filled with foul smelling fumes. Your eyes are
itching and your lungs burning.

As you can see, there was space for two more words on the line.

Possibly it's a problem with the algorithm that handles line wrapping in the interpreter.

Is the width value to be considered as inclusive of the max chars per line? (i.e. 80 = 80 chars, or 79?)

I have the impression that white spaces are being included in the computation, for output lines often have a space as their last character. This might be affecting calculation of the wrapping point, falsifying its value.

@thoni56
Copy link
Collaborator Author

thoni56 commented Oct 25, 2020

I could not find the issue about the random white space issue exposed clearly in https://github.com/alan-if/alan-bugs-testbed/tree/master/whitespace-bug (if we ever had one about that precise problem), so I'll make a note about the following here, in case that actually is a part of this problem too.

In dev snapshot 2103 I fixed a valgrind discovered illegal memory read in the output() function when it tries to analyze spacing for an empty string. This might very well be the cause of that problem, at least. So I hope this solves the problem, @tajmone.

This is probably also (one of the) culprits behind AnssiR66/AlanStdLib#110.

@tajmone
Copy link
Contributor

tajmone commented Apr 23, 2021

I wanted to try out the latest dev snapshot, but it turns out the Windows binaries are not working (see #26)

@thoni56
Copy link
Collaborator Author

thoni56 commented Apr 23, 2021

There are no recent dev snapshots since I've not restored the CI on my machine yet. My plan is to get to that after the UTF-work is done. (Yeah, I know, it would be good to have that...)

@tajmone
Copy link
Contributor

tajmone commented Apr 23, 2021

Just give me a buzz when the working Alpha is out, so I can test for the whitespace bug and let you know if it's finally fixed.

@thoni56
Copy link
Collaborator Author

thoni56 commented Apr 29, 2021

I've setup a new CI-chain for the Alan SDK leveraging my companies Jenkins install at https://ci.alanif.se.

The upside of that is that it will stay up (in contrast to my own desktop Windows machine for which I've never managed to control its sleep cycles ;-)

Another upside is that even though it's running on Linux cross-compiling the Windows command line SDK seems to be working great. Downside is that cross-compiling WinA*n does not give a executable .exe:s. I can cross-compile from Cygwin, maybe because it has a more recent version of the gcc cross chain.

Another slight downside is that there will be other jobs that might be visible if you are curious...

But it's a start. And it will give me feedback on the CI chain of someone downloads the binaries and uses them for anything. You have to do that from Jenkins, I haven't gotten to the upload bit yet. Also have to adjust the build number to continue from where the previous builds left off.

@thoni56
Copy link
Collaborator Author

thoni56 commented Apr 29, 2021

Crossing my fingers for the whitespace bug...

@tajmone
Copy link
Contributor

tajmone commented Apr 30, 2021

I remembered you telling me that WinArun gave some problems, due to Glk if I remember correctly. Maybe you should try older versions of GCC for cross compiling to Windows. I know that some versions work better than others, and it's rarely the latest one, especially if a library was built for the MS 2010 C runtime, which is still largely used for Win32 API based applications — the problem in this case is the usual lack of support for C99, because MS compilers of that time only supported C89, and the latest one don't really support C99, they just provide some hacks that might make it compile.

CygWin, if I remember correctly, leverages to old ABI for the 2010 CRT, whereas MSYS and later versions of MinGW might be using more recent versions. This has been a source of many problems for me when trying to bind to C libraries under Win.

This is also the reason why Fossil and SQLite still use C89/ANSI-C, to ensure full cross-platform compatibility, and in fact they work like a charm under Windows.

@thoni56
Copy link
Collaborator Author

thoni56 commented Apr 30, 2021

Cross-compilation on Darwin/Brew also produces a workable WinArun. Brew also has 10.3, same as Cygwin, while Ubuntu 20.04 (on which the current CI is running) is using 9.3. So I expect that upgrading will solve this.

Update: and in 21.04 Ubuntu will also have 10.3.

@tajmone
Copy link
Contributor

tajmone commented Apr 30, 2021

Many cross-platform languages (e.g. Nim, Rust, and others) advise using GCC 6.3.0 to compile under Windows. Higher versions tend to be problematic, from what I understood.

I've also used that GCC version to compile numerous projects, including IF tools and libraries, and it works well (including old projects that were designed for much older versions of GCC).

MSYS2 uses GCC 10.2.0 (both 32- and 64-bit), and in fact it fails to compile many IF tools (especially those with a GUI)

@thoni56
Copy link
Collaborator Author

thoni56 commented May 8, 2021

Any feedback or conclusions on the whitespace bug?

@tajmone
Copy link
Contributor

tajmone commented May 8, 2021

Any feedback or conclusions on the whitespace bug?

I was waiting for the new Alpha SKD. Is it released? (i.e. working)

@thoni56
Copy link
Collaborator Author

thoni56 commented May 8, 2021

There are still no packages uploaded, but I thought I mentioned (but maybe didn't) that the command line tools are available from the CI.

I thought that was sufficient for a test, but if not, give me a ping and I'll put some focus on restoring the alpha SDK packaging and uploading working again.

@tajmone
Copy link
Contributor

tajmone commented May 9, 2021

There are still no packages uploaded, but I thought I mentioned (but maybe didn't) that the command line tools are available from the CI.

Sorry, I must have forgotten about it.

I thought that was sufficient for a test, but if not, give me a ping and I'll put some focus on restoring the alpha SDK packaging and uploading working again.

OK, I've downloaded the Win SDK from the Jenkins page — a bit confused about the fact that it uses the Alpha build numbering but the Beta naming (alan3.0beta8-2120.win32.i686.zip), but I guess it's an Alpha-2120 build.

Anyhow, I couldn't run the tests because I get the following errors when I try to run the arun.exe and alan.exe binaries:

The procedure entry point iconv could not be located in the dynamic library.

It seems it's asking for the iconv DLL.

Did you link iconv dynamically?

Is it one of those Windows cases where it's hard (or impossible) to build and link the library statically?

Unless I know the exact DLL version required I can't run them (I believe I'd have to donwload the exact version the SDK was compiled for/with).

@thoni56
Copy link
Collaborator Author

thoni56 commented May 9, 2021

OK, I've downloaded the Win SDK from the Jenkins page — a bit confused about the fact that it uses the Alpha build numbering but the Beta naming (alan3.0beta8-2120.win32.i686.zip), but I guess it's an Alpha-2120 build.

The packages uses the same naming scheme as before (see download page). The upcoming beta-release number and a build number. Or do you mean that the information from --version has changed? No, can't be since you couldn't get them to start...

Anyhow, I couldn't run the tests because I get the following errors when I try to run the arun.exe and alan.exe binaries:

Darn. I forgot about that when I merged the UTF-8 work into master. Build 287 works and contains the fixes related to the "whitespace bug".

The Windows SDK is a cross build from Linux and did work (before the UTF-8 merges), it was the GUI-variant that previously did not result in usable Windows exe's. (Still doesn't.)

@thoni56
Copy link
Collaborator Author

thoni56 commented May 9, 2021

So I've just changed to static linking of iconv when cross-building and it seems to be working. I downloaded 2121 and it ran in a powershell on my Windows. Hope that works for you.

Incidentally, 2121 also has UTF-8 support (sources, error messages, command input and scripted input) if you want to start toying with that.

(I'm just going to verify that non-Ascii UTF-8 works in all verbs, location names etc.)

@tajmone
Copy link
Contributor

tajmone commented May 9, 2021

Sorry for the delay in replying, but I was finishing up the publication of the StdLib v1.00 in the Alan Goodies repository (see #27).

So, I've managed to successfully download the latest SDK from the Jenkins CI website, and now both the Alan compiler and ARun work fine.

Tests Result w/ Beta8 build 2121

So, I've used these binaries to run the StdLib testsuite. I've staged the resulting diffs from the first run, and after having run the tests multiple times the transcripts didn't change any further.

So the problem seems to be finally gone!

I also noticed that it's doing a much better job at handling trailing spaces at line ends (the only exception seems to be a whitespace after INDETERMINATE ARTICLES, but since I'm only looking at the diffs it's hard to tell).

I haven't had a chance to check the column wrapping problem (i.e. if it's now wrapping at 80 correctly) but only focused on the whitespace problem.

What was the origin of this mysterious bug?

@thoni56
Copy link
Collaborator Author

thoni56 commented May 9, 2021

Thanks, for taking the time to check this out. Good to hear that it at least seems to be fixed.

The source of the problem was a memory write outside of the allocated buffer or possible reading, only remember that I caught it in a valgrind session and suspected that it could affect wrapping.

I'll let this issue be alive a little longer, but will probably close it in a week or so. It's better to open new ones for particular issues that we find later.

@tajmone
Copy link
Contributor

tajmone commented May 10, 2021

The source of the problem was a memory write outside of the allocated buffer or possible reading, only remember that I caught it in a valgrind session and suspected that it could affect wrapping.

The type of nasty bug to trace and fix in C. This simply couldn't happen in Rust, because of the strong memory safety guarantees enforced by the compiler.

I still wonder why this bug displayed an alternating behaviour — at each run it would add then remove the exact same spurious whitespace, as if there was an "odd/even" recurring pattern, where each diff was identical to its previous-skip-one diff.

I'll let this issue be alive a little longer, but will probably close it in a week or so. It's better to open new ones for particular issues that we find later.

I agree, if (as I suspect) there's still a minor issue with the ind.art. being followed by a trailing space when occurring before a line wraps, it would be a new Issue.

@thoni56 thoni56 closed this as completed Jul 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants