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 use a pypi based install - from 5431 #5449

Merged
merged 15 commits into from
Oct 18, 2022
Merged

SickChill use a pypi based install - from 5431 #5449

merged 15 commits into from
Oct 18, 2022

Conversation

BKSteve
Copy link
Contributor

@BKSteve BKSteve commented Oct 13, 2022

Description

SickChill update test with #5431

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Bug fix
  • New Package
  • Package update
  • Includes small framework changes
  • This change requires a documentation update (e.g. Wiki)

Tested

  • x64-6.1
  • x64-7.0
  • armv7-6.1
  • armv7-7.0
  • evansport-6.1
  • evansport-7.0

@BKSteve BKSteve changed the title SickChill use a pypi based install - build testing only SickChill use a pypi based install - build testing only (5431) Oct 13, 2022
@th0ma7
Copy link
Contributor

th0ma7 commented Oct 13, 2022

Have you got it working? If so, what was missing (seing the last build being succesful)

@BKSteve
Copy link
Contributor Author

BKSteve commented Oct 13, 2022

Waiting for others not using x64 to report back.
Everything else pretty clear with the changes

@BKSteve
Copy link
Contributor Author

BKSteve commented Oct 14, 2022

@th0ma7 ultimately it was getting the requirements sorted out. (a couple of typos and bits)
Need to cross/cryptography (copied from your python 310 makefile)
Then a sickchill pyproject.toml clean up.

As of the last build using 20221013 and writing this have 1 positive working on a DS114 armv7 DSM6
Everybody with x64 is of course working. I've tested on both DSM6 and DSM7 x64 devices.
With the full install there's a couple more pure items that are in python310 which I could still comment out.
distlib, platformdirs, filelock, virtualenv

@BKSteve BKSteve changed the title SickChill use a pypi based install - build testing only (5431) SickChill use a pypi based install - from 5431 Oct 14, 2022
@BKSteve
Copy link
Contributor Author

BKSteve commented Oct 15, 2022

The pip-cache going into /var or /@appdata is noted.

Wondered why cachedir=${SYNOPKG_PKGVAR}/pip-cache in spksrc.service.installer.functions is SYNOPKG_PKGVAR rather than say SYNOPKG_PKGDEST/share

As this puts the cached pip items in the user variable location rather than package location. (@appdata not @appstore)

@BKSteve
Copy link
Contributor Author

BKSteve commented Oct 15, 2022

We have x64, armv7 and evansport confirmed working on both DSM 6 & 7.

@th0ma7 grateful if you could run you eyes over this and then merge to master if all good.

Copy link
Contributor

@th0ma7 th0ma7 left a comment

Choose a reason for hiding this comment

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

Overall LGTM besides building duplicate cryptography wheels (see comments below).

spk/sickchill/Makefile Outdated Show resolved Hide resolved
spk/sickchill/src/service-setup.sh Outdated Show resolved Hide resolved
spk/sickchill/src/requirements-crossenv.txt Show resolved Hide resolved
@th0ma7
Copy link
Contributor

th0ma7 commented Oct 15, 2022

The pip-cache going into /var or /@appdata is noted.

Wondered why cachedir=${SYNOPKG_PKGVAR}/pip-cache in spksrc.service.installer.functions is SYNOPKG_PKGVAR rather than say SYNOPKG_PKGDEST/share

As this puts the cached pip items in the user variable location rather than package location. (@appdata not @appstore)

Previously everything was under the target directory (e.g. SYNOPKG_PKGDEST). Starting with DSM7 you can uninstall a package without uninstalling the user-files and upgrade also now avoid impacting user-files as well (on DSM6 we needed to temporarily copy them elsewhere).

For all theses reasons, having the pip-cache located there makes it relevant as otherwise it would get deleted at every package upgrade, thus removing the interest of caching in the first place. By being under the var directory, on older much slower targets, it fasten the upgrade process significantly.

@BKSteve
Copy link
Contributor Author

BKSteve commented Oct 17, 2022

@th0ma7 I'm having some issues with the USER used with DSM6 installation.
The USER isn't consistently using sc-sickchill but instead using sickchill as USER.

I tried forcing USER in service-setup.sh with the following grep === sickchill.log > SC.log result

service-setup.sh without USER definition

2022/10/17 21:52:05	===> Step preupgrade. USER=sickchill GROUP=sc-download SHARE_PATH=
2022/10/17 21:52:10	===> Step preuninst. USER=sickchill GROUP=sc-download SHARE_PATH=
2022/10/17 21:52:15	===> Step postuninst. USER=sickchill GROUP=sc-download SHARE_PATH=
2022/10/17 21:52:16	===> Step preinst. USER=sickchill GROUP=sc-download SHARE_PATH=
2022/10/17 21:52:20	===> Step postinst. USER=sickchill GROUP=sc-download SHARE_PATH=
2022/10/17 21:57:51	===> Step postupgrade. USER=sickchill GROUP=sc-download SHARE_PATH=

service-setup.sh with USER="sc-sickchill"

2022/10/17 22:31:20	===> Step preupgrade. USER=sc-sickchill GROUP=sc-download SHARE_PATH=
2022/10/17 22:31:23	===> Step preuninst. USER=sickchill GROUP=sc-download SHARE_PATH=
2022/10/17 22:31:25	===> Step postuninst. USER=sickchill GROUP=sc-download SHARE_PATH=
2022/10/17 22:31:27	===> Step preinst. USER=sc-sickchill GROUP=sc-download SHARE_PATH=
2022/10/17 22:31:29	===> Step postinst. USER=sc-sickchill GROUP=sc-download SHARE_PATH=
2022/10/17 22:34:50	===> Step postupgrade. USER=sc-sickchill GROUP=sc-download SHARE_PATH=

Because of this it doesn't / I can't get it to consistently backup and restore the files and folders which are all sc-sickchill

@th0ma7
Copy link
Contributor

th0ma7 commented Oct 18, 2022

Sorry for the very limited cycles this week (and on my cellphone). Try setting up SERVICE_USER = AUTO in Makefile.

@BKSteve
Copy link
Contributor Author

BKSteve commented Oct 18, 2022

Capitalising auto to AUTO in makefile resolved the upgrading. Now the SPK does the backup and restore of the var folder without further code.

@BKSteve BKSteve requested a review from th0ma7 October 18, 2022 04:19
@th0ma7
Copy link
Contributor

th0ma7 commented Oct 18, 2022

Awesome, great work! And yes you can simply use make spkclean or wheelclean to invoke rebuilds.

Leave me a few day before doing a review + merge + publishing. Ping me by EOW if you don't hear back from me.

@th0ma7 th0ma7 merged commit 79c294e into SynoCommunity:master Oct 18, 2022
@th0ma7
Copy link
Contributor

th0ma7 commented Oct 18, 2022

awesome work, thnx for your contribution!
now merged and building + publishing in progress

@th0ma7 th0ma7 added status/published Published and activated (may take up to 48h until visible in DSM package manager) build/rust Requires rust build system labels Oct 18, 2022
@BKSteve BKSteve mentioned this pull request Oct 19, 2022
10 tasks
@BKSteve
Copy link
Contributor Author

BKSteve commented Oct 19, 2022

Sorry think I messed this up. #5458
Did this so I can pull the built packages zip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build/rust Requires rust build system status/published Published and activated (may take up to 48h until visible in DSM package manager)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants