Bug 2512679

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.

> # To generate this list, go into %%{buildroot}%%{chromium_path} and run
> # for i in `find . -name "*.so" | sort`; do NAME=`basename -s .so $i`; printf "$NAME|"; done
> # for RHEL7, append libfontconfig to the end
> # make sure there is not a trailing | at the end of the list
> # We always filter provides. We only filter Requires when building shared.
> %global __provides_exclude_from ^(%{chromium_path}/.*\\.so|%{chromium_path}/.*\\.so.*)$
> %global __requires_exclude ^(%{chromium_path}/.*\\.so|%{chromium_path}/.*\\.so.*)$

This needs to be updated to use "%{electron_path}". Nothing is installed in %chromium_path and it's otherwise unused.

> ## ELECTRON: overlay source (extracted on top of Chromium in %prep)
> [...]
> # RPM macros for building electron native module packages (installed in %install)

You either need to escape the macros or use %dnl instead for the comment lines.

Updated again. Thanks!

I see no updates.

My mistake. I actually have two copies of this git repo on codeberg and pushed to the wrong one. You should see the updates now.

(I've also offered a couple of changes related to the source archive prep scripts in this repo to the chromium package in a PR.)

Review notes:

* Package follows Fedora Packaging Guidelines (or at least looks similar enough to chromium and cef)
* Package licensing is (reasonably) correct and license files are captured
* Package builds and installs
* Nothing serious stands out at this point

It seems to be in a "reasonable enough" state. Let's get this in.

PACKAGE APPROVED.

The Pagure repository was created at https://src.fedoraproject.org/rpms/nodejs-electron
Monitoring:
Wasn't able to create project in Anitya. You can create it manually on: `https://release-monitoring.org`

FEDORA-2026-831312a8d8 (nodejs-electron-43.2.0-1.fc46) has been submitted as an update to Fedora 46.
https://bodhi.fedoraproject.org/updates/FEDORA-2026-831312a8d8

FEDORA-2026-831312a8d8 (nodejs-electron-43.2.0-1.fc46) has been pushed to the Fedora 46 stable repository.
If problem still persists, please make note of it in this bug report.