Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sickchill package updates #4538

Closed
wants to merge 2 commits into from
Closed

sickchill package updates #4538

wants to merge 2 commits into from

Conversation

miigotu
Copy link
Contributor

@miigotu miigotu commented Apr 2, 2021

remove cross #4161 (comment)
set update_frequency to 24 hours #4161 (comment)
fix upgrade, force port to 8081 https://github.com/SickChill/SickChill/issues/7070

Removed the pure python forcing, because lxml, greenleaf, sqlalchemy, tornado, mako, and cryptography have c extensions
We can build without c extensions for most of them, except cryptography, but they all make the package run faster and with less resources if compiled.

@publicarray @ymartin59 Please let me know of any requested changes as soon as possible. Users of the existing package are having update issues because of the port.
Please do not merge until tested.

Signed-off-by: miigotu [email protected]

Checklist

  • Build rule all-supported completed successfully
  • Package upgrade completed successfully
  • New installation of package completed successfully

Copy link
Member

@publicarray publicarray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok when we wanted cross/sickchill removed it was because I was under the assumption that this was a pure python package. So we probably want to revert part of this change here (sorry)

Please make sure the source code for your package is included otherwise the package will fail to install.

Since @BenjV has a noarch package: https://github.com/BenjV/SYNO-packages/blob/master/SickChill%201.2.Python%203%20noarch.spk I'd like he's feedback. Is it worth cross compiling for a potential speedup? Has anyone noticed a performance issue with the noarch version? I'm asking because cross compiling takes significantly more time for me to publish with seemingly little benefit.

I've made some changes here: https://github.com/publicarray/spksrc/commits/sickchill-cross
Please let me know if this looks good, if you like you can cherry-pick the commit or patch file:
https://github.com/publicarray/spksrc/commit/d20c8eeb786c6573a4151c7df01a68482202154d.patch

@miigotu
Copy link
Contributor Author

miigotu commented Apr 3, 2021

Thanks, I'm sure there are a lot more changes that will need made while the package is young. Hopefully this works.

@miigotu
Copy link
Contributor Author

miigotu commented Apr 11, 2021

!DNM

@miigotu
Copy link
Contributor Author

miigotu commented Apr 11, 2021

@publicarray I do not think cross compiling is proper here unless we can just cross-compile the python requirements and then clone SC. We need to retain git history and cross currently would download a tarball which breaks SC. I am not sure git is always available in the package build environment, so cloning inside cross/* might not always work. As an upside, we can now allow users to select the git branch at install. The purpose of that is so that users can test changes on synology to see if their issues are fixed without pushing a new update to synocommunity. (This is exactly how it is done in sickbeard/sickbeard-custom and sickrage packages also)

We also do not need to require a cross compile of python38, because python3.7 is perfectly sufficient and is provided by synology out of the box.

@miigotu
Copy link
Contributor Author

miigotu commented Apr 19, 2021

Anyone know why the service is created with no privileges? There is no shortcut until you manually go into the privileges applet and grant, and then restart the service manually.

package.tgz/app/config has the proper info generated afaik:

{ ".url": { 
  "com.synocommunity.packages.sickchill": {
    "title": "SickChill",
    "desc": "Automatic Video Library Manager for TV Shows. It watches for new episodes of your favorite shows, and when they are posted it does its magic.",
    "icon": "images/sickchill-{0}.png",
    "type": "url",
    "protocol": "http",
    "port": "8081",
    "url": "/",
    "allUsers": true,
    "grantPrivilege": "all",
    "advanceGrantPrivilege": true
} } }

@BKSteve
Copy link
Contributor

BKSteve commented May 22, 2021

Is this update frozen?

@miigotu
Copy link
Contributor Author

miigotu commented May 22, 2021

Is this update frozen?

This is just the packaging source, changes to SC are applied when you update SC inside after installing.

@BKSteve
Copy link
Contributor

BKSteve commented May 22, 2021

It's just no update on Community since 'Initial version' Nyaran.
Is this update going to also look for the Synology Python3 package rather than community 3.7.10?

@miigotu
Copy link
Contributor Author

miigotu commented May 23, 2021

Both pythons packages are acceptable, we just require python3 >3.6

@miigotu
Copy link
Contributor Author

miigotu commented May 23, 2021

Once the package is set proper on community, it never needs updated again unless requirements change.

SPK_REV = 1
SPK_ICON = src/sickchill.png

DEPENDS = cross/$(SPK_NAME)
SPK_DEPENDS = "python3>=3.6:git"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@publicarray can we do python3>=3.6||python>=3.6:git here? Or is there some other reason Synology official python won't satisfy the dependency?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we can't. We must decide for one or the other (or create two different packages like sickchill/Sickchill (or sc-sickchill/synology-sickchill), but as git package is by synocommunity I would prefere to keep it like "python3;git"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it wasnt the git package I was worried about, it is the confusion by some people who already have python 3 installed from syno official and then are asked to install python3 from sc when they install sickchill. One is named python, the other is named python3, even though both are python3.6+

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a rule of thumb the (internal) name of packages from SynoCommunity are all lower case and the packages from synology start with uppercase letter (synocommunity: mono, synology: Mono)

@BenjV
Copy link

BenjV commented Jul 6, 2021

@hgy59
Because DSM 7 has python 3 standard installed you could remove the dependency to python 3 from the INFO file.

If you add this to the start-stop-status script it would use the python from the SynoCommunity if installed, else the standard python3 from DSM.

PIDFILE="${SYNOPKG_PKGTMP}/${SYNOPKG_PKGNAME}.pid"
if [ -x "/var/packages/python38/target/bin/python3.8" ]
then
      PYTHON="/var/packages/python38/target/bin/python3.8"
else
      PYTHON="python3"
fi
env LANG=en_US.UTF-8 "${PYTHON}" "${SYNOPKG_PKGDEST}/start.py" -q --daemon --pidfile="${PIDFILE}" --datadir="${SYNOPKG_PKGVAR}" --port=${SYNOPKG_PKGPORT}

@miigotu
Copy link
Contributor Author

miigotu commented Jul 15, 2021

Im reworking this package for dsm7 (are the changes necessary usually backwards compatible to dsm6?)
Do you guys have a discord/telegram/irc or something to discuss easier?

@BenjV
Copy link

BenjV commented Jul 16, 2021

Not backwards compatible, DSM 7 needs this in the INFO file:

os_min_ver="7.0-40000"

This will prevent it from being able to install it on DSM versions lower then 7
Also on DSM 7 you cannot run packages nor install script as root, so that has to be removed from the privilege file.
This file must now look like this:

{
  "defaults": {
    "run-as": "package"
  },
  "username": "sc-medusa",
  "groupname": "synocommunity"
}

So there must be two differnet packages, one for DSM 7 and higher and one for DSM 6 and lower.

@hgy59
Copy link
Contributor

hgy59 commented Jul 16, 2021

Im reworking this package for dsm7 (are the changes necessary usually backwards compatible to dsm6?)

The intention is to create packages for dsm6 and dsm7 with a single package definition (i.e. files below spk/{package-name}. Most of the compatibility is done in the development environment (i.e. the spksrc framework). The so called generic installer uses different installer generation for DSM7, DSM6 (and DSM5).
For special cases, it is possible to execute DSM version specific code for a package in service-setup.sh by evaluation of the SYNOPKG_DSM_VERSION_MAJOR variable.

@miigotu
Copy link
Contributor Author

miigotu commented Jul 17, 2021

Trying to fix and update in #4743 - This is outdated but I will come back to this branch if the other approach does not work out

@miigotu miigotu closed this Jul 17, 2021
@boomvalk
Copy link

After updating all packages on my Synology, including Sickchill: SickChill nor Sickrage will startup again "Failed to run the package service."I have DSM 6.2, Sickchill210329-01, Python 3, Pytohn 3.8, Python all installed, mono 5.2.1.34-16, SABnzbbd 3.2.1-48,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants