search

jamulussoftware / jamulus

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

Check For Updates #370

Closed corrados closed 4 years ago

corrados commented 4 years ago

Copy from https://sourceforge.net/p/llcon/discussion/developerforum/thread/506419bd89

Please add a Check For Updates function (menu item) to the Client and Server (GUI). It could be done in phases:

1) Query Github to see if there is a newer version than what is running, and informs the user, perhaps with a hyperlink to the newest version. 2) If there is a newer version, offer to download it. 3) After downloading, automatically install. 4) Provide an "always update on startup" option.

pljones commented 4 years ago

I've had something similar in a few applications I've written.

The version number would be compared with the running application version number and, where newer, the user offered the option to go to the URL, also displaying the description text.

I actually implemented it twice, once in C# and once in Scala.

The Scala version is here: https://github.com/pljones/jTrapKATEditor/blob/master/src/main/scala/info/drealm/scala/updateTool/Checker.scala

The slightly more cumbersome-looking C# one is in several places, of which this is one: https://sourceforge.net/p/sims3tools/git/ci/master/tree/s3pe/Settings/AutoUpdate.cs

corrados commented 4 years ago

The Jamulus software contacts some server and queries some information. Then it maybe downloads a file and executes it. I can see multiple ways for a hacker to use this to modify informations and inject unwanted code. Before anything is implemented, we should discuss possible security issues here: https://github.com/corrados/jamulus/issues/314

WolfganP commented 4 years ago

It's interesting to have the program check for updates. Probably a good start will be to just check and notify, and let the user take action (ie just implement your 1st step aboce)

pljones commented 4 years ago

I don't like "automatically run" but a lot of users seem to want this from software to make their life a one click solution...

gilgongo commented 4 years ago

It's speculation of course, but an update notification might prompt some server operators to upgrade to make use of genre-based Central Servers. This might further reduce the numbers on the defaults as there are a lot of pre-3.5.4 servers running. But it's perhaps a minor point :-)

pljones commented 4 years ago

They'd have to upgrade to get support for the notification.

gilgongo commented 4 years ago

Initially, yes (hence my "minor point"). I was thinking further ahead if update notifications mean that server operators don't simply stick with the version they first downloaded, as currently seems to be the case for many. Out of sight, out of mind, as it were.

pljones commented 4 years ago

If they were running the server as a service, even if it notified once a day (rather than only on start up), they operator would still need to check the logs. Most of the time the server really is out of mind, as it just works.

To get any attention - and it wouldn't be a very nice approach - you could update the server welcome message to indicate it's out of date. Everyone entering the server would then see it. Hopefully the server operator would eventually spot it...

gilgongo commented 4 years ago

Yes, not easy. I guess perhaps the only realistic route to encouraging upgrading to a given version is if a change (or security issue?) was introduced to prevent old servers from connecting to new central servers perhaps.

But I guess there isn't much reason to encourage people to upgrade servers really. It might benefit other server operators by freeing up more slots on the two former geo-based central servers perhaps, and of course help clients who wanted -F.

gvermag22 commented 4 years ago

It's somewhat unfair that JamulusOS has an update Jamulus ability, but Mac and windows don't. Moreover, one must go through multiple steps to update Jamulus on Mac, which is not such a good experience. We realize that Jamulus is evolving, and it would be better for users to know the latest features by trying them out in an easier manner. There should also be an easy/intuitive way for people to review the release notes for a specific release version. e.g. I have no idea what are the new features/enhancement in version 3.5.8

pljones commented 4 years ago

It's somewhat unfair that JamulusOS has an update Jamulus ability, but Mac and windows don't.

Someone put the effort in to make it happen. It's not a matter of "fairness". The Linux build itself doesn't, it's just like the Windows one and the MacOS one.

JamulusOS is an entire Linux Distribution, like CentOS or Ubuntu, not just the Jamulus software. So, just like Windows will update itself, JamulusOS (the operating system) will update itself.

So really, a "like for like" complaint would be "There's no version of Windows with Jamulus built in and maintained by the Microsoft OS maintenance team" or the equivalent for MacOS.

gvermag22 commented 4 years ago

JamulusOS was an excellent idea, and we are thankful for the great development work.

On Sat, Jul 4, 2020 at 10:00 AM Peter L Jones notifications@github.com wrote:

It's somewhat unfair that JamulusOS has an update Jamulus ability, but Mac and windows don't.

Someone put the effort in to make it happen. It's not a matter of "fairness". The Linux build itself doesn't. JamulusOS is an entire Linux Distribution, like CentOS or Ubuntu, not just the Jamulus software. So, just like Windows will update itself, JamulusOS (the operating system) will update itself.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/corrados/jamulus/issues/370#issuecomment-653788988, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQCIRBPSKUWF4MUSCL75PETRZ5N2PANCNFSM4OAZVQKA .

