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

R7RS implementation #273

Merged
merged 31 commits into from Sep 13, 2017

Conversation

Projects
None yet
2 participants
@wasamasa
Collaborator

wasamasa commented Sep 10, 2017

This was pretty fun, I've handed in a dozen bug reports over the course of this, nearly all of which have been resolved in a timely manner. Now comes the hard part for me, dockerization. I've ensured that all tests (save one optional one with metadata in native functions) pass, including self-hosted mode. How exactly shall I proceed regarding test runs, do I need to write a docker file that installs all Scheme implementations and teach Travis to do a run for each?

@kanaka

This comment has been minimized.

Show comment
Hide comment
@kanaka

kanaka Sep 11, 2017

Owner

In terms of Dockerization and testing, for now I think everything in one docker image is probably the least intrusive to the overall build/test process. You can then specify the mode environment variable for the different modes in separate travis phases. High-level support for sub-implementations can be tackled separately.

Would "scheme" be a better directory name for this? SCM seems to be the name of a specific implementation of R4RS/R5RS scheme (https://en.wikipedia.org/wiki/SCM_(Scheme_implementation)) so it seems a bit misleading (and doesn't communicate the scope of what you've accomplished either). I've personally been a bit inconsistent about using file extension versus full name for the directory name, but I would lean towards full-name (and most do follow that) unless its universally recognized by the extension (like vb, tcl, js).

Owner

kanaka commented Sep 11, 2017

In terms of Dockerization and testing, for now I think everything in one docker image is probably the least intrusive to the overall build/test process. You can then specify the mode environment variable for the different modes in separate travis phases. High-level support for sub-implementations can be tackled separately.

Would "scheme" be a better directory name for this? SCM seems to be the name of a specific implementation of R4RS/R5RS scheme (https://en.wikipedia.org/wiki/SCM_(Scheme_implementation)) so it seems a bit misleading (and doesn't communicate the scope of what you've accomplished either). I've personally been a bit inconsistent about using file extension versus full name for the directory name, but I would lean towards full-name (and most do follow that) unless its universally recognized by the extension (like vb, tcl, js).

@wasamasa

This comment has been minimized.

Show comment
Hide comment
@wasamasa

wasamasa Sep 11, 2017

Collaborator

Alright, I'll try to get everything built in one image. About the name, I've considered both "scheme" (somewhat generic, one might assume R6RS) and "r7rs" (doesn't convey the language to lesser experienced people), but didn't consider "scm" being problematic.

Collaborator

wasamasa commented Sep 11, 2017

Alright, I'll try to get everything built in one image. About the name, I've considered both "scheme" (somewhat generic, one might assume R6RS) and "r7rs" (doesn't convey the language to lesser experienced people), but didn't consider "scm" being problematic.

@kanaka

This comment has been minimized.

Show comment
Hide comment
@kanaka

kanaka Sep 11, 2017

Owner

Okay, no problem. Maybe adding an "(scm/R7RS)" parenthetical to README?

Owner

kanaka commented Sep 11, 2017

Okay, no problem. Maybe adding an "(scm/R7RS)" parenthetical to README?

@kanaka

This comment has been minimized.

Show comment
Hide comment
@kanaka

kanaka Sep 11, 2017

Owner

@wasamasa BTW, I just pushed a change to make MODE setting more generic. It's now _MODE and when you add _MODE variable to the top-level Makefile it should be conveyed automatically to the run script and implementation specific Makefile without having to modify anything else in the top-level Makefile.

Owner

kanaka commented Sep 11, 2017

@wasamasa BTW, I just pushed a change to make MODE setting more generic. It's now _MODE and when you add _MODE variable to the top-level Makefile it should be conveyed automatically to the run script and implementation specific Makefile without having to modify anything else in the top-level Makefile.

@wasamasa

This comment has been minimized.

Show comment
Hide comment
@wasamasa

wasamasa Sep 12, 2017

Collaborator

Great, I've adjusted the names and changed it so that running make on its own will set up the necessary dependencies depending on what scheme_MODE is set to. That leaves the Dockerfile, I'm afraid none of the Scheme implementations except for CHICKEN is in any Ubuntu repository and I'm not sure whether their version will even work.

Collaborator

wasamasa commented Sep 12, 2017

Great, I've adjusted the names and changed it so that running make on its own will set up the necessary dependencies depending on what scheme_MODE is set to. That leaves the Dockerfile, I'm afraid none of the Scheme implementations except for CHICKEN is in any Ubuntu repository and I'm not sure whether their version will even work.

@kanaka

This comment has been minimized.

Show comment
Hide comment
@kanaka

kanaka Sep 12, 2017

Owner

@wasamasa Pulling from distro repository is certainly best, but it's acceptable to pull binaries directly from the project site, or if there are no builds, then build them from sources in the Dockerfile. Basically, try and replicate how you got them installed in the first place but in the Dockerfile. Also, it's worth searching for existing docker images for the implementations in question so you can reference the steps from their Dockerfile.

Owner

kanaka commented Sep 12, 2017

@wasamasa Pulling from distro repository is certainly best, but it's acceptable to pull binaries directly from the project site, or if there are no builds, then build them from sources in the Dockerfile. Basically, try and replicate how you got them installed in the first place but in the Dockerfile. Also, it's worth searching for existing docker images for the implementations in question so you can reference the steps from their Dockerfile.

@wasamasa

This comment has been minimized.

Show comment
Hide comment
@wasamasa

wasamasa Sep 13, 2017

Collaborator

The PR should be good to go now, all that's missing is pushing the Docker image and retrying the relevant builds.

Collaborator

wasamasa commented Sep 13, 2017

The PR should be good to go now, all that's missing is pushing the Docker image and retrying the relevant builds.

@kanaka

This comment has been minimized.

Show comment
Hide comment
@kanaka

kanaka Sep 13, 2017

Owner

Okay, I pushed the docker image and restarted the scheme implementations. I'll merge once/if they pass.

BTW, can we set the default mode to one from the list so that make test^scheme works without requiring the mode to be set?

Owner

kanaka commented Sep 13, 2017

Okay, I pushed the docker image and restarted the scheme implementations. I'll merge once/if they pass.

BTW, can we set the default mode to one from the list so that make test^scheme works without requiring the mode to be set?

@wasamasa

This comment has been minimized.

Show comment
Hide comment
@wasamasa

wasamasa Sep 13, 2017

Collaborator

That should already be the case with Chibi, I've set it in several places as the default.

edit: Ah, my bad, I didn't do it correctly in the top-level Makefile. Fixed.

Collaborator

wasamasa commented Sep 13, 2017

That should already be the case with Chibi, I've set it in several places as the default.

edit: Ah, my bad, I didn't do it correctly in the top-level Makefile. Fixed.

@kanaka

This comment has been minimized.

Show comment
Hide comment
@kanaka

kanaka Sep 13, 2017

Owner

Looks like foment times out during the perf test: https://travis-ci.org/kanaka/mal/jobs/275101258 Everything else passed.

Owner

kanaka commented Sep 13, 2017

Looks like foment times out during the perf test: https://travis-ci.org/kanaka/mal/jobs/275101258 Everything else passed.

@wasamasa

This comment has been minimized.

Show comment
Hide comment
@wasamasa

wasamasa Sep 13, 2017

Collaborator

This is weird because I cannot reproduce this with scheme_MODE=foment DOCKERIZE=1 make "perf^scheme". Foment is be roughly at the same speed as Chibi so the timeout isn't inherent to the implementation.

Collaborator

wasamasa commented Sep 13, 2017

This is weird because I cannot reproduce this with scheme_MODE=foment DOCKERIZE=1 make "perf^scheme". Foment is be roughly at the same speed as Chibi so the timeout isn't inherent to the implementation.

@kanaka

This comment has been minimized.

Show comment
Hide comment
@kanaka

kanaka Sep 13, 2017

Owner

Huh, yes, it appears to be something specific to the Travis environment. Running the same command locally works fine for me too:

docker run -it -u 2000 -v `pwd`:/mal kanaka/mal-test-scheme make 'TEST_OPTS=--debug-file ../perf.err' MAL_IMPL=js scheme_MODE=foment 'perf^scheme'

That's pretty weird since the dockerization should remove almost all software differences. That leaves: hardware, kernel, docker engine, environment/permissions. Anything about foment that might be sensitive to hardware features or the kernel version?

Owner

kanaka commented Sep 13, 2017

Huh, yes, it appears to be something specific to the Travis environment. Running the same command locally works fine for me too:

docker run -it -u 2000 -v `pwd`:/mal kanaka/mal-test-scheme make 'TEST_OPTS=--debug-file ../perf.err' MAL_IMPL=js scheme_MODE=foment 'perf^scheme'

That's pretty weird since the dockerization should remove almost all software differences. That leaves: hardware, kernel, docker engine, environment/permissions. Anything about foment that might be sensitive to hardware features or the kernel version?

@kanaka

This comment has been minimized.

Show comment
Hide comment
@kanaka

kanaka Sep 13, 2017

Owner

BTW, if you want to disable just that mode in .travis.yml, I'm fine with merging it. Or it can wait until you debug foment. Either way is fine with me.

Owner

kanaka commented Sep 13, 2017

BTW, if you want to disable just that mode in .travis.yml, I'm fine with merging it. Or it can wait until you debug foment. Either way is fine with me.

@wasamasa

This comment has been minimized.

Show comment
Hide comment
@wasamasa

wasamasa Sep 13, 2017

Collaborator

The only thing I can think of is its setup for stdin/stdout where it tries detecting a tty. Perhaps that check fails and leaves it in a state where the rest of the test rig doesn't work either. I'm fine with deactivating it for the time being and reactivating in CI when the source of the bug has been discovered and fixed.

My only other idea is that the timing code depends on something that works fine on a real machine and fails (like by not ever advancing time) in CI.

Collaborator

wasamasa commented Sep 13, 2017

The only thing I can think of is its setup for stdin/stdout where it tries detecting a tty. Perhaps that check fails and leaves it in a state where the rest of the test rig doesn't work either. I'm fine with deactivating it for the time being and reactivating in CI when the source of the bug has been discovered and fixed.

My only other idea is that the timing code depends on something that works fine on a real machine and fails (like by not ever advancing time) in CI.

@kanaka kanaka merged commit de37556 into kanaka:master Sep 13, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@kanaka

This comment has been minimized.

Show comment
Hide comment
@kanaka

kanaka Sep 13, 2017

Owner

Okay, the 6 remaining have passed. Merged.

Owner

kanaka commented Sep 13, 2017

Okay, the 6 remaining have passed. Merged.

@wasamasa

This comment has been minimized.

Show comment
Hide comment
@wasamasa
Collaborator

wasamasa commented Sep 14, 2017

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