https://bugzilla.redhat.com/show_bug.cgi?id=2328456
Michael Cronenworth <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED --- Comment #8 from Michael Cronenworth <[email protected]> --- (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. -- 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=2328456 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202328456%23c8 -- _______________________________________________ package-review mailing list -- [email protected] To unsubscribe send an email to [email protected] 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/[email protected] Do not reply to spam, report it: https://forge.fedoraproject.org/infra/tickets/issues/new
