search

rubyforgood / loudoun_codes

A ruby on rails app to replace the PCSquared application used for HSPC coding contests.
MIT License
3 stars 10 forks source link

Crystal SubmissionRunner implemented #114

Closed danielpclark closed 7 years ago

danielpclark commented 7 years ago

Based off of C++ PR

yarmiganosca commented 7 years ago

can you merge upstream master back into this so I can see the updated diff?

danielpclark commented 7 years ago

@yarmiganosca One note about the Crystal language is when there is a compile error we do get the right exit status but Crystal writes to STDOUT rather than STDERR. I don't think this will be an issue for us. Just a good edge case to be aware of.

yarmiganosca commented 7 years ago

If the build phase exit is nonzero, then we show the #err to the participant. If the build phase exit is zero, then we move onto the next phase, where we overwrite whatever our #out call returned with the #out or #err from the next phase's result object. Or the next phase errors and we don't end up writing to the database. Overriding #out on this class accomplishes nothing. We could remove this method override and the behavior of this runner wouldn't change. Which means this code is a liability. Would you mind removing it?

On May 28, 2017 19:34, "Daniel P. Clark" notifications@github.com wrote:

@danielpclark commented on this pull request.

In lib/submission_runners/crystal.rb https://github.com/rubyforgood/loudoun_codes/pull/114#discussion_r118852348 :

  • @entry = EntryFile(source_file)
  • CrystalBuildResponse.new \
  • docker_run('crystal', 'build', '-o', entry.basename, entry.filename, chdir: submission_dir)
  • end
  • def run
  • docker_run("./#{entry.basename}", chdir: submission_dir, in: input_buffer)
  • end
  • class CrystalBuildResponse
  • delegate_missing_to :@result_object
  • def initialize(result_object)
  • @result_object = result_object
  • end
  • def out

There is not content meant for STDOUT when the exit status is not zero. See crystal-lang/crystal#4451 https://github.com/crystal-lang/crystal/issues/4451 . There is no content for out.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rubyforgood/loudoun_codes/pull/114#discussion_r118852348, or mute the thread https://github.com/notifications/unsubscribe-auth/ABq-GikwPlGcugL9Rx-WL-64X_CONlChks5r-gSfgaJpZM4NkevT .

danielpclark commented 7 years ago

You're overlooking the fact that this object is not used in the run phase. It's only run in the build phase and does not effect the run phase. The run phase doesn't have anything overwritten so it's perfectly safe.

Because this is accounted for I won't be removing it.

yarmiganosca commented 7 years ago

I'm not overlooking that. That fact is central to my reasoning. Bluntly, we could remove CrystalBuildResponse#out and it wouldn't change the behavior of SubmissionRunners::Crystal. I pulled this branch, removed this method, and the test suite reported zero failures. Given that, why should we introduce this method into the codebase?

On May 28, 2017 22:44, "Daniel P. Clark" notifications@github.com wrote:

You're overlooking the fact that this object is not used in the run phase. It's only run in the build phase and does not effect the run phase. The run phase doesn't have anything overwritten so it's perfectly safe.

Because this is accounted for I won't be removing it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rubyforgood/loudoun_codes/pull/114#issuecomment-304561221, or mute the thread https://github.com/notifications/unsubscribe-auth/ABq-GmDM11LrMpJgz_x1XF65LmvJyMjbks5r-jERgaJpZM4NkevT .

danielpclark commented 7 years ago

It's written to fix Crystal's results in the way Crystal should have done it in the first place. You know this yet this isn't enough for you.

On the same point you made that it won't change the behavior of SubmissionRunners::Crystal you seem to be really hard headed about it regardless.

Crystal's behavior is broken, I made a fix for it. Seems rather straight forward to me. You may not have a use for the CrystalBuildResponse#out method yourself but personally I like to have tests in place to know and verify that languages are behaving as they should be. Perhaps that can be reason enough for you?

If that's too much for you then I guess we're at an impasse.

If I had written the same tests that you had for Java I would not have discovered this bug in the first place. It has a reason to be.

yarmiganosca commented 7 years ago

Like I said, I took out that method, and all the tests in this branch passed. Given that code is where bugs come from, it seems to me that not having a method around that doesn't actually the affect the outcomes of the system is better than having that method around.

danielpclark commented 7 years ago

Once Crystal itself is fixed that entire class will simply be removed. The code in question doesn't pertain to “Given that code is where bugs come from” at all. That's sounds like stating where babies come from without making a point.

“the outcomes of the system” is not a system that has been fully determined or realized yet. As this is an evolving system which will change as the needs become realized what you're asking for is considered “preference” — of which preferences differ from developer to developer not necessarily being regarded as wrong or right.

The object is to be throw away code when the time comes. I don't get why you've been so stuck on it.

yarmiganosca commented 7 years ago

If the code is to be thrown away when the time comes, then there shouldn't be a problem removing a part of it that is currently useless. Again, please remove the method.

bokmann commented 7 years ago

Just reviewing this material, I didn't realize the CoC PR was in response to a conversation actually happening. To be clear on that, I'm a firm supporter of Code of Conducts, have had one in my other repos for quite some time (check https://github.com/bokmann/business_time), and support Coraline and 'Greater Than Code' via Patreon. My support of CoCs has nothing to do with this thread.

That said, I want our threads to remain professional, tolerant of ideas, patient for understanding, and free of comments, implied by gifs or explicitly said, that call people 'idiot' or say they are explicitly wrong.

My opinion on this - Crystal isn't worth this. I'm a fan of Crystal... It's my topic at the RubyNation conference coming up... but I've never had a student who has even heard of it, let alone proficient enough to desire to use it in a coding contest. Granted, that's one of the reasons we're making this project, but there are much better wins to implement, and none of them are worth crapping on other people about.

If crystal has a problem that makes it not suitable for what we're doing right now, address it upstream. This is an 'angels of the head of a pin' problem at this level right now.

danielpclark commented 7 years ago

I hadn't meant to imply “idiot” by what I had posted. For any implied insults I do apologize.

This discussion has been part of my experience in getting to know Chris. He has cordially asked me to change much of my implementation details in much of my code and I've happily complied when it made sense towards the greater purpose of the project. This discussion thread has largely arisen when I perceived that he crossed a line in micromanaging and I only meant to be clear in that and not insulting in any way.

As a developer I would greatly appreciate having some liberty in how I express my code. What may not have meaning for Chris may clearly have meaning for me and it shouldn't be a hindrance to contributing to OS.

Again I apologize for coming across as unprofessional. @bokmann if you want me to remove the method I will.

bokmann commented 7 years ago

I understand, and will take a fair share of the blame, as my management at R4G Saturday allowed two independent trains of thought to happen on the docker submission.

For complete context, I've known Chris for years, and in person he's the kind of rare genius that always makes me feel in awe, and most of the time makes me feel smarter. He and I talked for several weeks before R4G about the docker implementation of the runners, and I trust his ability to do the right thing. If I'm not fully paying attention to the context of a thread, I'd probably trust his opinion first, including over my own.

I value everyone's contribution, and I think my allowing two threads to solve the same problem was the initial mistake that allowed this conflict to happen, especially since I could have said up front 'Chris has been thinking about this for a couple weeks, and I'll probably trust anything he does for the runner issue over even my own contribution'. I totally understand the desire to express yourself; this might not have been the best task to do it on given those circumstances.

Reading the history, I see a suggestion to redirect the shell output to isolate our code's behavior from this problem. That seems the best approach to me to isolate our code from upstream's behavior.