pljones commented 4 years ago

Actually, I take part of that back.

I'd have been better saying "Why don't Microsoft and Apple make a standard distribution mechanism for add on apps?" And I guess they do - they both have app stores. So, just like many of the Open Source Linux Distributions have people contributing Jamulus builds to them, enabling users of those distributions to get automatic updates, it may be possible for someone to publish builds to Microsoft and Apple stores. Never having used either store, I wouldn't know.

gilgongo commented 4 years ago

I would guess they'd need to pay for the privilege :-)

gilgongo commented 4 years ago

BTW @pljones

Publishing the new application release created a file "latest.txt" or similar in a known location

I see that https://api.github.com/repos/corrados/jamulus/releases/latest does this (as long as Volker remembers to hit the "publish" button on the release)

pljones commented 4 years ago

Which lets the user check the version nicely, yes. I'd still suggest sending them to https://sf.net/projects/llcon for the download as that response doesn't indicate a platform-specific installer, which is what most users would want.

Although I wonder if "assets": [], could be filled in with the details (even if hosted on sf.net).

gilgongo commented 4 years ago

Yes the FS download link is the mot efficient, supplemented by the link to the changelog perhaps (although that might confuse some people as usually has the git head release in it).

I wonder if servers could then have a command line option to provide an email address for update notifications?

pljones commented 4 years ago

I wonder if servers could then have a command line option to provide an email address for update notifications?

Like I've been muttering everywhere, everything that's available through the UI should be available through command line and vice versa. So yes, some way of getting notifications for updates if you're running without the UI would be good.

gvermag22 commented 4 years ago

I was hoping there could be a check for updates menu item that would easily allow us to upgrade the server and client versions for Mac or windows. Other programs do that.

On Sun, Jul 5, 2020 at 1:46 AM Jonathan notifications@github.com wrote:

BTW @pljones https://github.com/pljones

Publishing the new application release created a file "latest.txt" or similar in a known location

I see that https://api.github.com/repos/corrados/jamulus/releases/latest does this (as long as Volker remembers to hit the "publish" button on the release)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/corrados/jamulus/issues/370#issuecomment-653859296, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQCIRBPNFP3RFOBPEVU3J73R2A4UTANCNFSM4OAZVQKA .

pljones commented 4 years ago

@gvermag22, that's what we're discussing here.

pljones commented 4 years ago

@gilgongo,

The problem with sending an email is ... you need to become an email client and know how to contact a server. A Raspberry Pi may be set up for Jamulus and nothing else, so even if you open port 23 and dump the email in, it doesn't mean it's going to (a) work (maybe nothing is listening) or (b) get delivered (maybe SMTP service is running unconfigured). And I've no idea about, say, a Windows Cloud VM server...

And at-me... what I was saying earlier about the Welcome message -- I've no idea what I was on about...

So we have:

Client (assume UI showing on all platforms):

Server - UI showing

Server - built headless or running without the UI

pljones commented 4 years ago

I'm thinking I might give the client end of this a go, next.

gvermag22 commented 4 years ago

THank you, that would be great!

On Tue, Aug 18, 2020 at 4:35 AM Peter L Jones notifications@github.com wrote:

I'm thinking I might give the client end of this a go, next.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/corrados/jamulus/issues/370#issuecomment-675426092, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQCIRBOCQI2GVQ3DUDNVJILSBJRRTANCNFSM4OAZVQKA .

pljones commented 4 years ago

554 is the first commit for this.

There's possibly a few more things that could be done - specifically for the server to have a timed checker so it polls once a day. As it is, the checker only runs on start up or request from the UI.

corrados commented 4 years ago

Thanks for your code. Unfortunately, I have not read your specification prior to your coding correctly.

Server - built headless or running without the UI

  • Details of the current version, the latest version plus the relevant parts of the ChangeLog are written to "standard out"
  • This should end up in the "system log"

I actually do not want this functionality for headless mode at all. GUI only support is what I want. You have changed that the headless server mode now always uses the settings file as given in the home directory. I actually do not want this. For the headless server mode I want so specify the behaviour on the command line. So everything works as expected and no unknowns on startup. So if you want to use an ini-file in headless server mode, you have to specify it with --inifile. That is predictable behaviour.

As in your specification, the version check output goes on standard out and system log. I do not think that Jamulus server operators expect the headless Jamulus making version checks at all. I do not think they will check the standard out or system log. I think the version check should be supported in the Jamulus client and server GUI. When the GUI is started, the check is performed. That is enough.

Sorry for starting the discussion on that a bit too late.

gilgongo commented 4 years ago

