Reimplement files.FileLines

[mfelsche]

• remove File.line
• add ponytest.UnitTest.set_up method for initializing test state that needs a TestHelper (i.e. for getting an Env)

Fixes #2185

[mfelsche]

This one still needs some docstrings and some manual changelog entries.

[SeanTAllen]

I'm uncomfortable with the iso change on buffered.Reader not going through some sort of discussion or RFC. Its out of scope for "Reimplement files.FileLines" and we should have a bit of discussion on a breaking change that expands that scope.

[mfelsche]

I know it's not classy to mangle it all together in one PR. I am happy to turn into RFC or separate PR what needs to be turned. This is my desired state of FileLines and i wanted to present it as such for discussion.

I just wanted to check back, if the outcome is in any way desirable: Having FileLines return String iso^, now being consistent with pretty much every method to retrieve bytes from some external resource. If this is not wanted, i can easily revert this breaking change and we all forget about it.

[mfelsche]

@SeanTAllen would you find an RFC appropriate for the iso change in buffered.Reader?

[SeanTAllen]

I think we should discuss what is appropriate during sync.

[slfritchie]

From today's Pony sync call: this work is inspired by #2185.

[mfelsche]

We discussed this on the sync call and decided to make the change on buffered.Reader go through the RFC process, before merging this, as this is a breaking change, although minor.

This also needs to have the examples fixed.

[mfelsche]

@SeanTAllen this is ready for a review.

[mfelsche]

Well actually this still contains a bug. It is not correctly resetting the file position in all cases, might be a bug in the try ... then ... expression, not calling the then clause under certain circumstances. I will investigate.

[mfelsche]

It seems to work correctly on most platforms, but is failing on windows due to some condition i do not fully understand. Appveyor fails with:

C:\projects\ponyc\packages\files\_test.pony:1024: Assert eq failed.  Expected ( b c d) == (b c d)

Which is, in the test it opens a file with the following contents: a\nb\nc\nd. It is seeking to position 2 from the start of the file and iterates over the lines. It seems, to be returning an empty first line although the file position should be behind the first \n. Do you maybe have a clue here @kulibali ? Could it be there is a \r slipping into the file somehow? Or is the line handling in buffered.reader on windows wrong?

[chalcolith]

I'll take a look later today.

[chalcolith]

Yeah, by default Windows will write \r\n to a file whenever you try to write \n. Here's a patch to default to "binary" mode, which I think is more sane in this day and age:

diff --git a/src/libponyrt/lang/directory.c b/src/libponyrt/lang/directory.c
index 5c59f940..e27b4165 100644
--- a/src/libponyrt/lang/directory.c
+++ b/src/libponyrt/lang/directory.c
@@ -116,22 +116,38 @@ PONY_API const char* ponyint_windows_readdir(WIN32_FIND_DATA* find)

PONY_API int ponyint_o_rdonly()
{
+#if defined(PLATFORM_IS_WINDOWS)
+  return _O_RDONLY | _O_BINARY;
+#else
  return O_RDONLY;
+#endif
}

PONY_API int ponyint_o_rdwr()
{
+#if defined(PLATFORM_IS_WINDOWS)
+  return _O_RDWR | _O_BINARY;
+#else
  return O_RDWR;
+#endif
}

PONY_API int ponyint_o_creat()
{
+#if defined(PLATFORM_IS_WINDOWS)
+  return _O_CREAT | _O_BINARY;
+#else
  return O_CREAT;
+#endif
}

PONY_API int ponyint_o_trunc()
{
+#if defined(PLATFORM_IS_WINDOWS)
+  return _O_TRUNC | _O_BINARY;
+#else
  return O_TRUNC;
+#endif
}

#if defined(PLATFORM_IS_POSIX_BASED)

Maybe this should be a separate PR?

[mfelsche]

Yes, i think so too, +1 for separate PR. This PR has been digging up some corpses already, one more is just fine.

[mfelsche]

I rebased, added some docstrings and added manual changelog entries for what has been fixed, changed and added.

If CI is fine this is ready to be merged.

[mfelsche]

Release notes draft

The function File.line(), which was returning a single line from the file as a string has been removed in favor of using File.lines() or FileLines directly.

In order to only get a single line from a file, and not iterate over the whole file, you can use the following code snippet:

let line = my_file.lines().next()?

[mfelsche]

Looking at this from afar i recognize that FileLines does not consume the file i.e. advancing the cursor up to where it was reading from the file in general makes sense, but always resetting it to where it was before might actually be counterintuitive.

