zrisher commented 7 years ago

Assume we've installed a steam app and are running some servers using it.

If an update for our app is released via steam, we can update the app's files using steamcmd.updateApp and it will succeed even if our servers are running (at least for my app 298740). However, we probably also want to restart any running servers to take advantage of the new update (and before that we may want to warn the online players, etc). To do this, we need to know an update occurred, or if one was available before we updated.

steamcmd.updateApp unfortunately doesn't tell us if an update occurred. I believe we receive:

Success! App 'XXXXXX' fully installed. if it was updated/installed
"Success! App 'XXXXXX' already up to date." if it was already up to date.

So if steamcmd.updateApp read these two conditions differently and returned something to that effect, that could solve this problem.

However, what if we know our app will fail to properly update if there are instances running?

We can use steamcmd.getAppInfo to find the most recently published build and release date for an app. But that means nothing if we can't find what build we currently have installed, which doesn't seem to be available through the steam executable at all.

We could maintain a cache of updated_at data within binDir/steam_node_cache.json. Whenever I update an app, I write Date.now() to cache[app_id][branch][installDir]. Then, when I want to check if an update is available, I can simply compare depots.branches[branch].timeupdated * 1000 from steamcmd.getAppInfo to the cached value. If the remote is newer, I preform the update and notify the running servers to restart.

If either of these two proposals sound reasonable and appropriate, I'm happy to provide a PR.

zrisher commented 7 years ago

I was actually wrong that updateApp treats the above conditions the same - in fact, updateApp returns an error when the app is already up to date. If you wanted to know whether that error was from steamcmd or simply because the app was already up to date, you would need see if the returned error message string contains 'already up to date'.

I think a significantly better resolution is:

app updated - returns true
app up-to-date - returns false
error resolving request - returns the error

I'll submit a PR with that shortly.

zrisher commented 7 years ago

So the first part of this issue can be resolved by #6.

For the second part I was somewhat incorrect in stating

what build we currently have installed, which doesn't seem to be available through the steam executable at all.

SteamCMD does indeed cache the last received app info, and it's available via app_info_print. However, since getAppInfo() forces an automatic update of information (// use app_update to force data to update), we always receive the latest.

I'm not sure it would be wise to rely on this cache to indicate what versions are currently installed anyway. Any time SteamCMD requests the latest info for this app it will be updated, regardless of if we were updating that app, simply requesting its info, or even updating one of many installs.

So I still think a managed cache of installed versions would be useful to solve this problem. Planning on opening a PR for this in a few days unless anyone disagrees.

mathphreak commented 7 years ago

(Sorry about the month and a half delay.)

On one hand, keeping a steam_node_cache.json file would be more implicit state than best practices would recommend; on the other hand, since this module wraps an entire third-party executable with fairly little control over it, I think best practices are a bit moot.

If you think node-steamcmd should cache installed versions, I'm OK with that. I don't necessarily feel like writing the code for it, but I'd merge a PR (and probably in less than a month this time).

zrisher commented 7 years ago

@mathphreak Small update here:

The version info we need is available in appInfo.depots.branches, which is available when the commands are run interactively, but is unfortunately truncated from the stdout of when run with command line args. See this issue for a full description. Doesn't look like this is going away any time soon.

I'm thinking perhaps I can try running the program interactively via a child process and pass in stdin after waiting between commands. Or I could parse the binary VDF data from binDir/appcache/appinfo.vdf. The former sounds fraught with edge cases, and the latter looks minimally supported, but we'll see what I can do. More to follow.

zrisher commented 7 years ago

My findings so far:

Steamcmd runs some commands asynchronously, so their output can merge with others in stdout, e.g.

"734"
{
"name"      "Counter-Strike: Global Offensive BERROR! Failed to install app '4' (Invalid platform)
eta Linux Bin"
"config"

Steamcmd does weird things with its output buffering and stdout direction that translate to the final chunk of text being dropped in the output of run
There are no well-supported binary vdf readers available, and the steam web API's method for checking versions doesn't return any useful data. So we have to fix 1 and 2.

To fix 1, I'd like to use app_info_update 1 instead of force_install_dir ./4, app_update 4 to force an update request because it has no stdout attached. I don't know if this is as effective; I see others using it but unfortunately it's difficult to test.

To fix 2, we just need to make our output long enough that the appInfo is contained within what's returned. I decided to use find e instead, which has long and predictable output.

Should have an initial PR out soon.

zrisher commented 7 years ago

10 implements the hardest part of this. Off of it, #11 implements getting the remote version.

Once those are merged I'll submit a PR for getting the local version from installDir/steamapps/appmanifest_appid.vcf.

mathphreak commented 7 years ago

I've been wondering if it would make sense to restructure run() to wrap a pseudo-TTY and run steamcmd interactively within that pseudo-TTY. That would probably eliminate the async output issue, it might fix some of the other weird things, and I'm fairly sure it would allow for a smarter HLDS workaround as well. The downside is I have no idea how to do that.

Also, would you be interested in taking over ownership of this library?

zrisher commented 7 years ago

I've been wondering if it would make sense to restructure run() to wrap a pseudo-TTY and run steamcmd interactively within that pseudo-TTY

Already played around with it and got it working! 😄 I just opted for the less invasive solution. Happy to take a crack at that, but I'd love to finish my version work first so I can advance with the project that depends on this library. Created https://github.com/mathphreak/node-steamcmd/issues/13 for it.

Also, would you be interested in taking over ownership of this library?

Like all programmers probably, I don't have nearly as much time as I'd like, so I'd definitely be keen to see someone who has plenty of free time at the helm. I've also been completely happy with your maintenance speed. If you'd prefer to hand it over to someone else though, and no one is raising their hand with the technical competency and needed free time, then yes I would be glad to.

zrisher commented 7 years ago

FYI getting the locally installed version is done in https://github.com/zrisher/node-steamcmd/commit/63ae26e35ed70c7891e40a176cc96b7d61fb1943 - steam caches it for us in appmanifest_appId.acf. I'll PR it once we clear up #12 and #13.

mathphreak / node-steamcmd

Check if update needed/installed #4

10 implements the hardest part of this. Off of it, #11 implements getting the remote version.