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

Fix Tvheadend and ffmpeg for DMS 7 #4545

Merged
merged 15 commits into from
Jun 19, 2021

Conversation

publicarray
Copy link
Member

@publicarray publicarray commented Apr 8, 2021

Motivation: cherry-picked commits from dsm7-packages branch and added fixes to uriparser and tvheadend version number
Linked issues: #4524

Checklist

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

@publicarray publicarray changed the title Fix Tvheaded and ffmpeg for DMS 7 Fix Tvheadend and ffmpeg for DMS 7 Apr 8, 2021
@publicarray publicarray mentioned this pull request Apr 8, 2021
2 tasks
@publicarray
Copy link
Member Author

On the dsm 6.x the version upgrade worked. (probably because the revision number is newer)

@publicarray publicarray force-pushed the tvheaded-ffmpeg branch 2 times, most recently from 14c5127 to ce7d822 Compare April 8, 2021 12:00
@publicarray
Copy link
Member Author

publicarray commented Apr 9, 2021

  • ejabberd & erlang is out of scope in this PR. (changes in expat caused them to build, and they failed to build before: update ejabberd to v21.01 #4333)
    * chromaprint is proven difficult and IMHO should be addressed in a separate PR as well.

@publicarray publicarray requested a review from th0ma7 April 9, 2021 08:07
@th0ma7
Copy link
Contributor

th0ma7 commented Apr 9, 2021

@publicarray I quickly went over this PR and at first glance things looked OK.
But indeed it failed to build miserably due to expat. It looked like it was unable to download properly from sourceforge.net as the URL changed. Can that be fixed so it builds?

I'll have another look at it over the week-end and from there might be able to propose experimental package on my github for the community to play with so we get some feedbacks.

cross/expat/Makefile Outdated Show resolved Hide resolved
@hgy59
Copy link
Contributor

hgy59 commented Apr 9, 2021

I would like to see this merged asap, as ejabberd depends on expat too (#4333).

publicarray added a commit to publicarray/spksrc that referenced this pull request Apr 11, 2021
publicarray added a commit to publicarray/spksrc that referenced this pull request Apr 11, 2021
publicarray added a commit to publicarray/spksrc that referenced this pull request Apr 11, 2021
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.

Here's a first review.
Still compiling in the background in the lookout for issues.

cross/libaom/Makefile Outdated Show resolved Hide resolved
cross/tvheadend/Makefile Outdated Show resolved Hide resolved
cross/tvheadend/digests Outdated Show resolved Hide resolved
spk/tvheadend/Makefile Show resolved Hide resolved
@th0ma7
Copy link
Contributor

th0ma7 commented Apr 11, 2021

As for chromaprint, adding CMAKE_ARGS += -DCMAKE_CXX_FLAGS="-Wno-deprecated-declarations" helps significantly under DSM7 but still fails. I believe the code change may be relatively easy to figure out to get this one going as well.

@hgy59
Copy link
Contributor

hgy59 commented Apr 11, 2021

as #4539 is now on the master branch, you have to remove >> ${INST_LOG} in service-setup.sh for all service_* functions to work.

publicarray added a commit to publicarray/spksrc that referenced this pull request Apr 12, 2021
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.

I noticed a few variables needing adjusting in the code below in order to get closer to standard.
Perhaps a bit of it is out of scope of this PR...
But timing would be good I believe to go over them now.
Let me know what you think.

spk/tvheadend/src/service-setup.sh Outdated Show resolved Hide resolved
spk/tvheadend/src/service-setup.sh Outdated Show resolved Hide resolved
DVR_DIR=$(grep -e 'storage\":' ${file} | awk -F'"' '{print $4}')
REAL_DIR=$(realpath "${DVR_DIR}")
TRUNC_DIR=$(echo "${REAL_DIR}" | cut -c9-28)
CHECK_VAR=$(echo "${REAL_DIR}" | cut -c29-32)
# Replace any remaining target links in log files first
sed -i -e "s,/var/packages/tvheadend/target,/usr/local/tvheadend,g" "${file}"
Copy link
Contributor

Choose a reason for hiding this comment

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

And here it would be better off using ${SYNOPKG_PKGDEST} par of sed.

Copy link
Member Author

@publicarray publicarray Apr 13, 2021

Choose a reason for hiding this comment

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

This sed command will work in DSM7 but the path won't be available. Why is it needed anyway? Why can't the app do its thing in /var/packages/tvheadend?

The /usr/local/{package} folder is old DSM5 stuff and not supported on DSM7.

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly I have no clue.
I guess this sed could just be removed entirely.

Copy link
Member Author

@publicarray publicarray Apr 14, 2021

Choose a reason for hiding this comment

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

I tried to find a sample in SYNOPKG_PKGVAR}/dvr/log but I don't see any. I guess I need to have a receiver and start a recording.(which I don't have)

Copy link
Contributor

Choose a reason for hiding this comment

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

But I do :)

Got the following on my DSM6:

