Fedora Account System
Red Hat Associate
Red Hat Customer
Spec URL: https://michael.cronenworth.com/RPMS/node-gyp.spec SRPM URL: https://michael.cronenworth.com/RPMS/node-gyp-10.2.0-0.1.fc41.src.rpm Description: Node.js native addon build tool Fedora Account System Username: mooninite This review is to unretire the package. The 'zwave-js-ui' package requires this package. I will be opening a separate package review for 'zwave-js-ui' soon.
Copr build: https://copr.fedorainfracloud.org/coprs/build/8305692 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2328456-node-gyp/fedora-rawhide-x86_64/08305692-node-gyp/fedora-review/review.txt Found issues: - License file LICENSE.APACHE is not marked as %license Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text - A package with this name already exists. Please check https://src.fedoraproject.org/rpms/node-gyp Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
This is an automatic check from review-stats script. This review request ticket hasn't been updated for some time. We're sorry it is taking so long. If you're still interested in packaging this software into Fedora repositories, please respond to this comment clearing the NEEDINFO flag. You may want to update the specfile and the src.rpm to the latest version available and to propose a review swap on Fedora devel mailing list to increase chances to have your package reviewed. If this is your first package and you need a sponsor, you may want to post some informal reviews. Read more at https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group. Without any reply, this request will shortly be considered abandoned and will be closed. Thank you for your patience.
I'd still like to get this in. I recognize I need to update to the latest version.
Spec URL: https://michael.cronenworth.com/RPMS/node-gyp.spec SRPM URL: https://michael.cronenworth.com/RPMS/node-gyp-12.2.0-0.1.fc44.src.rpm Updated to the latest release.
Are you interested to swap the package review with another nodejs package (bug 2511720)?
Sure! I'll update the spec and srpm since it's been almost 8 months. Spec URL: https://michael.cronenworth.com/RPMS/node-gyp.spec SRPM URL: https://michael.cronenworth.com/RPMS/node-gyp-13.0.1-0.1.fc46.src.rpm
(In reply to Michael Cronenworth from comment #6) > Sure! I'll update the spec and srpm since it's been almost 8 months. > > Spec URL: https://michael.cronenworth.com/RPMS/node-gyp.spec > SRPM URL: > https://michael.cronenworth.com/RPMS/node-gyp-13.0.1-0.1.fc46.src.rpm Issues: - Add a comment about the upstream status of the patch, even something like "not sent upstream because is specific for Fedora" - Use a better description than the summary, for example the first part of the project description or somethink like that: "node-gyp is a cross-platform command-line tool written in Node.js for compiling native addon modules for Node.js. It contains a vendored copy of the gyp-next project that was previously used by the Chromium team and extended to support the development of Node.js native addons." - Release should be >= 1 or use %autorelease - Because the system gyp is being used instead of the bundled one, remove it in %prep instead of copying it in %install "cp -rp package.json addon.gypi addon-rpm.gypi bin/ gyp/ lib/ %{buildroot}%{nodejs_sitelib}/%{name}" - upstream dependency is "node": "^22.22.2 || ^24.15.0 || >=26.0.0" but the Fedora package has an unversioned dependency, it should exclude nodejs20, build.log: WARNING: The nodejs(engine) dependency contains an OR (||) dependency: '^22.22.2 || ^24.15.0 || >=26.0.0. - In line 31 is being used an undefined macro that will be put literally in the final rpm, does it make sense? - Source2 is being included but not used, there are some tests that can be used in %check instead? Suggestions: - Use %autorelease and %autochangelog
(In reply to Fabio Porcedda from comment #7) > Issues: > - Add a comment about the upstream status of the patch, even something like > "not sent upstream because is specific for Fedora" > - Use a better description than the summary, for example the first part of > the project description or somethink like that: > "node-gyp is a cross-platform command-line tool written in Node.js for > compiling native addon modules for Node.js. It contains a vendored copy of > the gyp-next project that was previously used by the Chromium team and > extended to support the development of Node.js native addons." Changed. > - Release should be >= 1 or use %autorelease It has been my style for as long as I have packaged to use 0.x for package reviews and use '1' for post-review, package import. I'll change it now. > - Because the system gyp is being used instead of the bundled one, remove it > in %prep instead of copying it in %install "cp -rp package.json addon.gypi > addon-rpm.gypi bin/ gyp/ lib/ %{buildroot}%{nodejs_sitelib}/%{name}" > - upstream dependency is "node": "^22.22.2 || ^24.15.0 || >=26.0.0" but the > Fedora package has an unversioned dependency, it should exclude nodejs20, > build.log: > WARNING: The nodejs(engine) dependency contains an OR (||) dependency: > '^22.22.2 || ^24.15.0 || >=26.0.0. Thanks! I am not a NodeJS developer or packager. > - In line 31 is being used an undefined macro that will be put literally in > the final rpm, does it make sense? This review is so old I do not recall where the macro originated. It'll be fixed. > - Source2 is being included but not used, there are some tests that can be > used in %check instead? The test files are not included in the source tarball. Should the -dev.tgz be excluded? > Suggestions: > - Use %autorelease and %autochangelog I prefer manual methods. They are still permitted. I'll post a new spec once the SOURCE2 issue is resolved.
(In reply to Michael Cronenworth from comment #8) > (In reply to Fabio Porcedda from comment #7) > > Issues: > > - Add a comment about the upstream status of the patch, even something like > > "not sent upstream because is specific for Fedora" > > - Use a better description than the summary, for example the first part of > > the project description or somethink like that: > > "node-gyp is a cross-platform command-line tool written in Node.js for > > compiling native addon modules for Node.js. It contains a vendored copy of > > the gyp-next project that was previously used by the Chromium team and > > extended to support the development of Node.js native addons." > > Changed. > > > - Release should be >= 1 or use %autorelease > > It has been my style for as long as I have packaged to use 0.x for package > reviews and use '1' for post-review, package import. I'll change it now. > > > - Because the system gyp is being used instead of the bundled one, remove it > > in %prep instead of copying it in %install "cp -rp package.json addon.gypi > > addon-rpm.gypi bin/ gyp/ lib/ %{buildroot}%{nodejs_sitelib}/%{name}" > > - upstream dependency is "node": "^22.22.2 || ^24.15.0 || >=26.0.0" but the > > Fedora package has an unversioned dependency, it should exclude nodejs20, > > build.log: > > WARNING: The nodejs(engine) dependency contains an OR (||) dependency: > > '^22.22.2 || ^24.15.0 || >=26.0.0. > > Thanks! I am not a NodeJS developer or packager. > > > - In line 31 is being used an undefined macro that will be put literally in > > the final rpm, does it make sense? > > This review is so old I do not recall where the macro originated. It'll be > fixed. > > > - Source2 is being included but not used, there are some tests that can be > > used in %check instead? > > The test files are not included in the source tarball. Should the -dev.tgz > be excluded? The tests are not included in the source tarball retrieved by nodejs-packaging-bundler, but they are present in the GitHub source tarball. So, in order to run the tests, you can use the GitHub source tarball. You can choose to use the GitHub source tarball and run the tests, or instead exclude the -dev.tgz. In my case, for the markdownlint-cli2 package, I had the same issue, and I've chosen to use the GitHub source tarball in order to run the tests. The Fedora guidelines just say that the test suite SHOULD be executed, so it’s not a MUST—it’s up to you. > > Suggestions: > > - Use %autorelease and %autochangelog > > I prefer manual methods. They are still permitted. I'll post a new spec once > the SOURCE2 issue is resolved.
Spec URL: https://michael.cronenworth.com/RPMS/node-gyp.spec SRPM URL: https://michael.cronenworth.com/RPMS/node-gyp-13.0.1-1.fc46.src.rpm - I 'hard coded' nodejs 24 as the configure.js link. I'm unsure if this is safe or not. - I had to disable two tests that required network access. Other tests passed.
(In reply to Michael Cronenworth from comment #10) > Spec URL: https://michael.cronenworth.com/RPMS/node-gyp.spec > SRPM URL: https://michael.cronenworth.com/RPMS/node-gyp-13.0.1-1.fc46.src.rpm > > - I 'hard coded' nodejs 24 as the configure.js link. I'm unsure if this is > safe or not. Why exclude nodejs26? I think "Requires/BuildRequires: (nodejs24-devel or nodejs26-devel)" should be better. > - I had to disable two tests that required network access. Other tests > passed. Issue found: - Please add for each patch the upstream status - Because you are using the system gyp package you must remove in %prep the bundled version of gyp (rm -r gyp) - In the %description the reference to the bundled gyp-next should be removed because you use the system gyp - "Requires: nodejs(engine) > 20" still accept nodejs20, to fix that use "Requires: (nodejs24-devel or nodejs26-devel)"
Issues addressed. Spec URL: https://michael.cronenworth.com/RPMS/node-gyp.spec SRPM URL: https://michael.cronenworth.com/RPMS/node-gyp-13.0.1-1.fc46.src.rpm
Created attachment 2155561 [details] The .spec file difference from Copr build 8305692 to 10900219
Copr build: https://copr.fedorainfracloud.org/coprs/build/10900219 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2328456-node-gyp/fedora-rawhide-x86_64/10900219-node-gyp/fedora-review/review.txt Found issues: - License file LICENSE.md is not marked as %license Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text - A package with this name already exists. Please check https://src.fedoraproject.org/rpms/node-gyp Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
(In reply to Michael Cronenworth from comment #12) > Issues addressed. > > Spec URL: https://michael.cronenworth.com/RPMS/node-gyp.spec > SRPM URL: https://michael.cronenworth.com/RPMS/node-gyp-13.0.1-1.fc46.src.rpm Hi, when node is not already installed on the system, rpmspec emits: sh: line 1: node: command not found because the command is executed while parsing the spec instead of during %prep. Also the association to the nodejs-xx version is static. For example, if the package is built with nodejs24 associated, but at runtime only nodejs22 is available because it's the only one installed: dnf install node-gyp nodejs22 node-gyp installs fine, but when executed it tries to use nodejs24, while only nodejs22 is available. I think the best solution is to detect at runtime which nodejs version should be used. The node-gyp-system-node-gyp.patch should be updated to do that.