davidje13 / koth-webplayer

Framework for King-of-the-Hill Javascript competitions
MIT License
7 stars 7 forks source link

Support running inside nodejs #22

Closed eaglgenes101 closed 6 years ago

eaglgenes101 commented 6 years ago

A few months ago, I started putting in work to make the project work on node.js. Now the scope of this change turned out to grow, but I was still able to get to the top. There's probably some loose ends somewhere around, but I looked over it and find it okay, so it's coming in. There may be ussies, just as with any change of this magnitude.

20

davidje13 commented 6 years ago

Wow, impressive. This will need quite a bit of looking over before I can merge it, and I see that it's brought in a lot of dzaima's changes too (which might be better as separate PRs), but support for a server-side runtime is great!

I'll start looking over it next week, probably (though beware; usually when I promise to look at things in the future my timescales slip somewhat). In the meantime, if you want to help make this easier to review I suggest looking in to squashing your commits into more logical changes (e.g. commit messages like "My mistake!" should probably be squashed into whatever commit caused the mistake in the first place, to make the changes easier to follow). That should also help to unpick which changes are from your work and which have come in from dzaima's other experiments (some of which aren't ready to be pulled).

dzaima commented 6 years ago

From my tiny looking at the changes, eaglgenes101 seems to have reverted/fixed up many of my commits. I, too, was worried about him merging my changes in the official project :p

eaglgenes101 commented 6 years ago

Mostly, I wanted to pull dzaima's changes for improvements in the correctness of the challenge runner. I refined the skip mechanism to remove some of its bugs, and stripped out Formic-specific functionality from the base code.

The change also renames the bespoke module mechanism from require to requirejs to avoid name collisions with Commonjs requires (which are built into nodejs), and backward incompatibly changes the submission building interface to encourage more encapsulated submission running, which helps with security and performance of sandboxing. It also makes wide use of template strings to not have as messily built codestrings.

More backwards compatibly, the Mann-Whitney U test replaces the Kolgmorov-Smirnov test as the non-parametric significance test, and the buildFunctionFinder is exposed as a public interface method.

davidje13 commented 6 years ago

Due to the large number of changes in this PR, I'm going to merge it into a non-master branch and have a go at tidying it up and splitting it into smaller chunks. I think there's a lot of valuable stuff here, but I want to pull it in on a piece-by-piece basis.

Beware that I'm probably going to end up doing a lot of force-pushing to the branch to simplify the history. This PR will get closed, but the changes won't be part of the project until they're merged into master from that branch. I'm sure there are better ways to do this but I'm not exactly a pro at git.

davidje13 commented 6 years ago

I've tidied up the commit history by rebasing it, and I have removed some of the commits which were later undone, and some which were obviously unrelated to this change. I'm far from finished, but you may want to take a look to make sure I haven't dropped any important changes. You can see the branch here: https://github.com/davidje13/koth-webplayer/tree/eaglgenes101-pr

I haven't started checking for correctness yet; I'm just aggregating changes to make this more manageable.

I'll still be force-pushing to that branch as I tidy things up so don't use it as a base for any further commits just yet!

For future reference, the command I used to reshuffle this was:

git rebase --interactive 560a2db

(560a2db is the SHA of the common ancestor commit for all the various branches)

I then dropped any commits which were problematic (e.g. the commit which deleted non-formic files, the graph commits which were later un-done anyway, and a few unrelated features), and went through the rebase manually patching up any merge conflicts. I've squashed a couple of commits already, but now I plan to squash many more and give them more descriptive commit messages.

eaglgenes101 commented 6 years ago

We haven't merged the nodejs runtime branch to main yet. Any more blockers before the nodejs runtime can be merged in?

davidje13 commented 6 years ago

I still need to look over it some more before merging to master. As you probably noticed I've been picking out the related features and merging those separately to reduce the size of this change.

Next on my list is to look over the skip-to-frame functionality and merge that, since I think the nodejs runner relies on that feature for its own purposes. I suspect that one might need a couple of fixes in a similar manner to the statistics one, but I haven't started looking at it yet. I'm not 100% sure I've finished finding all the skip-to-frame changes from the later commits.

After that, there are 4 predominantly nodejs commits on the branch which also touch non-node files. A lot of that's fine, since the node changes required changes elsewhere too, but some seems to be not directly related (occasionally made worse by my heavy-handed merging early on in this process!)

In all, it's going to need a bit more of my time, and the next day I can throw a chunk of time at this is probably Sunday. If you want to speed things up, it would be great if you could check whether I've found all the skip-to-frame changes in cbd506b05a3fb4f0b2f88cd0df982373bfa8aab5, or if I missed anything which was patched-up in a later commit.

But overall don't worry; these are great changes and I'm still working on getting them merged!