https://bugzilla.redhat.com/show_bug.cgi?id=2328456
--- Comment #9 from Fabio Porcedda <[email protected]> --- (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. -- 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%23c9 -- _______________________________________________ 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
