Fedora Account System
Red Hat Associate
Red Hat Customer
Spec URL: https://codeberg.org/gordonmessmer/nodejs-electron/raw/branch/nodejs-electron/nodejs-electron.spec SRPM URL: https://gordonmessmer.fedorapeople.org/nodejs-electron/nodejs-electron-43.2.0-1.fc44.src.rpm Description: Build cross-platform desktop apps with JavaScript, HTML, and CSS Fedora Account System Username: gordonmessmer This package is derived from Fedora's chromium package, and includes portions of SUSE's nodejs-electron packaging (which was also published under MIT license). A diff of this package vs Fedora's chromium package is available here: https://codeberg.org/gordonmessmer/nodejs-electron/pulls/1 The intent for this package is to adopt a grafting approach similar to the cef package: https://src.fedoraproject.org/rpms/cef
Taking this review.
A build of this package will appear in COPR over the weekend, but it's going to take a day or two to build: https://copr.fedorainfracloud.org/coprs/gordonmessmer/nodejs-electron/
Copr build: https://copr.fedorainfracloud.org/coprs/build/10836965 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2512679-nodejs-electron/fedora-rawhide-x86_64/10836965-nodejs-electron/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- 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.
A build in copr succeeded: https://copr.fedorainfracloud.org/coprs/gordonmessmer/nodejs-electron/build/10837995/
I wish there were less `%{rhel}` macros, because this is supposed to be **Fedora** review and the spec is long enough even without them. But since they are there, this caught my attention: ~~~ # RHEL 9 needs newer nodejs %if 0%{?rhel} == 9 %global system_nodejs 0 %endif ~~~ What does this mean? There is Node.js 22 in RHEL9. Is the module the problem? Second question is why there is `%global nodejs_version v22.22.0`. Why not Node.js 26? It seems that there is `BuildRequires: nodejs, /usr/bin/node, /usr/bin/npm`, which means default Fedora version. Not sure if it is still Node.js 22, but this is about to change sooner than later. IOW it would be good to elaborate about the supported Node.js version (and make a note in the .spec file) and it would be good to pull in Node.js maintainers.
> I wish there were less `%{rhel}` macros I understand, but this package is derived from Fedora's chromium package, and I think it's only sustainable if we can keep the two as similar in possible. > # RHEL 9 needs newer nodejs > What does this mean? I'm not sure, the commit isn't very specific: https://src.fedoraproject.org/rpms/chromium/c/45cd37cf1ca68270ef38e33e24ae80f9d82e74a5?branch=rawhide > Second question is why there is `%global nodejs_version v22.22.0`. Why not Node.js 26? We should definitely ask Than. My guess would be that they are trying to produce a build that uses similar dependencies on as many target platforms as possible, in order to avoid bugs that have to be identified and resolved on individual targets. > Not sure if it is still Node.js 22 Yes, that's what the COPR build logs say.
> this package is derived from Fedora's chromium package, and I think it's only sustainable if we can keep the two as similar in possible. It would probably make sense to note this somewhere on the top of the .spec file, because nothing like this came to my mind seeing the review.
I've pushed a change to the git repo adding a README section (like CEF's) that explains that this spec is derived from chromium's. It also fixes the ELECTRON section tags and replaces the changelog with %autochangelog.
Initial spec review: > %package qt5-ui > Summary: Qt5 UI for Electron > Requires: nodejs-electron%{_isa} = %{version}-%{release} This also needs a "Supplements: (nodejs-electron%{_isa} and qt5-qtbase-gui%{_isa})" stanza. (This also probably needs to be added to chromium too... > %package qt6-ui > Summary: Qt6 UI for Electron > Requires: nodejs-electron%{_isa} = %{version}-%{release} This also needs a "Supplements: (nodejs-electron%{_isa} and qt6-qtbase-gui%{_isa})" stanza. (This also probably needs to be added to chromium too... > cp /dev/stdin %{buildroot}%{_rpmmacrodir}/macros.electron <<"MACROEOF" This should be an extra source file rather than inlined like this. It's too fragile as-is.
Updated and pushed to codeberg. Thanks, Neal.