root@th0ma7-nas:/var/packages/tvheadend/target/var/dvr# tree -d
.
├── autorec
├── config
└── log

3 directories

Honestly I've been wishing to rework all this for a year but never took the time to do so. The backup/recovery process does not work, period.

  1. For instance the /var/packages/tvheadend/target/var/config references to /usr/local/tvheadend should be seded to /var/package/tvheadend (clearly the "binary matches" isn't good in the grep below):
# grep -R "/usr/local/tvheadend" *
Binary file bin/tvheadend matches
var/channel/config/65ab00d40fe4addffdf4babbc3b390d4:	"icon": "file://usr/local/tvheadend/.xmltv/",
var/channel/config/f87d3e9167eceb6d6f13e9ab7739f0cb:	"icon": "file://usr/local/tvheadend/.xmltv/",
var/channel/config/0cdf1b095014ab7990f9c8569a69270b:	"icon": "file://usr/local/tvheadend/.xmltv/",
var/channel/config/1fbe2fe151b5c6d9f555a69ce61af02f:	"icon": "file://usr/local/tvheadend/.xmltv/",
var/channel/config/3b1392ac0539b5b4a400b0d81d4e34eb:	"icon": "file://usr/local/tvheadend/.xmltv/",
var/config:	"chiconpath": "file://usr/local/tvheadend/.xmltv/",
var/config:	"muxconfpath": "/usr/local/tvheadend/share/tvheadend/data/dvb-scan/",
  1. This entire directory (e.g. /var/packages/tvheadend/target/var) needs to be backup/restored along with creating a recovery something-config-backup-date.tar.bz in order to recover as needed.
    My personal backup script: https://raw.githubusercontent.com/th0ma7/synology/master/tvheadend-backup.sh
  2. There is a caching directory /var/packages/tvheadend/target/cache that needs to be recovered as well. It would benefit from being located elsewhere under /volume1/... from a user defined param at installation time.
# du -hs cache var
24M	cache
14M	var
  1. And also having a few scripts & configs that needs to be added manually that I should add to the default package...

BTW, just got my tvheadend and ffmpeg packages ready for DSM7... in theory ready for testing :)

Copy link
Contributor

@th0ma7 th0ma7 Apr 18, 2021

Choose a reason for hiding this comment

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

@publicarray I found what the issue is...

Directories accesscontrol and passwd are now located under /var/packages/tvheadend/target/var -> /volume1/@appstore/tvheadend/var) where the password/a927e30a755504f9784f23a4efac5109 looks like:

{
	"enabled": true,
	"username": "admin",
	"password2": "@password@",
	"wizard": true
}

Moving both directories (accesscontrol and passwd) to /var/packages/tvheadend/var -> /volume1/@appdata/tvheadend) then manually fixing the password solved the isse:

mv /volume1/@appstore/tvheadend/var/* /volume1/@appdata/tvheadend/.
wizard_password=$(echo -n "TVHeadend-Hide-${wizard_password:=admin}" | openssl enc -a)
sed -i -e "s/@password@/${wizard_password}/g" /var/packages/tvheadend/var/a927e30a755504f9784f23a4efac5109

EDIT: Thus the issue lies either with the changes of ${SYNOPKG_PKGDEST} vs ${SYNOPKG_PKGVAR}... or how the tvheadend_extra_install part of the Makefile is being done.

Copy link
Contributor

Choose a reason for hiding this comment

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

@publicarray reminder on this.
I've looked at compile options but can't see something obvious that allows moving around theses two directories...
I'll try to have another look at it later and see if I can come-up with a fix somehow.

  --prefix=DIR$                  Installation root [/usr/local]
  --bindir=DIR                   Install binaries in DIR [${prefix}/bin]
  --libdir=DIR                   Install libraries in DIR [${prefix}/lib]
  --mandir=DIR                   Install man pages in DIR [${datadir}/man]
  --datadir=DIR                  Install data files in DIR [${prefix}/share]

Copy link
Contributor

Choose a reason for hiding this comment

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

@publicarray and @hgy59 I believe I have found where the issue is.
And may have found a second one actually... or food for thought.

First issue:
It relates to $(STAGING_DIR) where the entire content is being tgz and ends-up being installed over to /var/packages/tvheadend/target/ (appstore). Although it happens that work-*/staging/var containing both accesscontrol and passwd that should follow the $(SYNOPKG_PKGVAR) and end-up into /var/packages/tvheadend/var (appdata) where all the automatically generated files also end-up going.

An idea to circumvent this:

  1. From the Makefile, install files under staging/appdata using a new variable such as $(STAGING_SPKVAR).
  2. installer script go over all files & directories in staging/appdata. If install just copy to /var/packages/tvheadend/var else perhaps something as trivial as a rsync no-overwrite over the directory (which could be used for all cases actually). Depending of DSM the destination folder is set from SYNOPKG_PKGVAR so it should always work.