So if you want to use an ini-file in headless server mode, you have to specify it with --inifile.

Off topic question, but I assume the current behaviour is that the client will create and always use an inifile, while the server will not unless you use --inifile in which case (some?) things on the command line at startup will be overridden? The feels like it needs some more documentation if so.

(For what it's worth - I agree that most server operators probably won't check logs and std out. I know I don't! This is a pity as it would be nice to have all those out of date servers upgraded. Would it help if the server version was displayed in the client together with an "upgrade available" message perhaps? Not sure if that was discussed.)

corrados commented 4 years ago

while the server will not unless you use --inifile

No, if the server is run in GUI mode, it will use the default ini file automatically like the client does. Just the headless server which is started on the command line will not use it per default but only if you explicitely specify it with --inifile.

gilgongo commented 4 years ago

Just the headless server which is started on the command line will not use it per default but only if you explicitely specify it with --inifile.

OK I might add something in the docs about that as people running servers (GUI or headless) might be curious. All we say about --inifile today is that it sets the initialization file name.

Sorry for hijacking the thread.

pljones commented 4 years ago

So --inifile should be an error if --nogui is specified or it's built HEADLESS? At the moment it's inconsistent, which is why I made the change. Now it works consistently (just the opposite way...).

Where should Jamulus store persistently the latest check time if there's no inifile?

corrados commented 4 years ago

So --inifile should be an error if --nogui is specified or it's built HEADLESS?

No, with --inifile you can explicitely specify an inifile you want to use for the headless server. The headless server just does not use the Jamulus default inifile which is stored in the home directory.

corrados commented 4 years ago

Where should Jamulus store persistently the latest check time if there's no inifile?

The headless client/server should not have an update check in my opinion (see my comment above https://github.com/corrados/jamulus/issues/370#issuecomment-683246488).

corrados commented 4 years ago

At the moment it's inconsistent, which is why I made the change.

Yes, it is inconsistent but on purpose.

pljones commented 4 years ago

There are two main issues:

I have less impression of the latter - mostly on support forums people get told "get the latest version" and most people running the client are likely to get this if they run into a problem. Hence I think the former is at least as important.

corrados commented 4 years ago

Many servers getting out of date

Yes, that's right. But the message in standard out or system log will not help. If people setup a headless server on a remote virtual machine, I do not think they are checking the system log files. So you will not make them update their server by this.

A better way of getting rid of old servers is to exclude them from the server list.

corrados commented 4 years ago

I think the most important functionality is that the client with GUI reports that a new version is available so that the Jamulus users can update to get the latest new features. This is what was mostly reported in the forums.

pljones commented 4 years ago

Just to be clear for ini file vs gui vs command line setting.

That seems non-intuitive, confusing, possibly annoying and at the least inconsistent. Either the command line settings should always get written or never get written. (I suggested never get written on my last pull request. I think it's better: if you're testing via "here's my preset", "here's my temporary override", even with the GUI, it prevents mistakes.)

Particularly writing out to the default inifile could cause people not quite sure what they're doing to get very frustrated.

pljones commented 4 years ago

I take your point on the headless solution, really.

69, Server Status, would be a better place to cover it and whether it would involve the Jamulus server doing any more than reporting its version is a different matter (i.e. it wouldn't need to go collect even the latest version details).

That would drop the write back for headless/nogui (and a bit of if/else coding).

corrados commented 4 years ago

That seems non-intuitive, confusing, possibly annoying and at the least inconsistent. Either the command line settings should always get written or never get written. (I suggested never get written on my last pull request. I think it's better: if you're testing via "here's my preset", "here's my temporary override", even with the GUI, it prevents mistakes.)

I don't have these concerns. Running the Jamulus server in headless mode is usually a complete different usecase. E.g. the server operators are:

pljones commented 4 years ago

Experienced Linux users running the Jamulus server only headless ... I configure the Jamulus server with command line arguments only. I would not expect that any settings are stored in some hidden in file. This would confuse me very much actually.

That's not necessarily the case. Very many servers have a configuration file that's read and then overridden by command line values.

Experienced Windows users which use command line arguments ... They expect their command line arguments overwrite the settings of the hidden inifile (which is the case).

That's what I continue to dispute. It seems counter-intuitive. You end up with non-repeatable behaviour. I'd expect it to work like a headless case - inifile servers as the basis, command line modifies behaviour but doesn't update inifile. (I'd almost want to lock values for which command line options have been given. This way you could provide someone with a shortcut file with specified options with some degree of knowing it should work and keep working - e.g. teachers setting up a classroom might want per-desk overides to a file server shared inifile.)

corrados commented 4 years ago

We should not mix things here. Let's focus on the update check in this Issue. I have created a new one for discussing the interaction between command line arguments and ini file: https://github.com/corrados/jamulus/issues/555

So back to the topic: I would remove the update check for any headless Jamulus operation. I would not introduce new command line arguments for the update check. And please change the settings behaviour. We should not mix things. If we want to change this, we should do it in a separate pull request. I think if you remove the headless update check you do not need the inifile so the change with the settings is not necessary anymore. Do you agree?

Let's do things step-by-step with as little changes per change as possible :-).

pljones commented 4 years ago

Sure - I tried to keep the commits separate. I'll revert to master and reapply without the headless code.

Just one last point on CSettings to clear up here: I'd like it to be a QObject / Q_OBJECT and own the life-cycle of the settings, anyway. It means it's only in one place. The rules about "only do this if" are then at the point the OnAboutToClose() signal gets handled -- or not -- and can be clearly documented.

Also the move of default central server just seemed to make sense -- I can split these two into a separate pull request, of course.

corrados commented 4 years ago

I have not fully understand this CSettings improvement, actually. Maybe it will be clearer if I see the changed code.

I can split these two into a separate pull request, of course.

Yes, that would make things easier. The smaller the increment is, the easier the review/testing/integration work will be.

pljones commented 4 years ago

The current pull request has five separate commits: https://github.com/corrados/jamulus/pull/554/commits

Excluding the main one for the update checker, these are...

I updated --help:

I moved the handling of appliction shutdown to CSettings (note, this will change with the headless handling to only happen as it does now - but I'm asking for it to stay here, even though it makes CSettings a QObject):

I moved defaulting of centralserver -- it seems to fit okay in CSettings and feels more consistent with the other "do we need to default" code that's also in there. Like the help change, it can be done as a separate pull request.

Actually, looking at this one, I don't think I did what I intended!! The main aim was bringing client and server flows into line as the update tool is identical for the two and the flow here differed.

With the headless mode reverted from that, the life-cycle change to CSettings and translations change could both be removed.

The life cycle as it is just feels wrong - either there should be a dedicated data only CSettings object and a dedicated settings application life-cycle QObject to handle QObject-level interaction, or that life cycle handling should be in CSettings itself.

Currently, it's split across multiple unrelated classes, so it means there are separate places to maintain when something does change.

pljones commented 4 years ago

Back on topic (for the moment :) )...

I noticed when testing, the Linux code seems happy accessing https://api.github.com/repos/corrados/jamulus/releases/latest but the Windows Qt call fails - it doesn't have HTTPS support built in. Github API redirects any non-HTTPS to the HTTPS endpoint.

So, OpenSSL is required - and that makes life a bit harder on Windows where it's likely not "just there". https://doc.qt.io/qt-5/windows-requirements.html#ssl

I guess, subject to meeting the license condition (i.e. reproduce the license in the docs), the Windows installer could ship it and the Linux distributions be made dependent upon it (and hence it's either already there or gets installed). Anyone installing by hand would need the dependency documented (transitive through Qt or not).

https://wiki.openssl.org/index.php/Binaries -> licence: https://www.openssl.org/source/license-openssl-ssleay.txt -> https://github.com/openssl/openssl/releases/tag/OpenSSL_1_1_1g latest stable at time of posting

Without this, it's a bit of a dead end...

corrados commented 4 years ago

Alternative approach: The Jamulus Central server will always have the newest version. The client/server GUI can simply send a PROTMESSID_CLM_REQ_VERSION_AND_OS message on startup and check if the Central server version is newer than the own version.

storeilly commented 4 years ago

I love your elegant simple solutions, why over complicate something when the solution can be so much simpler!

On Sat 29 Aug 2020, 18:51 Volker Fischer, notifications@github.com wrote:

Alternative approach: The Jamulus Central server will always have the newest version. The client/server GUI can simply send a PROTMESSID_CLM_REQ_VERSION_AND_OS message on startup and check if the Central server version is newer than the own version.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/corrados/jamulus/issues/370#issuecomment-683322215, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIJSQOVKZI44YQDNSAVNATSDE52HANCNFSM4OAZVQKA .

gilgongo commented 4 years ago

Could the client/server UI then show a message something like this if there was a newer version (where "upgrade available" is a link to the SF download link)?

image

corrados commented 4 years ago

That's a great idea. That can be implemented in no time with only a few changes in the Jamulus code. I really like this solution since the Jamulus user instantly sees the message and we do not need to store anywhere when we have shown the dialog since there is no dialog.

corrados commented 4 years ago

I'll start doing some tests how much effort it would be to implement such a simple solution. I hope pljones is ok with it (since he has spent some time for implementing the original specification already). But as you know, I am a big fan of simple but effective solutions ;-).

corrados commented 4 years ago

I just coded a few lines and it works like a charm.