autolab / Autolab

Course management service that enables auto-graded programming assignments.
http://www.autolabproject.com/
Apache License 2.0
752 stars 214 forks source link

MOSS does not properly handles tgz and zip files #271

Closed ymzong closed 9 years ago

dlbucci commented 9 years ago

It always opens a TarReader. Check out submissions_controller#getFileAt. Also, it doesn't check for tgz extensions, which can be a problem, and the external_tar upload seems to still try to use libarchive (there's a Archive function call in there), so that also needs to be updated. At this point, it may be nice to come up with a robust way of reading archives. I think this might even be an actual use case for a lib/modules module!

ymzong commented 9 years ago

Good point @dlbucci!

I think it would be great if we separate the methods for handling archives to another library module -- I don't like to see these archive handling code everywhere.

Will start working on this and submit a PR if no one objects.

dlbucci commented 9 years ago

Go for it.

RONNCC commented 9 years ago

Stupid question but Shouldn't it be a modification to the libarchive call rather then a removal considering it supports most compressed formats ?

ymzong commented 9 years ago

@RONNCC We are not removing the calls to libarchive.

dlbucci commented 9 years ago

@ymzong I thought we were? We couldn't get 'libarchive-ruby' installed, so we've been trying to remove it? I think libarchive-ruby isn't really being maintained either, and I've seen suggestions not to use it. That said, I'm pretty sure it simplifies things on our end, so if we can use it, there's no reason not to.

RONNCC commented 9 years ago

The thing is what library is as comprehensive to replace it :/

dlbucci commented 9 years ago

I'm not sure there's a single library for it, but we've managed to get away with this: https://github.com/autolab/Autolab/blob/master/app/controllers/submissions_controller.rb#L769 (Honestly, that code is godawful and should be removed or rewritten to be not terrible.)

ymzong commented 9 years ago

@dlbucci @RONNCC Oh alright, I was confused. Yes we are removing the calls to libarchive, but actually we are already not using them. I thought our next step is to refactor out the code that deals with archives, like submissions_controller.rb:769.

RONNCC commented 9 years ago

Ehh ok well I think the next step would be to add pass the zip option and merge that before refactoring :P

ymzong commented 9 years ago

@RONNCC: As @dlbucci mentioned in the email, our current issue is not that zip/tgz options are not passed, but that regardless of the archive type, the code handles the file as a tar.

In order to handle zip and tgz without refactoring, we will need to add the ugly code (as in submissions_controller.rb:769) to admins_controller.rb, which is far from an elegant solution.

RONNCC commented 9 years ago

Can we use that as a monkey patch before refactoring

dlbucci commented 9 years ago

@ymzong Isn't this a call to libarchive? https://github.com/autolab/Autolab/blob/master/app/controllers/admins_controller.rb#L448 Also, that code in submissions_controller has already been more or less duplicated in submission.rb as well (get_filename_in_archive_at). So yeah, I'd make an Archive model that can be required in the files and gives us a nice interface for dealing with archives. @RONNCC wouldn't be the first time we've done that with this exact code...

dlbucci commented 9 years ago

Hey, that's the whole reason we're open source, man :)

RONNCC commented 9 years ago

bump

icanb commented 9 years ago

@RONNCC This is on the list for the next milestone which mean that it will take us a week or more to complete. If it's really urgent for your case, feel free to send a PR.

dlbucci commented 9 years ago

Fixed by #357

RONNCC commented 9 years ago

is this version of autolab running now? // when will it be switched in?

dlbucci commented 9 years ago

Just put it in production.

RONNCC commented 9 years ago

awesome

RONNCC commented 9 years ago

errors :(

RONNCC commented 9 years ago

http://i.imgur.com/9sjEeqG.png

dlbucci commented 9 years ago

Remove macosx file system bs (like that folder) and try again. I'll fix this in the morning.

RONNCC commented 9 years ago

it doesnt seem to handle the directory after extracting ironically - i can't, too many mac users. Can it ignore private _* directories? / is there anyway to ignore OS-specific things e.g. . files from linux and __MACOS from Mac, random thumbs.db from Windows, blah~ from linux, etc.

or can we just send all of them to MOSS like it did in the past?