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 update to fix v3 install errors #4964

Merged
merged 4 commits into from
Nov 17, 2021
Merged

SickChill update to fix v3 install errors #4964

merged 4 commits into from
Nov 17, 2021

Conversation

BKSteve
Copy link
Contributor

@BKSteve BKSteve commented Nov 11, 2021

Improved updating by clearing main core files for new one built in the spk.
Python 3.8.12-6 or greater check.

Motivation: Fix v-3 installation issues on non x64 devices.

Edit
All builds install except DSM 7.0 Build which gives invalid file format and seek additional testers or for somebody else to build the evansport 7.0 package to see if it is my build that causes the issue.

Improved updating by clearing main core files for new one built in the spk.
Python 3.8.12-6 or greater check.
@BKSteve
Copy link
Contributor Author

BKSteve commented Nov 11, 2021

I see the builds failing with

Error: ERRORS:
2021.11.11 15:47:00 - sickchill: (x64-7.0) FAILED

See log file of the build job to analyze the error(s).

Last 15 lines of the build log:

$ make TCVERSION=7.0 ARCH=x64 -C ./spk/sickchill
make[2]: Leaving directory '/github/workspace/cross/cffi'
make[2]: Entering directory '/github/workspace/cross/libsodium'
===>  Downloading files for libsodium
===>    File libsodium-1.0.18-stable.tar.gz already downloaded
===>  Verifying files for libsodium
===>    Checking sha1sum of file libsodium-1.0.18-stable.tar.gz
===>      Wrong sha1sum for file libsodium-1.0.18-stable.tar.gz
===>      Renamed as libsodium-1.0.18-stable.tar.gz.wrong
===>      Download cookie removed to trigger the download again
make[2]: *** [../../mk/spksrc.checksum.mk:42: checksum_target] Error 1
make[2]: Leaving directory '/github/workspace/cross/libsodium'
make[1]: *** [../../mk/spksrc.depend.mk:51: depend_target] Error 2
make[1]: Leaving directory '/github/workspace/cross/PyNaCl'
make: *** [../../mk/spksrc.depend.mk:51: depend_target] Error 2
make: Leaving directory '/github/workspace/spk/sickchill'

My local build says

make[4]: Entering directory '/spksrc/cross/libsodium'
===>  Downloading files for libsodium
===>    File libsodium-1.0.18-stable.tar.gz already downloaded
===>  Verifying files for libsodium
===>    Checking sha1sum of file libsodium-1.0.18-stable.tar.gz
===>    Checking sha256sum of file libsodium-1.0.18-stable.tar.gz
===>    Checking md5sum of file libsodium-1.0.18-stable.tar.gz
/spksrc/cross/libsodium/../../distrib/libsodium-1.0.18-stable.tar.gz
===>  Processing dependencies of libsodium
===>  Extracting for libsodium

@BKSteve
Copy link
Contributor Author

BKSteve commented Nov 11, 2021

I went looking for changes in the build packages and note Bazarr 1.0.0 (#4916) updates mk/spksrc.wheel.mk
Bazarr update

start line 89

post_wheel_target: $(WHEEL_TARGET)
	@if [ -d "$(WHEELHOUSE)" ] ; then \
		mkdir -p $(STAGING_INSTALL_WHEELHOUSE) ; \
		cd $(WHEELHOUSE) ; \
+		if stat -t requirements*.txt >/dev/null 2>&1; then \
        	cat requirements*.txt > $(STAGING_INSTALL_WHEELHOUSE)/$(WHEELS_PURE_PYTHON) ; \
+        fi ;\
		if [ "$(EXCLUDE_PURE_PYTHON_WHEELS)" = "yes" ] ; then \
			echo "Pure python wheels are excluded from the package wheelhouse." ; \
			for w in *.whl; do \
				if echo $${w} | grep -viq "-none-any\.whl" ; then \
					cp -f $$w $(STAGING_INSTALL_WHEELHOUSE)/`echo $$w | cut -d"-" -f -3`-none-any.whl; \
				fi ; \
			done ; \
		else \
			for w in *.whl; do \
				cp -f $$w $(STAGING_INSTALL_WHEELHOUSE)/`echo $$w | cut -d"-" -f -3`-none-any.whl; \
			done ; \
		fi ; \
	fi

Does this new end if cause the problem?
Reversed this edit and am building now, will report if this resolves the issue on the DSM7 builds

@th0ma7
Copy link
Contributor

th0ma7 commented Nov 11, 2021

Feel free to give it a shot against #4951 as there are a few key fixes out there.

@BKSteve
Copy link
Contributor Author

BKSteve commented Nov 11, 2021

I was really hoping to get them all fixed up and then this DSM7 build issue for evansport and armv7 giving invalid file format.
Which is bazaar or bazarr?? 🏋️

@publicarray
Copy link
Member

libsodium changed their archives again.

@BKSteve
Copy link
Contributor Author

BKSteve commented Nov 12, 2021

Would this explain why the DSM 7 builds are all giving the file format error?

@miigotu
Copy link
Contributor

miigotu commented Nov 12, 2021

I went looking for changes in the build packages and note Bazarr 1.0.0 (#4916) updates mk/spksrc.wheel.mk Bazarr update

start line 89

post_wheel_target: $(WHEEL_TARGET)
	@if [ -d "$(WHEELHOUSE)" ] ; then \
		mkdir -p $(STAGING_INSTALL_WHEELHOUSE) ; \
		cd $(WHEELHOUSE) ; \
+		if stat -t requirements*.txt >/dev/null 2>&1; then \
        	cat requirements*.txt > $(STAGING_INSTALL_WHEELHOUSE)/$(WHEELS_PURE_PYTHON) ; \
+        fi ;\
		if [ "$(EXCLUDE_PURE_PYTHON_WHEELS)" = "yes" ] ; then \
			echo "Pure python wheels are excluded from the package wheelhouse." ; \
			for w in *.whl; do \
				if echo $${w} | grep -viq "-none-any\.whl" ; then \
					cp -f $$w $(STAGING_INSTALL_WHEELHOUSE)/`echo $$w | cut -d"-" -f -3`-none-any.whl; \
				fi ; \
			done ; \
		else \
			for w in *.whl; do \
				cp -f $$w $(STAGING_INSTALL_WHEELHOUSE)/`echo $$w | cut -d"-" -f -3`-none-any.whl; \
			done ; \
		fi ; \
	fi

Does this new end if cause the problem? Reversed this edit and am building now, will report if this resolves the issue on the DSM7 builds

That might do it, because there aren't supposed to be spaces for indents in a mk file. Change the spaces before the cat and the fi to tabs. (3 tabs before the cat, 2 tabs before the fi)

@BKSteve
Copy link
Contributor Author

BKSteve commented Nov 12, 2021

Some poor Bazarr editing to the mk file.
@publicarray

@BKSteve
Copy link
Contributor Author

BKSteve commented Nov 12, 2021

So how to resolve the DSM7 version build issue?
Also in the checks and build below does it make the spk and is built spk accessible?

@th0ma7
Copy link
Contributor

th0ma7 commented Nov 12, 2021

Just a thought... DSM6 $HOME is pointing under the target dir while with DSM7 $HOME is now located under:

/var/packages/<package>/home -> /volume1/@apphome/<package>

What I noticed is that with DSM7 the virtualenv now gets created in $HOME/.local/share/virtualenv which means that its not under target anymore like previously with DSM6. So startup or service setup scripts might fail as still pointing to the previous virtual environment location. I sort of noticed this with tvheadend.

Although note that there still is a target/env/bin containing pip and python3 but it might get screwed-up when upgrading from DSM6 to DSM7 as $HOME changed between the two, thus not finding the background virtualenv.

@publicarray
Copy link
Member

publicarray commented Nov 12, 2021

Thanks for the find @BKSteve & @miigotu I missed it during review. Unfortunately I don't have my machine for the next few days. @th0ma7 you be able to push a fix?

The spaces should be tabs.

@BKSteve
Copy link
Contributor Author

BKSteve commented Nov 13, 2021

#4970 cleans the 2 lines

@BKSteve BKSteve mentioned this pull request Nov 13, 2021
37 tasks
@BKSteve
Copy link
Contributor Author

BKSteve commented Nov 15, 2021

OK, on my testing I was using 3.xx for all builds. DSM6 no issue with decimal build numbers but the DSM7 has issues any build number with a decimal would create invalid file format.
I never tested 4 as a whole number but now confirm the 4 as uploaded builds correctly and is loadable by my x64 DSM7.

I will have the tests on DSM7 machines and confirm clear to release.

  • evansport 7.0
  • armv7 7.0
  • x64 7.0

@hgy59
Copy link
Contributor

hgy59 commented Nov 15, 2021

DSM7 has issues any build number with a decimal would create invalid file format.

Your find the regex for DSM 7 version validation in #4524 footnote 9).
I hope that synology didn't change this with DSM 7.0.1.
So make sure that the dash is - (ASCII 45dec) and dot is . (ASCII 46dec).

Anyway, as you speak of sickchill 3.x I hope you can change the version to that format instead of using a timestamp.
We prefer official versions over timestamped or git-hash versions.

EDIT:
just looked at https://github.com/SickChill/SickChill/tags and saw that the Releases are 2021.11.10, 2021.11.07, ...
So it is OK to use this format (but you will not be able to change later to 3.x without breaking the package update).
The format yyyy.mm.dd should not be the cause for DSM 7 invalid package format as it works well with MinIO Package.

The invalid version format (former minio and sslh packages) was reported by an error log in the file /var/log/messages.
You might find the reason for the error of sickchill on DSM 7 in this file.

@BKSteve
Copy link
Contributor Author

BKSteve commented Nov 16, 2021

I will now use a different naming convention whilst building test version. As I was building on v-3 I was using 3.xx so anybody on a partial release of 3 would be able to upgrade to v-4 once it comes out. At least that was my thinking.
I will do a couple of tests but am thinking that the test numbering will be 4-xx and not 4.xx moving forward as v-4 I believe is good to go.

There's also now a pull request that links invalid file format to integer numbered releases and a standard 'full stop' hex 2e or ASCII 46 causes the error. I double checked it against the regex string [^\d+(\.\d+){0,5}(-\d+)?$] and the dot being the one between comma and / on my keyboard and not some special 'hard dot' character is used. So this should be looked at and others know this.

Add: version numbering is yyyymmdd-version (no '.') but whilst testing I was using yyyymmdd-version.testbuild and the .testbuild so when moving to version 4 it would be a clean update which caused the invalid file due to the .testbuild. Before v-3 was released my testing was just an increment on versions so was up to 20210729-22. We know v-3 had issues and as I wasn't sure a new release of SickChill was coming I did the .testbuild rather than -23. Which got me in the ass. But that's it. v-4 is ready and I have a different plan for test builds in the future.

@miigotu
Copy link
Contributor

miigotu commented Nov 16, 2021

I will now use a different naming convention whilst building test version. As I was building on v-3 I was using 3.xx so anybody on a partial release of 3 would be able to upgrade to v-4 once it comes out. At least that was my thinking. I will do a couple of tests but am thinking that the test numbering will be 4-xx and not 4.xx moving forward as v-4 I believe is good to go.

There's also now a pull request that links invalid file format to integer numbered releases and a standard 'full stop' hex 2e or ASCII 46 causes the error. I double checked it against the regex string [^\d+(\.\d+){0,5}(-\d+)?$] and the dot being the one between comma and / on my keyboard and not some special 'hard dot' character is used. So this should be looked at and others know this.

Imho you should use the SickChill version tag as the spk version as well. So whenever you build the spk with a new version of SC, you know what version is in the spk very easily.

I know that doesn't help if you release multiple versions in one day, but sickchill is going to be modifying the release policy so that only one release is allowed per day, and hotfixes will not be versioned. The hotfix will be pushed to master and built into a proper release the next day.

This is nice then because for semver reasons there will be a 2021 tag (newest version for year 2021), a 2021.11 tag (the latest version for the month), and a daily tag 2021.11.15.

I would eventually like to remove the git and source based updater completely, and only use pip/poetry for upgrades, while improving stability of releases.

@BKSteve
Copy link
Contributor Author

BKSteve commented Nov 16, 2021

Imho you should use the SickChill version tag as the spk version as well. So whenever you build the spk with a new version of SC, you know what version is in the spk very easily.

Yes, I've moved from v-3 using 20210729 [20210729-3 version name] to v-4 using 20211110 giving version 20211110-4.
I agree that this way it's very clear what version is built inside the package. It's just I didn't put this number in the title of the pull but it's in all the code.

SPK_NAME = sickchill
- SPK_VERS = 20210729
- SPK_REV = 3
+ SPK_VERS = 20211110
+ SPK_REV = 4

@publicarray
Copy link
Member

publicarray commented Nov 16, 2021

2021.11

That is no longer possible as the version must always be increasing. So the next version must be larger than 20210729-3

@publicarray
Copy link
Member

publicarray commented Nov 16, 2021

Thanks @BKSteve That dot thing is a bit surprising for me too. I did know that DSM7 has tighter version checks but yea wow. You'd think the error message could be more obvious.

Just a heads up I will be without my dev machine for at least a month.

@BKSteve
Copy link
Contributor Author

BKSteve commented Nov 16, 2021

Can we have the v-4 rolled out before that then. It's well tested now and ready.
Then I can close the 400+ message issue 7504 and work with th0ma7 on Py310 and the wheels.

@publicarray
Copy link
Member

@th0ma7 or @hgy59 will have to do that when they can. I can't really do anything at the moment.

@th0ma7
Copy link
Contributor

th0ma7 commented Nov 16, 2021

@publicarray in your absence I'll take care of it.
@BKSteve to confirm, this is good for a merge & publish?

@publicarray
Copy link
Member

Thanks mate!

@BKSteve
Copy link
Contributor Author

BKSteve commented Nov 17, 2021

It's well tested now and ready.

@th0ma7 Yes, it's ready. All the testers confirm installed and working.
I built it as v-4 and put in my share folder, so tested as it will be built (DSM7 issue was last fix required)'

It's ready. And will resolve all v-3 issues that we know of.

@th0ma7 th0ma7 merged commit ccee440 into SynoCommunity:master Nov 17, 2021
@th0ma7 th0ma7 added status/published Published and activated (may take up to 48h until visible in DSM package manager) and removed status/ready-to-merge status/to-publish labels Nov 17, 2021
@th0ma7
Copy link
Contributor

th0ma7 commented Nov 17, 2021

Now meged and published.

@BKSteve BKSteve deleted the SickChill_4 branch November 18, 2021 05:19
publicarray pushed a commit to publicarray/spksrc that referenced this pull request Dec 19, 2021
Improved updating by clearing main core files for new one built in the spk.
Python 3.8.12-6 or greater check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants