https://bugzilla.redhat.com/show_bug.cgi?id=2369466 --- Comment #23 from Jan Staněk <jstanek@xxxxxxxxxx> --- (In reply to TomasJuhasz from comment #17) > Few small rpmlint complains: > 1.) Non-executable npm scripts. > > nodejs24-npm.noarch: E: non-executable-script /usr/lib/node_modules_24/npm/lib/utils/completion.sh 644 /bin/bash > and other javascript/python files on path: > > /usr/lib/node_modules_24/npm/ > > Caused by this block (line 411): > ># === NPM installation and tweaks > ># Correct permissions in provided scripts > ># - There are executable scripts for Windows PowerShell; RPM would try to pull it as a dependency > ># - Not all executable bits should be removed; the -not -path lines are the ones that will be kept untouched > >declare NPM_DIR="${RPM_BUILD_ROOT}%{nodejs_common_sitelib}/npm" > >find "${NPM_DIR}" \ > > -not -path "${NPM_DIR}/bin/*" \ > > -not -path "${NPM_DIR}/node_modules/node-gyp/bin/node-gyp.js" \ > > -not -path "${NPM_DIR}/node_modules/@npmcli/run-script/lib/node-gyp-bin/node-gyp" \ > > -type f -executable \ > > -execdir chmod -x '{}' + > > Used to remove permissions from power-shell scripts (e.g. > /usr/lib/node_modules_24/npm/bin/npm.ps1), but it's probably not necessary > as these already don't have execute privileges. Experimenting with the removal now. I'll check what interpreters it will pull in addition of what is already required. As bunch of those scripts just happen to be shipped in the bundled dependencies and never intended to be run by the end user, we might end removing them anyway and just ignore rpmlint here. > 2.) Spelling of ICU > >nodejs24-full-i18n.x86_64: E: spelling-error ('icu', '%description -l en_US icu -> ICU, icy, Cu') Will be fixed (`full-icu support` -> `full ICU support`). > 3.) Empty roadmap file > >nodejs24-npm.noarch: E: zero-length /usr/lib/node_modules_24/npm/node_modules/smart-buffer/docs/ROADMAP.md > (Empty file created 7 years ago and never used - maybe eventually it could > be stripped during creation of tarball, but probably not a high priority.) Generally, I would ignore this sort of warnings from the bundled dependencies – they are far from perfect, but can change from release to release, and they are usually not worth worrying about. (In reply to Jarek Prokop from comment #21) > So far the biggest problem is that scratch-build in Fedora Koji fails: > > It seems that the current state is: macro fails, the %if further down only > has that macro, if it is undefined in this case, the build fails. > > > https://koji.fedoraproject.org/koji/taskinfo?taskID=134584043 mock_output.log Cannot open the link right now (yay infra migration!), so bit of a guesswork will be included. > ~~~ > sh: line 1: hexdump: command not found > > error: unexpected end of expression: > > error: ^ > error: /chroot_tmpdir/srpm_unpacked/SPECS/nodejs24.spec:400: bad %if > condition: > ~~~ This is very weird – hexdump should be included in util-linux rpm, and both util-linux and gawk are part of the buildsys-build group (according to my laptop), so they should be installed in the buildroot by default and we should not be needing them listed as BuildRequires. Our COPR builds provide them by default, so I'm very confused why koji would not. > 2 notes for this: > ~~~ > BuildRequires: %{_bindir}/hexdump > BuildRequires: %{_bindir}/awk > BuildRequires: %{_bindir}/grep > ~~~ > should prevent this. Guessing, but it won't. The `%()` construct will attempt to resolve at spec parse time, so even before the BuildRequires can be acted upon. Again, not sure why those packages won't be there automatically. > I have used that approach here: > https://koji.fedoraproject.org/koji/taskinfo?taskID=134584200 > and the following in tandem: > > Prepending 0 to prevent syntactical errors when it fails: > ~~~ > %if 0%{little_endian} > ~~~ That is a workaround, and in this case, I would rather get a syntax error than just continuing assuming big-endian system. > And lastly, the echo uses `-N`, seems the intent was `-n`?: > ~~~ > %global little_endian %(echo -n I|hexdump -o|awk '{print substr($2,6,1); > exit}') > ~~~ > otherwise the echo echoes out too much. That actually is a typo, thanks! > Though currently I am wondering whether we could grab the 1 or 0 from lua > with some magic... Did not find anything. Previously, python was used to extract the value (`1 if sys.byteorder == "little" else 0`), which was more readable, but required python to be installed in the buildroot beforehand. Not sure how or if that worked. Unfortunately, I did not find anything similar in lua, but perhaps you will have a better luck. > https://koji.fedoraproject.org/koji/taskinfo?taskID=134584199 failed on > ppc64le and i686, the ppc64le seem to be around some builtin SIMD ppc > functions: `__builtin_vsx_xvcvspuxds`, or maybe something with ICU data > maybe. If you run it with the changes you mentioned above, than you might very well ended with big-endian data on little-endian system, in which case I would expect problems. This entire issue makes me think that maybe we could be fine with having larger full-i18n-subpackage and just install both variants of the database on any system. Granted, it would meant always wasting 32MiB of space by providing a file that would never be used natively… WDYT? > > License: > > Having enumerated which files have which license would be nice-to-have. As far as I know, nobody knows, even in upstream. The best effort is to have a look into the LICENSE file, which should actually be an assembly of the various licenses extracted from the sources (https://github.com/nodejs/node/blob/main/tools/license-builder.sh). > > Macro consistency > > Openssl has a version requirement in macro so it is later used as > `pkgconfig(openssl) >= %{openssl_minimal_version}` while compilers use > explicit versions: `BuildRequires: gcc >= 10.0, gcc-c++ >= 10.0, pkgconf, > ninja-build` %{gcc_minimal_version} could be defined together with the > openssl's minimal version. Good point. I'll review the usage of the openssl_minimal_version macro (again, inherited from the previous spec version), and either introduce the gcc equivalent, or get rid of it, depending on on how many places it is actually used and how much readability it adds. > v8 vs v8-devel Because that was the case in previous NodeJS streams and I did not want to touch it. The v8-devel package is actually a versioned one (e.g. v8-13.6-devel), perhaps to be able to use versions from multiple streams in parallel… I'm not sure. This is perhaps something we can investigate in the future. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2369466 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202369466%23c23 -- _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
