jamulussoftware / jamulus

Jamulus enables musicians to perform real-time jam sessions over the internet.
https://jamulus.io
Other
997 stars 222 forks source link

Make version check more reliable #1120

Closed hoffie closed 3 years ago

hoffie commented 3 years ago

Is your feature request related to a problem? Please describe.

The current version check, which triggers the "update available" message in the UI, has some shortcomings:

  1. It relies on a single server (anygenre1). Should this ever be down, update checks will stop working.
  2. Currently it's rather naive and it would false show an update when the reference server would be running a beta release (i.e. 3.7.0beta1 would be considered newer than 3.6.2)

Describe the solution you'd like I'm suggesting all of the following:

  1. Decouple the update check role from the dictionary server role. In practice, this could still be the same server, but would gain us some flexibility if we ever were to move. To be specific: use updatecheck1.jamulus.io in the code and make this a CNAME for anygenre1.jamulus.io for now.
  2. Use a second server to remove the single point of failure. updatecheck2.jamulus.io should be set up as a CNAME for another (maybe already existing) server in another network.
  3. Also check the version of the server which the user connected to. This would keep the update check working if we ever had some jamulus.io-related issue. It would also keep it working in disconnected, private networks.
  4. Make the update check ignore "special" versions such as betas. The code uses QVersionNumber which supports detecting suffixes such as "beta" or "dev". The only practical issue I see is that the currently running update reference server by Volkre is using a dev version (3.6.2dev-c1bc4274), so blindly doing this would break it. Either we whitelist dev versions or we add a special command line option for the update reference server which could be used to override what gets sent as the version in the protocol.

Describe alternatives you've considered

Additional context

https://github.com/jamulussoftware/jamulus/issues/1079#issuecomment-782761723

Opinions? Is this something which should partially (1 & 2 maybe) go into 3.7.0?

pljones commented 3 years ago

3 is good - the client always (at least after a certain version) gets the version of the server, for compatibility checks. If it gets a newer one, that's an obvious point to say "Hey! You're not up to date."

4 - The version taken into account should be the /[1-9]+\.[0-9]+\.[0-9]+/ part - nothing beyond should affect the client. It shouldn't care if something is dev or beta -- it's only interested in the latest release version number, which comes before (now).

1 and 2 are nice and easy to do. I'd suggest we think about putting them into 3.7.0, indeed.

I'll repeat this:

Whatever happens, we MUST remember, we can't change the clients out there - they need to keep working with whatever approach is taken.

hoffie commented 3 years ago

4 - The version taken into account should be the /[1-9]+.[0-9]+.[0-9]+/ part - nothing beyond should affect the client. It shouldn't care if something is dev or beta -- it's only interested in the latest release version number, which comes before (now).

That would be what QVersionNumber gives us. This works for final releases and for custom builds from git (i.e. 3.6.2dev-123456). The latter works because git builds still use the previous version number. In contrast to that, beta builds will use the upcoming version number. Running a server with 3.7.0beta1 and stripping of the suffix in the comparison, will lead a 3.6.2 client to falsely trigger the update notification. I can come up with the following options:

1 and 2 are nice and easy to do. I'd suggest we think about putting them into 3.7.0, indeed.

I can do that. Any other opinions on that from @gilgongo @softins @ann0see? Would anything prevent us from setting up those two new DNS aliases?

Thinking some more about it: We would still have to hardcode the port, so it would not gain us total flexibility, but I guess it's not needed either. If we were to move one of the updatecheck endpoints at some time, we would probably move it to infrastructure controlled by us and so we should be free to chose the port in this case.

pljones commented 3 years ago

The reason it's 3.6.2dev even though 3.6.2 has been released and it's later than 3.6.2, is exactly that. Arguably, we should have the beta being r3_6_2betaX and only switch to r3_7_0.* for the release and later.

pljones commented 3 years ago

Thinking some more about it: We would still have to hardcode the port, so it would not gain us total flexibility, but I guess it's not needed either. If we were to move one of the updatecheck endpoints at some time, we would probably move it to infrastructure controlled by us and so we should be free to chose the port in this case.

We could require that the server hosting one of the update hostnames maintain port 22124 available for that purpose.

hoffie commented 3 years ago

We could require that the server hosting one of the update hostnames maintain port 22124 available for that purpose.

Yep.

The reason it's 3.6.2dev even though 3.6.2 has been released and it's later than 3.6.2, is exactly that. Arguably, we should have the beta being r3_6_2betaX and only switch to r3_7_0.* for the release and later.

Calling the beta for 3.7.0 3.6.2beta1 would be really counter-intuitive for me. I agree that it would be a workaround for the technical issue, but I think it's highly confusing as this is not what people know from other software. We could think of adopting or at least make our logic similar to a common standard such as Semver. This would basically be:

Example: 1.0.0-alpha < 1.0.0-alpha.1 < 1.0.0-alpha.beta < 1.0.0-beta < 1.0.0-beta.2 < 1.0.0-beta.11 < 1.0.0-rc.1 < 1.0.0.

softins commented 3 years ago

On my first read through the thread (not seen it till now), you seem to have covered all the things that I would have mentioned. I’ll look through again and comment if anythng comes to mind.

But I agree that saying 3.6.2betaN for what is actually the betaN of 3.7.0 is counter-intuitive. But I have no problem with keeping beta builds of 3.7.0 still with a version string of 3.6.2dev-nnnnnnn, and just using Github tags to denote their beta status. It maybe depends on how widely we want beta releases tried out in the field, or whether they are just more for our testing, and anyone who happens to find them.

pljones commented 3 years ago

I think it's highly confusing as this is not what people know from other software

That's a developer's view point. Most of the people seeing the version number are not. The moment they see a higher version than that of the client they're running, they start trying to find it.

That's why the version is currently done the way it is. It was changed last year - previously it had been the "developer-oriented" view.

hoffie commented 3 years ago

But I have no problem with keeping beta builds of 3.7.0 still with a version string of 3.6.2dev-nnnnnnn, and just using Github tags to denote their beta status.

I could live with that as well and I think this is the simplest thing to do. This would make the initial spec from #1079 obsolete, though.

That's a developer's view point. Most of the people seeing the version number are not. The moment they see a higher version than that of the client they're running, they start trying to find it.

I still think that even mainstream software uses the approach where a alpha, beta or preview comes before the identically named version without any beta suffix. Common examples: Microsoft Office 2010 Preview < Microsoft Office 2010 (final), macOS 11.3 beta 2 < macOS 11.3.

hoffie commented 3 years ago

My impression is that we've agreed on doing 1+2 (set up updatecheck1/2 aliases and use them) and trying to get this into 3.7.0 (that's why I added the milestone). @softins volunteered to make that happen, as far as I understood.

The other points still need deciding, but are not urgent in any way. It might make sense to split parts of this issue into separate issues and closing this one so that we don't lose track? I can do that if there is agreement.

softins commented 3 years ago

1, 2 and 4 were easy. Given the redundancy, I'm not sure whether 3 is really useful.

hoffie commented 3 years ago

1, 2 and 4 were easy.

Cool, thanks! Will look at the PR later.

Given the redundancy, I'm not sure whether 3 is really useful.

It's useful in disconnected environments or if we ever had problems with the domain name.