Site upgrade #2398
Comments
|
So, I see a good endpoint here as:
|
|
One desirable endpoint is that we develop a procedure for doing such site upgrades, so that we can do them as necessary in the future. |
|
Regarding:
It sounds like what you're really looking for here is an audit --- you want someone to look at every piece of software we're running, propose a current version, and make a plan for upgrading to that current version. But you're still not really saying why. I'm seeing this as:
And, okay, if step 1 is to do an audit, i'm happy to do an audit as step 1. But i want to be able to say stuff like: "We're running MySQL X, the newest MySQL is Y; MySQL X is still supported, so there's nothing to do here," and i think the onus should be on someone to say there's an actual benefit to upgrading beyond "Y is greater than X and big numbers are better." Note that if version X is no longer getting minor revision security updates, fine, that is a reason. But most major software continues to support old versions. What i'm saying here is not so much an argument with anything, as a notice of how i would intend to proceed with this. So if you have specifics like "We need PHP Y because PHP X is messing us up in the following way," the most valuable thing you can do is put specific information about why this issue is needed, on this issue. |
|
Here are the current problems that we are currently experiencing (off the top of my head), due to the old versioning of our software products:
|
|
Okay, so, one note is that i'm not planning to integrate R as part of this upgrade --- i understand that wanting to add R is one of your reasons for wanting the upgrade to get done, but it is a separate thing. I think the two major asks here are:
So i'm going to call those things the acceptance criteria for this issue (and i agree that it almost certainly makes sense to do them together rather than separately, because either of debugging old Ubuntu + new PHP or old PHP + new Ubuntu is likely to be more hassle than it's worth, though i'll probably take a few minutes to test that theory). |
|
Okay, that sounds fine. I've tested on stats.dev that Xenial is okay for running R, so anything at least that recent will work for a subsequent installation of R. |
|
I have a virtualbox xenial instance that was able to get far enough to run phpunit. Here's the phpunit output so far --- it's possible that there are simple "tolerate X" type flags whose defaults have changed between php5 and php7, that we could modify. But i figured i'd throw these out here because some of them may simply be code shortcuts that would be easy for us to fix. I'll work on them myself under that assumption as time permits, but Shadowshade you may be interested in looking yourself: |
|
The first two errors are easily fixed by having the catch expect // james: note that end normally shifts the pointer, so use
// call_user_func and array_values to avoid changing the original array
$lastLogEntry = call_user_func('end', array_values($game->actionLog));so i am definitely going to rely on Shadowshade to reverse-engineer what we were trying to accomplish there. |
|
Okay, here we go. In PHP 5.3.0, using call_user_func with a non-reference call when a reference call is expected now creates an E_WARNING (see http://php.net/manual/en/function.call-user-func.php). Looking at the code again (and reading https://stackoverflow.com/questions/3687358/whats-the-best-way-to-get-the-last-element-of-an-array-without-deleting-it), we'd probably both prefer replacing that nasty line and the associated comment (all three times that they appear) with |
|
Sounds reasonable. I'll try it. |
|
That seems to have helped. Here's the current state: |
|
The following change to BMInterfaceGame fixes the remaining error, and IMO is a good idea anyway: $ git diff src/engine/BMInterfaceGame.php
diff --git a/src/engine/BMInterfaceGame.php b/src/engine/BMInterfaceGame.php
index 340c32b..f50ad38 100644
--- a/src/engine/BMInterfaceGame.php
+++ b/src/engine/BMInterfaceGame.php
@@ -1035,6 +1035,10 @@ class BMInterfaceGame extends BMInterface {
) {
try {
$game = $this->load_game($gameId);
+ if (!$game) {
+ $this->set_message('Invalid game');
+ return FALSE;
+ }
if (!$this->is_action_current(
$game,
BMGameState::CHOOSE_AUXILIARY_DICE,
leocrotta,[~/src/git/cgolubi1/buttonmen-2398_site_upgrade],09:28(0)$ |
|
Yep, that looks like a good idea. |
|
Hey, check this out --- after months (possibly hours) of toil, i have gotten circleci to fail similarly to how my local phpunit is failing. In order to get circleci to run under xenial, i had to switch us from the "machine" executor to the "docker" executor. To my knowledge, there's no downside to the docker executor except that i didn't want to debug getting it working, but now that i've done so, unless we hit any problems in the post-phpunit steps, we should be good to just use it from now on. |
|
I stood up a dev site for this at 54.159.124.54 --- irilyth, mind setting that up as "xenial"? (If i get a chance to setup my own route53-updater, i will, but one thing at a time, so if you have it handy, go for it.) |
|
Shadowshade: if you want to see the phpunit failures interactively, you can login to that site and run: In theory you can also pull from my branch and setup your own local test site under vagrant/virtualbox, of course. |
|
Other testers, if you want to check the dev site out and play some games, that would be great. My working assumption is that unit tests are failing because shadowshade took shortcuts when setting up objects for some of the tests, and php7 is stricter in ways that outlaw some of those shortcuts. That is, the problem is in the tests, not in the code. So far, that's proved true in all the tests we've root-caused. Anything weird people see in gameplay or the UI should be reported. |
|
By the way, i should mention that (a) i'm currently optimistic about our ability to get this working pretty soon, as a result of which (b) i think we should stay in merge-freeze mode until we either get this over the fence or conclude it's not going to be that easy. Given that we expect to do this as a site-rebuild/cutover with downtime, it'll simplify matters considerably if we're not trying to apply any database changes at the same time. So i think we should ignore the release schedule in the interest of getting this one change ready to go and going as soon as we think we're ready. |
|
This change makes $ git diff
diff --git a/test/src/engine/BMGameTest.php b/test/src/engine/BMGameTest.php
index 5b08146..45c0963 100644
--- a/test/src/engine/BMGameTest.php
+++ b/test/src/engine/BMGameTest.php
@@ -5874,7 +5874,7 @@ class BMGameTest extends PHPUnit_Framework_TestCase {
$this->assertEquals('V', $game->activeDieArrayArray[1][2]->swingType);
$this->assertEquals(11, $game->activeDieArrayArray[1][2]->swingValue);
$this->assertEquals(6, $game->activeDieArrayArray[1][2]->max);
- $this->assertTrue(isset($game->activeDieArrayArray[1][2]->value));
+ $this->assertNotNull($game->activeDieArrayArray[1][2]->value);
$this->assertEquals(array(40.5, -12.0), $game->roundScoreArray);
}and when i modify the assert so it lets me look at the actual value, it's some number like 1, or 3, or 4 --- it's the value of a die which was rerolled, like you'd expect. So i'm guessing the behavior of I guess i have a couple of comments about this:
|
|
My shot in the dark was wrong. If i modify the test in the location we're looking at to include: $this->assertEquals(6, $game->activeDieArrayArray[1][2]->max);
$this->assertTrue(isset($game->activeDieArrayArray[1][2]->max));the test fails on the second line. So [Incidentally, my new shot-in-the-dark theory is that this is related to the fact that: error_log(var_export($game->activeDieArrayArray[1][2], $return=TRUE));fails with: There's something structurally suspect about the BMDie object, and i don't think that's new (i've noticed the var_export thing before). If we can get through this upgrade without debugging it, so much the better as far as i'm concerned.] |
|
|
|
Yeah, so this has got to be a result of the changes to the behavior of There's a comment on the former page which seems instructive "...will always echo 'false'. because the isset() accepts VARIABLES as it parameters, but in this case, $foo->bar is NOT a VARIABLE. it is a VALUE returned from the __get() method of the class Foo. thus the isset($foo->bar) expreesion will always equal 'false'." And we can trivially verify that this is the issue (WARNING: when you read this next bit, you will involuntarily say "oh my god WHY are we using PHP?" out loud, so make sure that's okay with your neighbors). This code? $myvar = $game->buttonArray[0]->name;
$this->assertFalse(empty($myvar));
$this->assertFalse(empty($game->buttonArray[0]->name));Guess which line that fails on? Yep. The last one. TL;DR: let's see how hard it would be to remove |
|
Argh. That sounds like more than a trivial amount of work. I'll take a look when I can. |
|
...yeah. I was going to say it's not that bad, but in fact it sort of is that bad. Looking at non-test code: As far as i can tell, item 10 at https://www.toptal.com/php/10-most-common-mistakes-php-programmers-make seems pretty relevant to us. (And i don't see why it's not also an issue with Anyway, before you go off and boil the ocean, we should talk on IRC about whether boiling the ocean is actually necessary --- i can think of a couple of options that might be less painful than replacing all occurrences of these two functions one by one. |
|
Also potentially relevant --- https://stackoverflow.com/questions/21227585/what-is-the-difference-between-isset-and-isset Presumably we knew something about |
|
I always knew that AI would take over the world. |
|
(copied from IRC and slightly formatted for easier comprehension) So, I'm currently working on getting my PHPUnit tests up and running. I note that the xenial instance is running PHPUnit 5.1.3. I also note that PHPUnit 5 is already out of support Add to that the fact that the transition from PHPUnit 5 to PHPUnit 6 requires changing all the to and I ask myself the obvious question: do we want to be using PHPUnit 5 or 6 (noting that PHPUnit 7 doesn't support PHP 7.0 any more) and the obvious corollary: Is it worth it for me to go to the effort of doing a two-step PHPUnit test upgrade on my AMP stack, or would it be easy enough for us to move the xenial instance up to PHPUnit 6? |
|
The major justification I have for wanting to discuss this now is that I can't currently run any unit tests under PHPUnit 6, since that would require me to do the 147 replaces in the test code to use the new PHPUnit namespace, and I don't want to have to deal with the mental juggling of toggling these changes in and out as required. (the clue for what was wrong with my unit tests came from https://stackoverflow.com/questions/29299206/phpunit-no-tests-executed-when-using-configuration-file) |
|
Ignoring all of the above: why are you running PHPUnit 6? Like, what caused it to happen that when you, shadowshade, ran the tests, you were running them under PHPUnit 6 rather than PHPUnit 5? |
|
I'm asking because switching away from phpunit5 would be a pretty bad deal for me. phpunit5 is the default version that apt installs under xenial. This means that instead of hackeration to download phpunit, find a way to run it, etc, which has been proven to break multiple times a year because someone in the php world sneezes on something, i can just say: and that Does The Right Thing for both virtualbox and circleci/docker. In addition, we don't have to upgrade those 147 tests. The fact that the words "out of support" are in some nebulous way attached to phpunit5 has zilch to do with any problems i care about. Upgrading means doing 10 hours of debugging right now to get it running, and having recurring headaches later. If getting phpunit5 installed under AMP is more than a few hours of work for you, let's talk about this. Otherwise, i don't see the benefit, unless i'm missing something. |
|
I've been attempting to run PHPUnit 6 because that's the version that's currently co-shipping with the version of PHP 7 (7.1.13) under my version of MAMP. I'm happy to downgrade my installation of PHPUnit to PHPUnit 5 under the understanding that we're not planning to move up to PHPUnit 6 any time soon. |
|
Yep, exactly --- i don't think we have any reason to be on PHPUnit 6 right now. Give downgrading a shot, and if you have a lot of trouble doing it, complain, and we'll revisit. |
|
I've successfully downgraded the version of PHPUnit used on my Mac to version 5.7.27 (running with PHP 7.1.13). When running on the current master, it comes up with failures and errors. I switched over to the branch cgolubi1/2398_site_upgrade and at least the errors are gone, although I seem to have a huge number of extra errors that you don't seem to have. |
|
Looks like I'm having issues with the random number generation. |
|
That's probably not the random number generator. You're specifically getting the wrong RandomBM types. I would guess something is wrong with your definition of the [Edit: actually, or if you weren't overriding |
|
Huh, interesting. I should be able to look at that tonight. It's quite possible I'm not calling the correct bootstrap file, considering all the changes in infrastructure that I've been going through to get everything up and running. |
|
Good news (I think). Once I started using the bootstrap file again, all my tests pass. |
|
So, I went back and ran the tests on our current master branch, and got errors and failures, as expected: |
|
@cgolubi1, it looks promising that I seem to be replicating the behaviour of PHPUnit that you're seeing. If your replay engine passes, I'm happy for this site upgrade go ahead. We'll still want to ask testers to hit the site, of course. Also, I'm not forgetting about the isset() and empty() audit, I still intend to go through and check all of those at some stage. |
|
Okay, good and bad news on the audit front. I've just had a look at the isset() calls. I count 16 that need to be rewritten in the production code, but hundreds that need to be rewritten in the test code. The reason that the test code isn't showing errors is because the asserts are currently not testing anything, since they're similar to which will always pass, due to the change of behaviour of isset(). I'll base a pull request on the 2398_site_upgrade branch. |
|
The replay site has played 13100 each replayed and novel games with no errors or mismatches. How many is enough? I don't know. That certainly seems like enough to tell us that any logic bugs introduced are pretty subtle. So i'll let it keep going, but i'm inclined to say that logically speaking, the site can play games. We need to figure out what to do about the automated apt package updates:
My inclination is to leave them on, because it gets us patching of some packages with no human time expended, and the downside (a bad update completely toasts the prod site and we have to rebuild it) isn't that bad. Anyone disagree or agree or have any opinion about this at all? |
|
Is it possible to just accept some level of automatic patching, e.g., critical or mature patches? |
|
Automatically notifying about updates (all, or a subset) seems like a fine idea; automatically applying them seems less appealing to me. |
|
Okay, i hear y'all --- i'll figure out how to disable automatic patching. Irilyth: as far as i know, you are the first person to say the words "automatic notification" in this conversation. I don't know how to configure that, and it doesn't sound valuable to me. So i'm not planning to look into it unless someone tells me i'm missing something. |
|
Oh, if you're in favor of automatic updates and want to try it out, we can. I assumed you were opposed and James was in favor. :^) The value of automatic notification is that we'd get e-mail from a cron job or something when there were updates available, and then we could go apply them. If you don't have that, and will just install updates in some other way on some other schedule, that's fine with me. |
|
I think Chaos understands, but I'd be okay with automatic patching of critical fixes. Automatic patching of everything seems like a recipe for disaster. :) |
|
Okay, i pushed a change to disable automated updates. |
|
I'm very tempted to try to get this upgrade done this week. Here's what i think that would look like:
|
|
@cgolubi1, just one update to this issue. In the initial post of this issue, I listed related issues that might be affected by the site upgrade. I reckon that we might be able to close some of those issues now. Did you feel like doing so? |
The production instance that we are currently running needs updating, for a number of reasons:
Specific issues that may relate to this site upgrade are:
The text was updated successfully, but these errors were encountered: