Remove extraneous machinery left over from OmniBuilder

[yarmiganosca]

@danielpclark

None of Docker::EntryFile, Docker::InputFile, Docker::OutputFile, Docker::ResultFile, Docker::IOFile are necessary. We don't need to build &/ run in a temp directory--building in the submission directory and persisting any compilation artifacts provides more of an audit trail should that be necessary. Everything else these classes provide is redundant given that SubmissionRunner::Base#source_file returns a Pathname object. This is born out by the fact that the Java runner, which uses none of this machinery, works.

Support::InterpretativeLanguage::MockResult is a good solution to the design flaw I left in SubmissionRunner::Base, and it should be moved into the runner namespace.

Support::InterpretiveLanguage itself strikes me as too much DRY too quickly. We've only had this domain model in our heads for a weekend. I think it's too early to completely remove the docker_run invocation from Runners. It might be a good idea down the road, but I don't think extracting it from only a certain set of Runners is the right move. There's probably a more comprehensive abstraction that lets us pull that invocation out of all the Runners, but we probably will be less likely to see it if we provide an abstraction to a subset now.

[danielpclark]

FYI: I'm not angry, I'm not argumentative, I'm merely openly discussing design and implementation. :dark_sunglasses:

Support::InterpretativeLanguage::MockResult is a good solution to the design flaw I left in SubmissionRunner::Base, and it should be moved into the runner namespace.

Thanks :+1:

Regarding Docker::EntryFile, Docker::InputFile, Docker::OutputFile, and Docker::ResultFile — these are simple abstractions to make the use of specific Pathname kinds of objects more elegant to work with. In all my learning of O.O. over the years these kind of abstractions are a good thing. Are the necessary? … no, there is no need for object oriented anything really. It's just a way to do things.

As far as their added (and not necessary) responsibility of creating temp files the helpers generated in lib/docker/helpers.rb are the only place that would do this should the file not exist. This was intended for if the code was handed a string of source code directly from the database to just work as a file. I'm okay with that part of it being refactored into either a different kind of implementation and perhaps not creating temp files at all. The functions Docker::Helpers#EntryFile, Docker::Helpers#InputFile, Docker::Helpers#OutputFile, and Docker::Helpers#ResultFile were implemented to behave just as Kernel#Array does as a type safety enforcer. During the several refactors I've done the InputFile, OutputFile, and ResultFile types have become unused and will be removed in a future refactor. But I'd like to keep EntryFile, albeit perhaps to improve upon.

As for the temporary file block code I implemented — I'm all for refactoring that out of existence. Each of these steps were taken in order to guarantee to me that things were working as I expected since I had so much trouble just getting the alternative docker execution to run. Only when I had peace of mind about the other major parts of the code working as I intended was I able to figure out how to get the proper Docker implementation going. There's nothing quite as nice as being able to have that peace of mind. I'm okay with removing the temp_file_block method. But at the moment my removing it to point directly at a file causes it to break so I'll take a look at that when I have more time I want to give to it.

I'm open to discussing Support::InterpretiveLanguage. Personally I don't know of any interpretive languages that won't work with this. A simple executable file.kind format. If you can show me a good reason why this would not work with other interpretive languages then I'd see a point for making this less DRY. So for now I think this would be an exception to “too much DRY too quickly”. It does the job well, it fits the bill for its intended purpose, and doesn't violate common best practices.

I'm not a person with “Primitive Obsession” and personally like clarity with higher abstractions. Having something like file.basename(file.extname).to_s in public methods that developers have to read is a bit much, especially if that bit of code is used a lot. The file kind of object should have a higher abstraction in this case IMHO.

[yarmiganosca]

In re: EntryFile, why support an extra class that provides strictly fewer methods than Pathname? I can see no benefit to an extra class which delegates every single method call to a class we're already using elsewhere, and with only trivial modifications. Pathname implements #extname, #basename, #to_path, #to_s, & #read. The only IOFile method it doesn't implement (that we still need) is #path, but again, it implements #to_path, and IOFile#path is an alias of IOFile#to_path, so we're fine there too. The only things it still does are:

• return #basename as a String, which is worse than Pathname#basename because you're destroying type information sooner than you have to (ideally, do it as late as possible)
• give a different, but not usefully different, constructor error from Pathname#initialize
• provide a layer of indirection

IOFile takes choices away: we can no longer decide when to coerce #basename without converting back to a Pathname, which Pathname doesn't make us do. And it gives us no increase in power or expressiveness in exchange for that loss of freedom. This isn't about paradigms like OO or FP. This is about design: we have a design element that isn't providing us with anything. Therefore, there's no reason to keep it.

In re: all interpreted languages sharing a common cli pattern, here are some counterexamples off the top of my head:

• I don't think any Smalltalk is executable file.kind, because Smalltalks aren't CLI-centric at all. Pharo Smalltalk, the newest and seemingly strongest (in terms of community) Smalltalk definitely doesn't start that way.
• Steel Bank Common Lisp (SBCL) takes a --script argument instead of a positional argument.

Heck, some languages are embedded in others:

• Bloom's only current implementation, Bud, straight up uses ruby to launch from command line.
• TLA+ launches with java.

That's to say nothing of times where we might have to (after trial and error) start some languages with command line parameters. Nodejs and Perl are both infamous for this, but any interpreted language that requires an image (Smalltalks, as well as Factor, come to mind here) will probably need that specified as a command line option.

In re: Remaining issues with the SubmissionRunner::Base api, here are the current things that could be improved in SubmissionRunner (in no order): 1 build phase being required is a glaring design hole 2 chdir and in are going to happen in every run phase. This we know. They should be extracted. 3 the permission adjustment in Java::build should probably be absorbed into Base. 4 source_file.basename.to_s is a bit much

As I mentioned above, MockResult is a good part of a solution to 1. 4 is pretty easy to solve by changing Base#source_file to apply the .basename invocation for us (since we only use the relative path in Runners), and by having Base#docker_run map #to_s across the pieces it receives so that descendant Runner classes don't have to. This turns source_file.basename.to_s into source_file. Once 1 is solved, we probably have somewhere to put the submission_dir.chmod(0777) invocation, and thereby solve 3. 2 can be solved with a defaults hash and a current_phase method.

1-4 addressed: https://github.com/rubyforgood/loudoun_codes/pull/116

Once that's taken into account, the cost of making lots of the Runners have a different developer experience isn't worth the small savings you get by only making developers provide the executable path instead of the executable path and the source file path. If we were making them provide them separately, then providing them both would constitute more boilerplate. But we're asking the developer for both the command and the file in a single place, and we're using enough skeuomorphism (that #docker_run looks like docker run isn't accidental) that the design itself prompts them for the file path--the names and flow of the code itself helps them remember.

[danielpclark]

Brilliant! Chris you've given a very well written and thought out answer. With all you've written I've come to agree with you on all your points.

One point in favor of the name EntryFile is merely having the code be slightly more legible. So what I propose is we can take away all of it's current methods and design and simply do: class EntryFile < Pathname; end . This will remove any of the drawbacks you stated and would make a good place to use for a couple of additional methods such as what you wrote for your java_class method - as I reuse that code for all my compiled output targets. Perhaps EntryFile could have the method called build_target.

def build_target
  self.basename(self.extname)
end

You can keep the exact same solution you proposed for calling basename and to_s in just the ways you have stated.

---

One helpful FYI I learned from implementing the temp_file_block is that any files we do end up making in our code base from a string for docker to consume need to at least have permissions of 0604 for it to work.

[danielpclark]

I see that you've self assigned this #116