The general expectation is (i suppose) that whenever i read something from a file the cursor is advanced by that amount. FileLines is usually reading more, buffering these contents.

It might be more reasonable to advance the cursor beyond the trailing linebreak of the returned line? Any thoughts on this?

[winksaville]

Advancing the cursor would be my expectation.

…

On Wed, Jul 4, 2018 at 12:12 PM Matthias Wahl ***@***.***> wrote: Looking at this from afar i recognize that FileLines does not consume the file i.e. advancing the cursor up to where it was reading from the file in general makes sense, but always resetting it to where it was before might actually be counterintuitive. The general expectation is (i suppose) that whenever i read something from a file the cursor is advanced by that amount. FileLines is usually reading more, buffering these contents. It might be more reasonable to advance the cursor beyond the trailing linebreak of the returned line? Any thoughts on this? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2707 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA-hHGzDXeSDy0Gz3sGWkWgKogFgnyH6ks5uDROIgaJpZM4UJZX7> .

[mfelsche]

Thanks @winksaville for your input. I think the same way. I would just like to ask @SeanTAllen for his, as he was around when the decision for the reimplementation of FileLines was made during a sync call (I remember what you did last summer!).

Gonna change the logic accordingly.

[mfelsche]

There seem to be a segfault when running the stdlib tests on armhf:

PONYPATH=.:/usr/cross/lib build/debug/ponyc --triple=arm-unknown-linux-gnueabihf --cpu=cortex-a9 --link-arch=armv7-a --linker='arm-linux-gnueabihf-gcc' --checktree --verify packages/stdlib
Building builtin -> /home/pony/project/packages/builtin
Building packages/stdlib -> /home/pony/project/packages/stdlib
Building ponytest -> /home/pony/project/packages/ponytest
Building time -> /home/pony/project/packages/time
Building collections -> /home/pony/project/packages/collections
Building assert -> /home/pony/project/packages/assert
Building encode/base64 -> /home/pony/project/packages/encode/base64
Building buffered -> /home/pony/project/packages/buffered
Building builtin_test -> /home/pony/project/packages/builtin_test
Building bureaucracy -> /home/pony/project/packages/bureaucracy
Building promises -> /home/pony/project/packages/promises
Building capsicum -> /home/pony/project/packages/capsicum
Building files -> /home/pony/project/packages/files
Building term -> /home/pony/project/packages/term
Building strings -> /home/pony/project/packages/strings
Building signals -> /home/pony/project/packages/signals
Building random -> /home/pony/project/packages/random
Building cli -> /home/pony/project/packages/cli
Building collections/persistent -> /home/pony/project/packages/collections/persistent
Building crypto -> /home/pony/project/packages/crypto
Building format -> /home/pony/project/packages/format
Building debug -> /home/pony/project/packages/debug
Building glob -> /home/pony/project/packages/glob
Building regex -> /home/pony/project/packages/regex
Building ini -> /home/pony/project/packages/ini
Building itertools -> /home/pony/project/packages/itertools
Building json -> /home/pony/project/packages/json
Building logger -> /home/pony/project/packages/logger
Building math -> /home/pony/project/packages/math
Building net -> /home/pony/project/packages/net
Building options -> /home/pony/project/packages/options
Building ponybench -> /home/pony/project/packages/ponybench
Building process -> /home/pony/project/packages/process
Building backpressure -> /home/pony/project/packages/backpressure
Building serialise -> /home/pony/project/packages/serialise
Building net/ssl -> /home/pony/project/packages/net/ssl
Generating
 Reachability
 Selector painting
 Data prototypes
 Data types
 Function prototypes
 Functions
 Descriptors
Optimising
Verifying
Writing ./stdlib.o
Linking ./stdlib
qemu-arm-static --cpu cortex-a9 -L /usr/local/arm-linux-gnueabihf/libc ./stdlib --sequential
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
make: *** [test-stdlib] Segmentation fault
Exited with code 2

@dipinhora does that ring a bell to you?

[dipinhora]

@mfelsche That output from qemu happens if there is a segfault. Without a backtrace there isn't much to go on to determine what might be occurring.

If you think it might be a transient issue (since all the other armhf builds passed), you can try re-running the failed job.

[mfelsche]

Alrighty, all tests have passed. Thx @dipinhora !

@SeanTAllen or @jemc this is ready for review.

[dipinhora]

@mfelsche due to the release of 0.24.1 and its related changes to the CHANGELOG file, please merge or rebase onto master and adjust your CHANGELOG entries into the appropriate area of "unreleased" in the CHANGELOG file.

[mfelsche]

updated changelog and rebased. :)