platformsh / legacy-cli

This is the legacy version of Platform.sh's command-line interface. The new version is at: https://github.com/platformsh/cli
https://docs.platform.sh/administration/cli.html
MIT License
221 stars 120 forks source link

CLI tool does not update when using Windows #1147

Open mattc-cogapp opened 2 years ago

mattc-cogapp commented 2 years ago

We run automated scripts on a Windows machine to populate a database and then switch environment variables to switch between them (a sort of cursory green/blue style workflow). Unfortunately, the build scripts have to be run on Windows due to a particular tool we use that does not run very well at the moment on Linux machines.

As part of these scripts, we run the Platform.sh CLI commands to update environment variables. This was working well for a while, but now when we get prompted to update the CLI if a new verison is available, Windows reports the CLI as having been updated - but this is not actually the case.

When running in verbose mode, we can see that there's an access denied error:

C:\Users\cogapp>platform self:update -vvv
...
Update to version 3.81.0? [Y/n] y
Updating to version 3.81.0
Deprecated: version_compare(): Passing null to parameter [#1](https://support.platform.sh/hc/requests/1) ($version1) of type string is deprecated in phar://C:/Users/cogapp/.platformsh/bin/platform/vendor/padraic/phar-updater/src/VersionParser.php on line 165
Warning: rename(C:\Users\cogapp\.platformsh\bin/platform.phar.temp,C:\Users\cogapp\.platformsh\bin\platform): Access is denied (code: 5) in phar://C:/Users/cogapp/.platformsh/bin/platform/vendor/padraic/phar-updater/src/Updater.php on line 382
The Platform.sh CLI has been successfully updated to version 3.81.0

C:\Users\cogapp>platform --version
Platform.sh CLI 3.80.3

However the message afterwards is misleading, as the CLI tool has not actually been updated.

I suppose this boils down to two things:

1) Should this work under Windows? If so - then the above is a legitimate error (at least for us). 2) Should the error message say something else in the event that an error occurred during an update?

We're currently running PHP 8.1.7 on an AWS Windows machine (I believe it's one of the old Enterprise LTS Windows versions - 20H2 or something like that)

benr-cogapp commented 2 years ago

Actually I'd suggest four things. If in fact this should (1) run under Windows then:

(2) The error on line 382 is reported (in -vvv verbose mode) as a warning (and therefore nor normally shown to the user, and not treated as a failure to update); instead it should be handled as an error, and the update process should report the error, instead of reporting that the CLI has been succesfully updated.

(3) This error shouldn't occur! The observed behaviour is that after running the update command .platformsh\bin contains two files, platform.phar.tempand platform. Swapping these does indeed render the CLI updated. I don't know whether the code attempts to rename the current platform out of the way, before issuing the rename command to put the new version in place; and that after failing to do so, reverts the previous rename. Or whether it attempts to use rename to overwrite the previous file. Some possibilities:

(4) A final issue is that although we invoke the command platform self:update --yes --timeout=120 at the start of the daily script, in an attempt to ward off the problem by avoiding a check for two hours, it has no apparent effect. If there is an update, the update process of course fails; and the next invocation of platform ..., just a minute later, produces another request to consider updating.

pjcdawkins commented 2 years ago

Thank you both for the detailed descriptions.

I agree it looks like the key is that the failure to rename is not properly handled, and that needs to be fixed.

But... I wouldn't expect to run self:update in a script. Are you doing that to avoid the update check? I would advise instead setting the environment variable PLATFORMSH_CLI_NO_INTERACTION to 1, e.g. (bash/POSIX):

export PLATFORMSH_CLI_NO_INTERACTION=1

which removes the chance of any blocking questions, and also disables update checking.

PS. The mix of back- and forward-slashes is ugly but it should be handled fine by PHP.

benr-cogapp commented 2 years ago

Hi Patrick,

Thanks for your response.

The process is using platform variable:get to get the value of an environment variable; and the problem is that when there's an update to the CLI available, it interprets the information about the update as the value of the variable.

(I'm almost certain that we experimented with using --yes to avoid this, without success; I know we did on some other commands; but I can't now be sure whether we tried it on variable:get. Of course, we can only experiment in this area when there is an update available and it hasn't yet been applied!)

Hence, we tried having the batch script that kicks off the process first do a self:update to avoid this problem.

However, we'll now try PLATFORMSH_CLI_NO_INTERACTION which I wasn't aware of - thanks for the tip!

benr-cogapp commented 2 years ago

Meanwhile, please let me know if there's some way we can help you debug the underlying issue with updating the CLI.

benr-cogapp commented 2 years ago

Hi Patrick - now that there's been an update allowing us to test it - I can confirm that using PLATFORMSH_CLI_NO_INTERACTION has solved our main problem. Thanks very much for this,

It remains the case (as of 3.80.0 -> 3.81.1) that the updater fails on Windows.

pjcdawkins commented 2 years ago

Thanks for the update. There are 2 things remaining: debugging the permissions issue (to fix Windows update) and also patching or replacing the now-abandoned updater library (to fix the error message).

So maybe let's focus on this to start with

Warning: rename(C:\Users\username\.platformsh\bin/platform.phar.temp,C:\Users\username\.platformsh\bin\platform): Access is denied (code: 5) in phar://C:/Users/username/.platformsh/bin/platform/vendor/padraic/phar-updater/src/Updater.php on line 382

It might be related to this note in the PHP docs:

Note: On Windows, if to already exists, it must be writable. Otherwise rename() fails and issues E_WARNING.

Is the existing platform (phar file) itself writable?

benr-cogapp commented 2 years ago

Hi Patrick

is the existing platform (phar file) itself writable?

Yes it is.