Remove chown -R root:root from specfile

[brianjmurrell]

Changes
%install is run as non-root and as such, chown root is not allowed to be done it.

The correct way of achieving a setting of ownership in RPM specfiles is to do it
in the %files section(s) (as it is already done).

Signed-off-by: Brian J. Murrell [email protected]

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.6% Duplication

[brianjmurrell]

Can this be reviewed and landed? This preventing this project from being able to build into an RPM.

[joshuaboniface]

I think this is causing the CentOS build to fail on this:

Executing(%***): /bin/sh -e /var/tmp/rpm-tmp.jc03oK
+ umask 022
+ cd /root/rpm***/BUILD
+ cd jellyfin-web-10.8.0
+ exit 0
Executing(%install): /bin/sh -e /var/tmp/rpm-tmp.TAAJAf
+ umask 022
+ cd /root/rpm***/BUILD
+ '[' /root/rpm***/BUILDROOT/jellyfin-web-10.8.0-1.el7.x86_64 '!=' / ']'
+ rm -rf /root/rpm***/BUILDROOT/jellyfin-web-10.8.0-1.el7.x86_64
++ dirname /root/rpm***/BUILDROOT/jellyfin-web-10.8.0-1.el7.x86_64
+ mkdir -p /root/rpm***/BUILDROOT
+ mkdir /root/rpm***/BUILDROOT/jellyfin-web-10.8.0-1.el7.x86_64
+ cd jellyfin-web-10.8.0
+ npm ci --no-audit --unsafe-perm
npm ERR! Error while executing:
npm ERR! /usr/bin/git ls-remote -h -t https://github.com/jellyfin/JavascriptSubtitlesOctopus.git
npm ERR! 
npm ERR! fatal: Cannot change to '/root/rpm***/..': Permission denied
npm ERR! 
npm ERR! exited with error code: 128

npm ERR! A complete log of this run can be found in:
npm ERR!     /root/.npm/_logs/2021-12-11T19_27_11_051Z-debug.log

[brianjmurrell]

I think this is causing the CentOS build to fail on this:

Well, before the PR the CentOS build should have been failing anyway, as the #1 golden rule of building RPMs (just like building any other piece of software) is to never, ever build RPMs as root. There is no properly built build system in the world that builds RPMs as root, so trying to do anything that actually requires root permissions in an RPM specfile is will be broken and will not build on any such properly built build system.

So the answer here is absolutely not to revert this change as that is just substituting one flavour of broken for another.

That said, what does not make any sense is fatal: Cannot change to '/root/rpm***/..': Permission denied and yet just a few commands before that:

 + mkdir -p /root/rpm***/BUILDROOT
 + mkdir /root/rpm***/BUILDROOT/jellyfin-web-10.8.0-1.el7.x86_64

was successful.

Here is what running the %install of this RPM build looks like when it is done as a non-root user (from this build log on Copr):

Executing(%install): /bin/sh -e /var/tmp/rpm-tmp.OCT6qU
+ umask 022
+ cd /builddir/build/BUILD
+ '[' /builddir/build/BUILDROOT/jellyfin-web-10.8.0-1.fc35.x86_64 '!=' / ']'
+ rm -rf /builddir/build/BUILDROOT/jellyfin-web-10.8.0-1.fc35.x86_64
++ dirname /builddir/build/BUILDROOT/jellyfin-web-10.8.0-1.fc35.x86_64
+ mkdir -p /builddir/build/BUILDROOT
+ mkdir /builddir/build/BUILDROOT/jellyfin-web-10.8.0-1.fc35.x86_64
+ cd jellyfin-web-10.8.0
+ npm ci --no-audit --unsafe-perm
npm WARN old lockfile 
npm WARN old lockfile The package-lock.json file was created with an old version of npm,
npm WARN old lockfile so supplemental metadata must be fetched from the registry.
npm WARN old lockfile 
npm WARN old lockfile This is a one-time fix-up, please be patient...
npm WARN old lockfile 
npm WARN deprecated [email protected]: Please see https://github.com/lydell/urix#deprecated
npm WARN deprecated [email protected]: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.
npm WARN deprecated [email protected]: https://github.com/lydell/resolve-url#deprecated
npm WARN deprecated [email protected]: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
npm WARN deprecated [email protected]: flatten is deprecated in favor of utility frameworks such as lodash.
npm WARN deprecated @types/[email protected]: This is a stub types definition. vfile-message provides its own type definitions, so you do not need this installed.
npm WARN deprecated @types/[email protected]: This is a stub types definition for localforage (https://github.com/localForage/localForage). localforage provides its own type definitions, so you don't need @types/localforage installed!
npm WARN deprecated [email protected]: Browserslist 2 could fail on reading Browserslist >3.0 config used in other tools.
> [email protected] prepare
> node ./scripts/prepare.js
Browserslist: caniuse-lite is outdated. Please run:
npx browserslist@latest --update-db
Why you should do it regularly:
https://github.com/browserslist/browserslist#browsers-data-updating
assets by path *.woff2 27.4 MiB 901 assets
assets by path *.js 10.4 MiB 15 assets
assets by path assets/ 732 KiB 43 assets
assets by path *.png 684 KiB 30 assets
assets by path libraries/ 7.53 MiB 11 assets
assets by path themes/ 1.15 MiB 8 assets
assets by path *.json 2.54 KiB 3 assets
assets by path *.ico 13.3 KiB 2 assets
assets by path *.gif 3.99 KiB
  asset 74ce2b743c33a4197e5c.gif 2.56 KiB [emitted] [immutable] [from: assets/img/equalizer.gif] (auxiliary name: playback-queue)
  asset f3bc149017432b87da2e.gif 1.43 KiB [emitted] [immutable] [from: ../node_modules/jstree/dist/themes/default/throbber.gif]
assets by path *.svg 2.2 KiB
  asset cb6e840e08726299bf8f.svg 1.39 KiB [emitted] [immutable] [from: assets/img/fresh.svg]
  asset 0d2b37694d352e7e4c59.svg 833 bytes [emitted] [immutable] [from: assets/img/rotten.svg]
6 assets
orphan modules 2.88 MiB [orphan] 972 modules
runtime modules 22.8 KiB 12 modules
cacheable modules 14 MiB (javascript) 28 MiB (asset)
  modules by path ../node_modules/ 5.52 MiB (javascript) 28 MiB (asset) 1430 modules
  modules by path ./ 8.48 MiB (javascript) 4.76 KiB (asset) 446 modules
  modules by path data:image/ 1.22 KiB
    modules by path data:image/gif;base64,R0lGODlhCwAHAIAAACgoKP/// 218 bytes 2 modules
modules by path ./controllers/ lazy ^/.// 320 bytes
  ./controllers/ lazy ^\.\/.*$ chunkName: [request] namespace object 160 bytes [built] [code generated]
  ./controllers/ lazy ^\.\/.*$ namespace object 160 bytes [built] [code generated]
modules by path ./components/tvproviders/ lazy ^/.// 320 bytes
  ./components/tvproviders/ lazy ^\.\/.*$ namespace object 160 bytes [built] [code generated]
  ./components/tvproviders/ lazy ^\.\/.*\.template\.html$ namespace object 160 bytes [built] [code generated]
7 modules
LOG from InjectManifest
<i> The service worker at serviceworker.js will precache
<i>         1357 URLs, totaling 47.7 MB.
WARNING in libraries/subtitles-octopus-worker-legacy.js is 3.01 MB, and won't be precached. Configure maximumFileSizeToCacheInBytes to change this limit.
WARNING in asset size limit: The following asset(s) exceed the recommended size limit (244 KiB).
This can impact web performance.
Assets: 
  main.jellyfin.bundle.js (1.16 MiB)
  194d1b9ef54d0014b3c4.ttf (311 KiB)
  themes/blueradiance/bg.jpg (933 KiB)
  libraries/subtitles-octopus-worker.js (303 KiB)
  libraries/subtitles-octopus-worker-legacy.js.mem (627 KiB)
  libraries/subtitles-octopus-worker.wasm (1.85 MiB)
  libraries/subtitles-octopus-worker-legacy.js (2.87 MiB)
  libraries/pdf.worker.js (641 KiB)
  libraries/wasm-gen/libarchive.wasm (875 KiB)
  4695.07552e330c5c873cdd7c.chunk.js (783 KiB)
  5181.b91688ccf987a0041751.chunk.js (342 KiB)
  5487.77437949ec822cd7dbb3.chunk.js (332 KiB)
WARNING in entrypoint size limit: The following entrypoint(s) combined asset size exceeds the recommended limit (244 KiB). This can impact web performance.
Entrypoints:
  main (1.16 MiB)
      main.jellyfin.bundle.js
webpack 5.64.0 compiled with 3 warnings in 150220 ms
added 1850 packages in 4m
npm notice 
npm notice New minor version of npm available! 8.1.2 -> 8.3.0
npm notice Changelog: <https://github.com/npm/cli/releases/tag/v8.3.0>
npm notice Run `npm install -g [email protected]` to update!
npm notice 
+ /usr/bin/mkdir -p /builddir/build/BUILDROOT/jellyfin-web-10.8.0-1.fc35.x86_64/usr/share
+ mv dist /builddir/build/BUILDROOT/jellyfin-web-10.8.0-1.fc35.x86_64/usr/share/jellyfin-web
+ /usr/bin/install -D -m 0644 LICENSE /builddir/build/BUILDROOT/jellyfin-web-10.8.0-1.fc35.x86_64/usr/share/licenses/jellyfin/LICENSE

So my first suggestion to working this out would be to stop building the RPMs as root and see if the problem persists.

[joshuaboniface]

Well, Azure is building these, not me. Here's the build log: https://dev.azure.com/jellyfin-project/jellyfin/_build/results?buildId=34942&view=logs&j=5404562b-245d-5392-77db-b338601c3067&t=94e8c372-eb87-5491-597a-f5fcde0ab6d6 As far as I can tell, it is not running as root, and we have nothing in the CI config that should cause it to run as root. Worth mentioning that any time you see *** it should be build. Azure censors this for some reason.

[brianjmurrell]

Right. But Azure it building them per the supplied Dockerfile. It's all of the activity in /root/ that happens in %install that leads me to believe that the RPM building, in the docker image, is actually being done as root. I'm no Docker expert, but I think you need to insert a USER statement to downgrade your userid from root to a non-root user in a Dockerfile, which you should do before you start running rpmbuild. You might need to do a useradd before that to create an account to can USER to.

[mueslimak3r]

Right. But Azure it building them per the supplied Dockerfile. It's all of the activity in /root/ that happens in %install that leads me to believe that the RPM building, in the docker image, is actually being done as root. I'm no Docker expert, but I think you need to insert a USER statement to downgrade your userid from root to a non-root user in a Dockerfile, which you should do before you start running rpmbuild. You might need to do a useradd before that to create an account to can USER to.

Maybe you could do chown -R $(id -u):$(id -g) . instead and avoid the whole thing

[thornbill]

Web builds for centos and now fedora have been broken in CI for a month now. Can we get some commitment to fixing this @brianjmurrell? This change is responsible for completely breaking our unstable build pipeline as well as the 10.8 alphas.

[brianjmurrell]

This change is responsible for completely breaking our unstable build pipeline as well as the 10.8 alphas

@thornbill I disagree. The breakage in CI is not really the fault of this PR. This PR fixed brokenness in the jellyfin.spec file. Anybody who even knows remotely anything about RPM building will tell you that RPM scriptlets are not allowed to assume they can do things that require root privileges as it's very (very!) well accepted in the RPM building ecosystem that you DO NOT build RPMs as root. That in fact is exactly why the [def]attr knobs are available in the %files section -- to set the ownership and permissions of the installed files because it cannot be done in %install because %install is supposed to be run without root privileges.

That the CI build was depending on the jellyfin.spec brokenness, is the real cause of the existing CI breakage. I have not really looked at how the RPMs are being built, but I suspect they are being built as root. Fix that, and you will probably fix your CI issue.

I will see if I can take a look at some point (hopefully soon), but the unfortunate reality is that $day_job, $family and $life almost always take priority over hacking.

Do PRs normally do RPM builds? I didn't see any failures in this PR. Would any PR reproduce the build failure you are seeing? Can you point to a recent example of a CI build failing?

[thornbill]

Look at the Azure build of every commit to master since this was merged for examples of failures.

Frankly I don’t care if the rpm spec was technically incorrect previously. I do care that all unstable builds of web have been failing for a month due to this.

[brianjmurrell]

I found https://dev.azure.com/jellyfin-project/jellyfin/_build/results?buildId=35141&view=logs&j=b8f1083d-0e74-517d-1ebe-6648c20a9fa9&t=1744f064-bb38-5491-66a4-58fbbd5263c6. I assume that's the kind of breakage to which you are referring.

If so, that has nothing to do with this PR and looks more related to the last two commits to https://github.com/jellyfin/jellyfin-web/commits/master/fedora/Makefile.

TBH, it seems really odd to replace the proper git-tag derived package version with date-stamp-type version

jellyfin-web/deployment/build.fedora

Line 17 in 9515733

sed -i "s/Version:.*/Version: ${BUILD_ID}/" jellyfin-web.spec

. Usually one embeds datestamps or git hashes into the Release: so that the Version: maintains some semblance of the real version the nightly/unstable build is based on.

But if that is really what you want to do, I can see if I can make those last two commits compatible with that.

I'm still puzzled why the PRs that tested those two commits did not fail these package build stages. Are they not run for PRs?

[brianjmurrell]

Frankly I don’t care if the rpm spec was technically incorrect previously. I do care that all unstable builds of web have been failing for a month due to this.

But again, this PR is not the reason for the failures and again do PRs not do the package building tests in CI?

[brianjmurrell]

I see the problem(s). Working on a fix. But if PRs don't build packages, how will we know it actually fixes the problem?

[brianjmurrell]

And now we are back to the original problem the specfile was broken to fix:

+ npm ci --no-audit --unsafe-perm
npm ERR! Error while executing:
npm ERR! /usr/bin/git ls-remote -h -t https://github.com/jellyfin/JavascriptSubtitlesOctopus.git
npm ERR! 
npm ERR! fatal: Cannot change to '/root/rpmbuild/..': Permission denied
npm ERR! 
npm ERR! exited with error code: 128

npm ERR! A complete log of this run can be found in:
npm ERR!     /root/.npm/_logs/2021-12-23T20_39_16_702Z-debug.log
+ id
uid=0(root) gid=0(root) groups=0(root)
+ echo /root
+ ls -l /root
/root
total 8
-rw------- 1 root root 3416 Nov 13  2020 anaconda-ks.cfg
drwxr-xr-x 8 root root 4096 Dec 23 20:39 rpmbuild
+ touch /root/foo

But it makes no sense at all. id says the user is root. ls -l /root shows that /root/rpmbuild/ does exist and has the correct permissions for root to change to it. Assuming that means chdir.

So what is so funky about this environment that would cause such contradictions?