gracelang / minigrace

Self-hosting compiler for the Grace programming language
39 stars 22 forks source link

Revised IO Module #246

Closed Dmitri-2 closed 6 years ago

Dmitri-2 commented 7 years ago

This update includes revisions to the io module, a test for the io module, and some refactoring to gracelib.js. I think I addressed all comments and concerns on the previous pull request for this, but we should not merge this if there is anything else that needs to be fixed. Travis shows the first build as failing, but this is due to the build exceeding the time limit for the job, and not any actual defects in the code.

Travis CI Error: "No output has been received in the last 10m0s ... The build has been terminated."

apblack commented 7 years ago

I think that you have broken output, at least on Travis. Does make self.test still work locally for you?

Dmitri-2 commented 7 years ago

I did a clean clone of the io-module branch, and make self.test worked fine, passing all tests. (My main repo did not, but I think that is due to local changes I have done by hand, like changing file permissions.)

It looks like the new Travis build ran fine, but just in case, here are the only changes that I think might be capable of causing problems:

  1. I changed typeof(process) !== "undefined" to !(typeof global === "undefined") This change may have had some unintended consequences, as I changed all of the instances of this code in gracelib. (I refactored everything to use the inBrowser variable for this check.)
  2. I added return GraceDone; to the close method for the command line part of io module. I don't think this would cause any problems, but it seems like the only change that I made to the command line functions.
apblack commented 7 years ago

So I re-ran the build on travis, and this time it passed without a hitch (in 31 minutes). Very strange.

I've built this, and your tests run fine in my browser. But I noticed that a different suite of tests runs at the command line. Why aren't the tests the same?

Dmitri-2 commented 7 years ago

That's good! The reason that I made two test suites for the io-module is that the command line and browser implementations are very different at the moment. The browser version implements the io module in the way that it is described in the specification; however, the command line version only implements critical functions such as write(_), read, and getline. (And some of them are limited in their function. For example, read will not return content that has just been written to the file. The only way to use read to check if content was really written is to create a new fileStream to the file). Additionally, the flags for opening files are slightly different, read/write mode flag in the browser would be rw while the command line flag would be w+.

Writing all of this up, I am understanding that you wanted both of these versions to be the same, and to implement the same functionality. Hence, I apologize for not completing the command line version yet – I was really focused on getting the browser version polished and ready to go. We can either close this pull request until that update is completed, or we can merge this pull request and then open a separate one for the command line io module changes. In either case, I will try to have this fixed by the middle of next week.

Dmitri-2 commented 7 years ago

The IO Module addition should now be ready to go, with all of the methods and behaviors required by the specification. Minigrace builds fine on my machine with the changes proposed in this pull request. However, the build appears to stall on Travis during the make self.test command. This command runs correctly for me locally, and I haven't been able to replicate the build stall that occurs on Travis.

However, one method that still lacks proper implementation is the TTY check on a FileStream. TTY in Node JS requires a readStream or a writeStream that dynamically write a file. In the io module, in order to only write the file to the file system on file close, the more simple synchronous write method is used – writeSync(). It writes the whole file out at once, rather than incrementally. The same goes for reading a file: the entire file is read into the Grace object and then the io module works with its contents internally, rather than incrementally reading it using a stream. Hence, I removed the "isatty" from the io module test where it was working with an actual TTY stream in "/dev/tty". This was causing the test to stall indefinitely, likely due to the synchronous read method attempting to read it. In terms of the isatty method on a fileStream, I have set it to always return false for now.

apblack commented 6 years ago

These changes have been incorporated. I'm not sure why github thinks otherwise.