Second issue: (not sure about this one)
I wonder if it would make sense to extend solution proposed for the first issue to add variables for $(STAGING_SPKHOME), $(STAGING_SPKETC) and $(STAGING_SPKTEMP) and use rsync to copy files over the end-state location?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or a simple work-around is to move the accesscontrol & passwd directory creation and file copying at the service-setup.sh level.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hgy59 and @publicarray Got a working solution.
Will provide a separate PR to address this later today.

spk/tvheadend/src/service-setup.sh Outdated Show resolved Hide resolved
spk/tvheadend/src/service-setup.sh Outdated Show resolved Hide resolved
spk/tvheadend/src/service-setup.sh Outdated Show resolved Hide resolved
spk/tvheadend/src/service-setup.sh Outdated Show resolved Hide resolved
cross/tvheadend/Makefile Show resolved Hide resolved
@th0ma7 th0ma7 mentioned this pull request Apr 13, 2021
3 tasks
@publicarray
Copy link
Member Author

publicarray commented Apr 13, 2021

@th0ma7 If you want you can add #4551 to this PR*, so we can see green(er) ✔️ s (I initially thought that it was more difficult, so I did not even attempt it, shame on me :) but good work!)

* just checkout this branch and add your commits (I use github's cli: gh pr checkout 4545)

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.

I've included PR #4551 directly in this PR with hope to get all green flags.
Other than the ${SYNOPKG_PKGDEST}/var vs ${SYNOPKG_PKGVAR} variable below which I now have doubts, everything else looks great!
Nice team work!

spk/tvheadend/src/service-setup.sh Outdated Show resolved Hide resolved
@th0ma7
Copy link
Contributor

th0ma7 commented Apr 13, 2021

@publicarray Awesome! There now only ejabberd failing for all arches but otherwise build is succesfull for all ffmpeg related packages. woot!

@hgy59
Copy link
Contributor

hgy59 commented Apr 13, 2021

@th0ma7 ejabberd is WIP in #4333 and waiting for cross/expat download fix with this PR.

@hgy59
Copy link
Contributor

hgy59 commented Apr 13, 2021

regarding SYNOPKG_PKGVAR we have to pay attention.
This variable is provided only by the DSM 7 context.
And it is defined in the generic installer for use with other DSM versions too.

But SYNOPKG_PKGVAR is still restricted to DSM 7 in custom start-stop-status scripts or when a custom installer is used.

publicarray added a commit to publicarray/spksrc that referenced this pull request Apr 14, 2021
@th0ma7
Copy link
Contributor

th0ma7 commented Jun 14, 2021

@publicarray I'll be revisiting this PR over the next few days, starting with rebasing in order to get this merged asap considering the increasing noise over DSM7.

@hgy59 and @ymartin59 would be great if you can find cycles to go over it and do another round of review to confirm all looks OK?

@th0ma7 th0ma7 requested a review from ymartin59 June 14, 2021 11:01
hgy59 pushed a commit to publicarray/spksrc that referenced this pull request Jun 14, 2021
hgy59 pushed a commit to publicarray/spksrc that referenced this pull request Jun 14, 2021
Based on feedbacks from previous maintainer remove unecessary
code to manage faulty root directory recordings.
Ref: SynoCommunity#4545 (comment)
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.

I now recall where we left...
Basically the PR was ready, besides this.

Issue is on DSM7: the following files gets installed in /var/packages/tvheadend/target/var while all the tvheadend tree ends-up going under /var/package/tvheadend/var.

As such, it doesn't work as the web server can't find the access and passwd files.

We need #4579 (or any other viable solution) to solve this.

publicarray and others added 15 commits June 15, 2021 19:40
fix ffmpeg (libaom) in newer gcc versions

opencv/opencv#10032
pkgverify.cpp:225 Failed to load pkg info, version=[4.3.20201011~9ed76c0-26], correct-format=[^\d+(\.\d+){0,5}(-\d+)?$]
pkgverify.cpp:278 Failed to verify package, spk=[/volume1/@tmp/upload_tmp.122880] result=[{"action":"prepare","beta":false,"betaIncoming":false,"error":{"code":261,"description":"invalid package info content"},"installReboot":false,"package":"tvheadend","packageName":"Tvheadend","stage":"prepare","success":false}]
Based on feedbacks from previous maintainer remove unecessary
code to manage faulty root directory recordings.
Ref: SynoCommunity#4545 (comment)
@th0ma7
Copy link
Contributor

th0ma7 commented Jun 16, 2021

As this solves all build errors I recommend moving ahead and to merge it.
Unit testing is looking good and I'm planning for an upgrade of both ffmpeg (#4576) and tvheadend (#4680 + another coming shortly) anyway so additional testing will take place.
Work on #4579 is not mandatory for this portion to get merged to master.

@hgy59 and/or @ymartin59 are you ok with this PR?

This was referenced Jun 17, 2021
@th0ma7 th0ma7 merged commit 1b293c2 into SynoCommunity:master Jun 19, 2021
@publicarray publicarray deleted the tvheaded-ffmpeg branch July 24, 2021 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants