-
Notifications
You must be signed in to change notification settings - Fork 276
Autocheck for new DOMjudge releases #3055
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -38,6 +38,7 @@ | |||||
use Doctrine\ORM\NoResultException; | ||||||
use Doctrine\ORM\Query\Expr\Join; | ||||||
use Doctrine\ORM\QueryBuilder; | ||||||
use Exception; | ||||||
use InvalidArgumentException; | ||||||
use Psr\Log\LoggerInterface; | ||||||
use ReflectionClass; | ||||||
|
@@ -63,6 +64,7 @@ | |||||
use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface; | ||||||
use Symfony\Component\Security\Core\User\UserInterface; | ||||||
use Twig\Environment; | ||||||
use z4kn4fein\SemVer\Version; | ||||||
use ZipArchive; | ||||||
|
||||||
class DOMJudgeService | ||||||
|
@@ -107,6 +109,8 @@ | |||||
protected string $projectDir, | ||||||
#[Autowire('%domjudge.vendordir%')] | ||||||
protected string $vendorDir, | ||||||
#[Autowire('%domjudge.version%')] | ||||||
protected readonly string $domjudgeVersion, | ||||||
) {} | ||||||
|
||||||
/** | ||||||
|
@@ -1671,4 +1675,59 @@ | |||||
->getQuery() | ||||||
->getResult(); | ||||||
} | ||||||
|
||||||
public function checkNewVersion(): string|false { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: add a comment what this function should return |
||||||
if (!$this->config->get('check_new_version')) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return false; | ||||||
} | ||||||
Check failure on line 1682 in webapp/src/Service/DOMJudgeService.php
|
||||||
$versionLocalString = explode("/", str_replace("DEV", "-prerelease", $this->domjudgeVersion))[0]; | ||||||
$versionLocal = Version::parse($versionLocalString, false); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why tarball in the user agent and not DOMjudge like we do when shadowing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We also wanted to know which installmethods are out there, so next step is a config variable but I had no idea on how to set that (yet) I suspect a make option.. |
||||||
$versionUrl = 'https://versions.domjudge.org'; | ||||||
$options = ['http' => ['method' => 'GET', 'header' => "User-Agent: tarball/" . $versionLocalString . "\r\n"]]; | ||||||
$context = stream_context_create($options); | ||||||
$response = @file_get_contents($versionUrl, false, $context); | ||||||
if ($response === false) { | ||||||
Check failure on line 1689 in webapp/src/Service/DOMJudgeService.php
|
||||||
return false; | ||||||
} | ||||||
$versions = json_decode($response, true); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: add a comment to explain what format you expect the server to return |
||||||
/* Steer towards to the latest patch first | ||||||
* the user can see on the website if there is a new Major/minor themselves | ||||||
* otherwise the latest minor, or Major release. So the user might make the upgrade path: | ||||||
* DJ6.0.0 -> DJ6.0.6 -> DJ6.6.0 -> DJ9.0.0 instead of | ||||||
* -> DJ6.0.[1..6] -> DJ6.[1..6] -> DJ[7..9].0.0 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole loop could also be done by sorting the versions lexicographically and taking the next one higher than the local version. This could replace all three checks? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't that give the path over all patches, minor plus patches in that minor etc? Basically the 2nd path I mentioned. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah you might be right but we could solve that by only exposing specific versions on the versions site? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if the array would only the latest patch release per But I get the point that we should do something smarter so the code is less duplicated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We still need the loop as at best we can only clean the list to always have the highest patch version in the list, We could ofcourse do this math on the version site and basically only let the server do a lookup there so: But that also feels expensive, it just removes the calculation to another server. I did some cleanup though and I think this is already more what you want. Although I'm not 100% if it will now not also alert for the current version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we not return just the newest version here? |
||||||
*/ | ||||||
$latestPatchString = $versionLocal; | ||||||
if (isset($versions[$versionLocal->getMajor()][$versionLocal->getMinor()])) { | ||||||
$latestPatchString = Version::rsortString($versions[$versionLocal->getMajor()][$versionLocal->getMinor()])[0]; | ||||||
$latestPatch = Version::parse($latestPatchString); | ||||||
if (Version::compare($versionLocal, $latestPatch) < 0) { | ||||||
return $latestPatchString; | ||||||
} | ||||||
} | ||||||
$latestMinorString = $versionLocal; | ||||||
if (isset($versions[$versionLocal->getMajor()])) { | ||||||
$highestMinorInMajor = array_keys($versions[$versionLocal->getMajor()]); | ||||||
rsort($highestMinorInMajor); | ||||||
$latestMinorString = Version::rsortString($versions[$versionLocal->getMajor()][$highestMinorInMajor[0]])[0]; | ||||||
$latestMinor = Version::parse($latestMinorString); | ||||||
if (Version::compare($versionLocal, $latestMinor) < 0) { | ||||||
return $latestMinorString; | ||||||
} | ||||||
} | ||||||
$latestMajorString = $versionLocal; | ||||||
try { | ||||||
$highestMajor = array_keys($versions); | ||||||
rsort($highestMajor); | ||||||
$highestMinorInMajor = array_keys($versions[$highestMajor[0]]); | ||||||
rsort($highestMinorInMajor); | ||||||
$latestMajorString = Version::rsortString($versions[$highestMajor[0]][$highestMinorInMajor[0]])[0]; | ||||||
$latestMajor = Version::parse($latestMajorString); | ||||||
if (Version::compare($versionLocal, $latestMajor) < 0) { | ||||||
return $latestMajorString; | ||||||
} | ||||||
} catch (Exception $e) { | ||||||
return false; | ||||||
} | ||||||
return false; | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,6 +93,13 @@ | |
</li> | ||
</ul> | ||
{% endif %} | ||
{% if new_version_available %} | ||
<ul class="navbar-nav"> | ||
<li class="nav-item nav-link"> | ||
<i class="fa-solid fa-ship fa-2x"></i> New release {{ new_version_available }} available. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add link? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To do that I need to make it unraw, I'll need @nickygerritsen for that if we want this. |
||
</li> | ||
</ul> | ||
{% endif %} | ||
|
||
<ul class="navbar-nav ml-auto"> | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.