Skip to content
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

Add Language.executable_extension property #1139

Merged
merged 6 commits into from
Jul 22, 2021
Merged

Add Language.executable_extension property #1139

merged 6 commits into from
Jul 22, 2021

Conversation

andreyv
Copy link
Member

@andreyv andreyv commented Oct 13, 2019

This property allows to simplify language compilation commands in multiple cases. The compilation commands presented to the user are also easier to understand.

The initial motivation for this change was to be able to use python3 -m zipapp instead of manual archiving for the Python 3 language. Currently this doesn't work, however, because from PEP 441 there must be a __main__.py file in the archive, and zipapp enforces this. But these changes are still useful for multiple other languages.

A couple of notes:

  1. The .pyz extension is specified in PEP 441.
  2. This code doesn't work if there are multiple source files, but no grader:
    main = self.GRADER_BASENAME \
    if self._uses_grader() else executable_filename

    I think it could query the task object instead. Perhaps a new issue can be opened.
  3. The PHP language plugin shows that it may be useful to have an InterpretedLanguage subclass without compilation commands. Currently a language with empty compilation commands fails here:
    stats = None
    for exec_num, command in enumerate(commands):

    because stats is None. On the other hand, it is not clear how this can be reconciled with the case of multiple source files.
  4. Logically, each language type plugin should implement its own updater function, so that the updater script can query all plugins, including out-of-tree ones. As this is overkill, the updater just has a hardcoded list of languages instead.

This change is Reviewable

@codecov
Copy link

codecov bot commented Oct 13, 2019

Codecov Report

Merging #1139 (bbffc51) into master (a446dff) will increase coverage by 0.47%.
The diff coverage is 95.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1139      +/-   ##
==========================================
+ Coverage   63.43%   63.90%   +0.47%     
==========================================
  Files         232      233       +1     
  Lines       17050    17106      +56     
==========================================
+ Hits        10815    10932     +117     
+ Misses       6235     6174      -61     
Flag Coverage Δ
functionaltests 47.82% <70.93%> (+0.52%) ⬆️
unittests 44.11% <54.65%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cms/grading/tasktypes/TwoSteps.py 75.15% <50.00%> (ø)
cms/grading/languages/php.py 95.00% <83.33%> (-5.00%) ⬇️
cms/service/Worker.py 86.73% <86.66%> (+9.31%) ⬆️
cms/db/__init__.py 96.42% <100.00%> (ø)
cms/db/filecacher.py 79.20% <100.00%> (+2.77%) ⬆️
cms/grading/language.py 90.00% <100.00%> (+0.63%) ⬆️
cms/grading/languages/csharp_mono.py 95.45% <100.00%> (+0.71%) ⬆️
cms/grading/languages/java_jdk.py 84.37% <100.00%> (+1.61%) ⬆️
cms/grading/languages/python2_cpython.py 96.77% <100.00%> (+0.10%) ⬆️
cms/grading/languages/python3_cpython.py 100.00% <100.00%> (ø)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2355d6...bbffc51. Read the comment docs.

Copy link
Member

@stefano-maggiolo stefano-maggiolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the current status of TwoSteps? Are we letting it rot away?

cms/grading/languages/php.py Show resolved Hide resolved
cms/grading/languages/python3_cpython.py Show resolved Hide resolved
cms/grading/languages/python3_cpython.py Outdated Show resolved Hide resolved
cms/grading/languages/python2_cpython.py Outdated Show resolved Hide resolved
cmscontrib/updaters/update_43.py Outdated Show resolved Hide resolved
andreyv added 2 commits July 8, 2021 17:36
Language executable file names can now have extensions. This allows to
simplify language compilation commands in multiple cases.

Task type plugins and tests are updated accordingly.
@andreyv
Copy link
Member Author

andreyv commented Jul 10, 2021

What's the current status of TwoSteps? Are we letting it rot away?

Thanks, I updated it as well (somewhat). Even in master, as I can see, TwoSteps won't work with Python etc., because it doesn't call get_evaluation_commands(), but rather just runs ./executable. But at least now it is not broken more than it is in master.

@stefano-maggiolo
Copy link
Member

I had one more comment request then feel free to push to master. Thank you!

@andreyv
Copy link
Member Author

andreyv commented Jul 17, 2021

Thanks! I'll push the changes soon.

@andreyv andreyv merged commit bbffc51 into cms-dev:master Jul 22, 2021
@andreyv andreyv deleted the executable-extensions branch July 22, 2021 16:07
@andreyv
Copy link
Member Author

andreyv commented Jul 22, 2021

Merged.

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

Successfully merging this pull request may close these issues.

2 participants