jdel / sspks

A very simple Synology Package Server, reverse engineered from the official Synology package repository and SynoCommunity.
GNU General Public License v3.0
258 stars 64 forks source link

Deactivate qinst or check for WIZARD_UIFILES dir #38

Closed DKeppi closed 6 years ago

DKeppi commented 7 years ago

Hi jdel,

sorry for another question.

I have a package where i need the wizard during installation to fill out the mysql root password, but in lib/SSpkS/Output/JsonOutput.php the standard for the server is "qinst = true"

Where i could change that to false via Docker image?

THX & BR DKeppi

mbirth commented 7 years ago

PING @benrubson

Your package could overwrite the default by setting qinst=false, I believe.

However, I think we should set the default back to false and let packages which are definitely quick-inst-capable supply a qinst=true field in their INFO.

benrubson commented 7 years ago

Yes, you can set qinst=false in your INFOfile. SSpkS will extract it and use this specific value.

Regarding the default setting, perhaps we could leave it to true, and if there is a WIZARD_UIFILES directory in the package, set it to false if this option is not already in the INFO file (to avoid changing a hard-defined user choice).

What do you think about this ?

benrubson commented 7 years ago

Here is why we set it to true as default : https://github.com/jdel/sspks/pull/29

mbirth commented 7 years ago

Yes, I can make it check for a WIZARD_UIFILES dir. Sounds like the best way to go.

DKeppi commented 7 years ago

Sounds nice, thank you - i will give it a try with qinst="false" in the INFO

mbirth commented 7 years ago

@benrubson Do you happen to know if there's a mapping between fields in the INFO file and what's returned via JSON? E.g. (INFO <=> JSON)

Or what's the difference between silent_* and q*?

Because this wiki doesn't say anything about qinst or similar being allowed in the INFO. And this code seems to just translate the silent_* keys to q*.

EDIT: Never mind, found the dev guide. So: q* are no official tags for the INFO file.

benrubson commented 7 years ago

silent_* and q* fields are different things and must not be mapped :

So I think they are clearly different things. q* may be "hidden" fields allowed only in the JSON from the server, which are set depending on the other documented fields.

So me may set qinst to true by default, to false if there is a WIZARD_UIFILES dir, without checking for a user value in the INFO file, as it is not a user field. For sure qupgrade does the same thing but for upgrade process :)

When qinst is true, if qstart is true, package will automatically start. If false, will not. This is something different from startable which disallows the user himself from starting/stopping the package. I don't see how we could map qstart to something (a documented field, a file or directory...). Is there any reason not to set qstart to true when qinst is true ?

So I would say set them all to true by default, to false if there is a WIZARD_UIFILES dir.

MORD0R commented 7 years ago

I cant get it to work. http://nzbusenet.com/Spotweb_noarch-all_20170305-1.spk

Could you test it?

Doesnt seems to work qinst="false" install_dep_services="db"

benrubson commented 7 years ago

See #43. You should get a message telling that you must first enable the database service. Or if you use install_dep_packages, you should get a message telling that you must install some packages before.

MORD0R commented 7 years ago

I have mariadb installed. image

And i still get the mesagge that i need to install mysql.

I think the script is not reading the nfo file.

image

mbirth commented 7 years ago

So, after thinking about it, I believe it is like this:

silent_* is a way for a package to specify that the UI wizard is not needed. I.e. there are sane defaults in place and the settings can be changed later. These fields also decide whether a package can be distributed to other DSMs using the CMS (Central Management System).

q* comes from the package server itself telling the DSM something like "There's no wizard anyways, so quick install/upgrade/uninstall is okay." (The DSM can't know about these things before downloading the package completely.) The DSM can then decide whether to show the generic confirmation dialog or not. Probably also controls what packages are available to be distributed via CMS.

jdel commented 7 years ago

If I understood correctly, we could just map silent_install to qinst and silent_upgrade to qupgrade, and set qstart to true if one of the two above is set to true ?

Opening the spk and checking wether there is a WIZARD_UIFILES every time somebody does a request is going to put some overhead on the server running sspks.

This way, the packge maintainer is responsible for setting these values correctly depending or not if there is a WIZARD_UIFILES in the package.

What do you think ?

mbirth commented 7 years ago

I'm not sure if this will work correctly. The Calendar package e.g. has silent_uninstall="yes", but there's a Wizard UI for uninstall. It's possible that q* skips the wizard completely while silent_* makes it run in background with the default values. I think, I need to check a few more official packages to find a pattern.

However, when scanning the packages for Wizard files, the result could be added to the cached INFO file. So we wouldn't have to rescan the files every time.

jdel commented 7 years ago

OK, it's time to reverse engineer this from the official syno repo then.

Does anyone happen to have the URL or should I get tcp dump running?

benrubson commented 7 years ago

Sorry no I don't have the URL.

Note that "server" q* parameters will only be used during installation, as during upgrade / uninstallation the built-in INFO file will be used (and will not contain q* parameters are they are not documented). So I think we can set them to true by default, to false if there is a WIZARD_UIFILES directory (as we will want to display the wizard. Certainly we should also leave them to true if silent_install=yes.

jdel commented 7 years ago

Ben, would you mind having a look at the files I posted on the Slack channel ? They might help us shed some light on the internal mechanics of these fields.

mesa57 commented 6 years ago

Is there any follow up on this issue ?

jdel commented 6 years ago

I'm afraid not. We never got to the bottom of this.

I can provide the Official synology JSON payload on slack if somebody has time to analyze it.

mesa57 commented 6 years ago

I saw the json output (pretty big). Lost the url however. But I noticed, qinst was in all cases "true". So I agree, the root cause is something else. However, if qinst is set to false in sspks, then it works. I am now trying to get it set in the package info file, but until now this doesn't work.

Other difference with synocommunity and my package server is that i do not serve a certificate. Could that be the root cause ?

mesa57 commented 6 years ago

And please send me a slack invite 😃

jdel commented 6 years ago

@mesa57 Slack invite sent

mesa57 commented 6 years ago

Ok, thx, after our discussion I tried adding qinst="false" to the INFO file in the package. That worked. I noticed however that the change required dsm to be restarted to be in effect.

jdel commented 6 years ago

@mesa57 : Would you be able to test if I implement this ? when WIZARD_UIFILES contains an install_uifile then that file is only displayed on installation when qinst="false"

benrubson commented 6 years ago

According to our discussion above, I would modify the 3 lines here : https://github.com/jdel/sspks/blob/master/lib/SSpkS/Output/JsonOutput.php#L113

like this :

'qinst'        => $this->ifEmpty($pkg, 'qinst', (is_dir('WIZARD_UIFILES') ? false : true)),
'qstart'       => $this->ifEmpty($pkg, 'start', (is_dir('WIZARD_UIFILES') ? false : true)),
'qupgrade'     => $this->ifEmpty($pkg, 'qupgrade', (is_dir('WIZARD_UIFILES') ? false : true)),

Not sure however about the correct complete path to WIZARD_UIFILES directory.

jdel commented 6 years ago

@benrubson Yes that's something along these lines, but i would stick the logic in lib/SSpkS/Package/Package.php.

I will have a look at how to implement this. This will most likely require a new method to extract WIZARD_UIFILES /install_uifile check that it was actually extracted then delete the file. The Phar library currently used doesn't have a method to list all files in the archive for example.

mesa57 commented 6 years ago

Of cause I can test with my local server and package.

jdel commented 6 years ago

Could you give a shot at this branch ? https://github.com/jdel/sspks/tree/fix-qinst

mesa57 commented 6 years ago

I will, tomorrow evening (if the storm won't keep me).

jdel commented 6 years ago

I've pushed a couple more commits to the fix-qinst branch, which should do the trick.

The logic is:

I am not sure this behaviour would suit everyone, but if everyone is happy with it I'm happy to merge it in master.

For reference, here is the JSON returned by the official Synology package center.

And the interesting fields filtered with jq. The fields marked as null here actually mean they are not present in the original JSON from synology as opposed to be set to false as SSPKS does.

I tried to make some sense of the qfields, but it doesn't seem to follow any logic. I can hardly come up with a pattern here.

Docker-GitLab only has a wizard, qstart and silent_upgrade set to true. elephantdrive doesn't have a wizard but only has qstart, qinst is not even set. GLPI has a wizard, but only qstart and no qinst at all even if silent_install: true.

The only thing I can assume is that Synology sets these flags accordingly, manually during the review before the package gets published.

benrubson commented 6 years ago

Sounds good to me @jdel 👍

If the qfields are not set in the INFO file, all of them will be set to false, otherwise the value from INFO is picked.

Reading at the fix-qinst branch code, all of them are set to true by default, to false if there is a WIZARD_UIFILES dir, or to their INFO value if exists. Perfect.

Not sure you should provide the ability to set q* values into the INFO file as they are not part of the documented fields. But perhaps it could help debugging a future case...

Ah, and what about touching a <package-name>.nowiz file to avoid opening the spk everytime when it does not include a WIZARD_UIFILES dir ? :)

Thx 👍

mesa57 commented 6 years ago

Test is succesfull 😄 The "qinst":false is returned and dsm will show in install-uifile on installation. json :


{
    "packages": [{
        "package": "Spotweb",
        "version": "20180118-1",
        "dname": "Spotweb",
        "desc": "Spotweb is a webbased usenet binary resource indexer based on the protocol and development done by Spotnet.",
        "price": 0,
        "download_count": 0,
        "recent_download_count": 0,
        "link": "http://wheezy/sspks/packages/Spotweb_noarch-all_20180118-1.spk",
        "size": 3051520,
        "md5": "737460949739bf7de3849bde5a3c349e",
        "thumbnail": ["http://wheezy/sspks/cache/Spotweb_noarch-all_20180118-1_thumb_72.png",
        "http://wheezy/sspks/cache/Spotweb_noarch-all_20180118-1_thumb_120.png"],
        "snapshot": [],
        "qinst": false,
        "qstart": false,
        "qupgrade": false,
        "depsers": "apache-web db",
        "deppkgs": "",
        "conflictpkgs": null,
        "start": true,
        "maintainer": "Spotweb",
        "maintainer_url": "",
        "distributor": "",
        "distributor_url": "",
        "changelog": "1. Update spk to support new installation.",
        "thirdparty": true,
        "category": 0,
        "subcategory": 0,
        "type": 0,
        "silent_install": false,
        "silent_uninstall": false,
        "silent_upgrade": false,
        "beta": false
    }]
}
jdel commented 6 years ago

@benrubson

Not sure you should provide the ability to set q* values into the INFO file as they are not part of the documented fields. But perhaps it could help debugging a future case...

I understand that, but considering it's pretty convenient for everyone, let's call it "added value" :)

Ah, and what about touching a .nowiz file to avoid opening the spk everytime when it does not include a WIZARD_UIFILES dir ? :)

Yes, thanks for pointing that out. This is exactly why github is great ! I will add this now, but I am tempted to use a INI library in order to add wizard="yes" or wizard="no" in the INFO file instead just for the sake of not creating extra files.

@mesa57: Thanks for testing this out. It will be soon merged to master, and I'll create a release / docker image too.

jdel commented 6 years ago

Closing this issue now it has been merged back to master.

Release v1.1.2 is out too, so is the docker image

benrubson commented 6 years ago

Perfect, many thx 👍 Finally you went with a .nowiz file :) Simple, perfect !

mesa57 commented 6 years ago

Thanks, will inform nzbusenet.