Details

Reviewers

netchild
egypcio
yuri
acm
nivit
sunpoet
pizzamig
bhughes

Group Reviewers

portmgr

Commits

R11:f4ffcdfe611a: */*: use new nodejs USES flag
R11:46ce086c7130: Mk/Uses: default version for nodejs

Summary

Currently ports are manually using dependency to www/node , www/node16, ....
Often you must use the same node version for several ports, else it will cause compile or runtime problems.

This commit will add a new default node version which currently points to the last LTS version (16).

If a port depends on node, a simple:
USES= nodejs

can be added. It is also possible to define a specific version:
USES= nodejs:14

I will commit this in two commits, one added the ports .mk files and one commit modifying to ports displayed here, I just included it into one review to have the big picture here.

Test Plan

All ports are building fine, I build most ports with bulk -t some of them have stage problems or build_violations, so some tries were necessary.
Here you can find all steps to build all ports touched by this modification:
https://pkg.fechner.net/build.html?mastername=130amd64-gitlab&build=2021-11-01_22h21m44s
Next run:
https://pkg.fechner.net/build.html?mastername=130amd64-gitlab&build=2021-11-02_00h25m09s
Next run to compile some failing ports without testing:
https://pkg.fechner.net/build.html?mastername=130amd64-gitlab&build=2021-11-02_07h17m18s
Next run to continue builds with testing:
https://pkg.fechner.net/build.html?mastername=130amd64-gitlab&build=2021-11-02_07h20m16s
Next run to compile some failing ports without testing:
https://pkg.fechner.net/build.html?mastername=130amd64-gitlab&build=2021-11-02_07h35m09s
Final build with testing:
https://pkg.fechner.net/build.html?mastername=130amd64-gitlab&build=2021-11-02_07h36m51s

Diff Detail

Repository

R11 FreeBSD ports repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Removed some debug lines

mfechner edited the test plan for this revision. (Show Details)Nov 1 2021, 11:26 PM

mfechner edited the test plan for this revision. (Show Details)Nov 2 2021, 6:43 AM

Added diff for nodejs.mk

mfechner added a reviewer: bhughes.Nov 3 2021, 7:33 PM

This is a great start, and something I have been wanting to do for about 4 years now I haven't every gotten around to finishing it, unfortunately, but this review is the nudge I/we need ;)

You can see where I started in my freebsd-ports fork on Github: https://github.com/bradleythughes/freebsd-ports/commits/nodejs-USES-node. I am very eager to join our efforts and get this into the ports tree.

Mk/Uses/nodejs.mk
8	bhughes@FreeBSD.org
36–37	Some ports will only need the BUILD_DEPENDS, others only the RUN_DEPENDS, and some need both. I think it would make sense to have the ability to say which one you need, e.g. USES=nodejs:build and USES=nodejs:run, with the default being USES=nodejs:build,run.
Mk/bsd.default-versions.mk
166	What about also having version "aliases", e.g. USES=nodejs:lts or USES=nodejs:current, in addition to the major version number?
devel/node-thrift/Makefile
13 ↗	(On Diff #97846)	As commented above, this adds a build dependency that previously wasn't needed.
net/cjdns/Makefile
17 ↗	(On Diff #97846)	And here, we add a RUN_DEPENDS on a port that previously only needed Node.js to build. I think we should really work to avoid this.

This revision now requires changes to proceed.Nov 3 2021, 9:13 PM

Nice comments, I like them.
I added all of them, tests are looking good.

Marked todo points as done.

FYI, I've added www/npm-node16.

Mk/Uses/nodejs.mk
23	Why add "lts" here? It should always follow NODEJS_DEFAULT. Otherwise, it is not a default version.
Mk/bsd.default-versions.mk
167	The default is 17.
www/npm-node14/Makefile
7 ↗	(On Diff #97986)	Please change it to: USES=nodejs:14,run
www/npm/Makefile
17 ↗	(On Diff #97986)	Please change it to: USES?=nodejs:run USES+=cpe python:3.5+ shebangfix tar:xz
www/yarn/Makefile
20 ↗	(On Diff #97986)	This is no-op now.

I will prepare another diff that includes npm-node16 and all the modification, thanks a lot @sunpoet for your review!

Mk/bsd.default-versions.mk
167	I suggest to use the latest LTS version by default. If a user would like to use current which currently breaks many packages can overwrite.

All suggested changes included into this new diff.

pizzamig requested changes to this revision.Nov 8 2021, 6:16 PM

pizzamig added inline comments.

Mk/bsd.default-versions.mk
167	It can make sense to have the lts as default version, but it's a breaking change and I'm not sure if this is what we want right now. BEFORE: yarn and npm have nodejs 17.x as dependency. ad-hoc packages (-node16 or -node14) are available to provide stable dependencies. DEFAULT lts: yarn and npm have nodejs 16.x as dependency. ad-hoc packages (-node16 or -node14) are still available to provide stable dependencies. yarn and yarn-node16 are exactly the same. There is no way to have yarn with node 17.x (same for npm) I would keep the default version to current and I would postpone the discussion about nodejs default version to a later point in time

This revision now requires changes to proceed.Nov 8 2021, 6:16 PM

Dear Luca,

I do not really understand your comment. Do you use application written in node on your servers?
I have many application out and most of them do not work with node 17 yet. Many dependencies have a block for node 17 in, so you cannot install them if you use node 17.

So this review tries to unbreak node for most users. One application that is in the ports is www/gitlab-ce. It does not work anymore with node 17, but with the LTS version 16.
But I cannot just use yarn-node16 because we have other dependencies which depend on node and node16 and node cannot be installed as they install same files, so we have here a collision.

The only way to really fix this (and most of the application we have in ports) define a default version and every port is then using this default version.
If you would like to use bleeding-edge version, you can overwrite the standard version.
But a normal user that is operating a FreeBSD cluster to run node application will never use the bleeding-edge version, but always an fully supported and maintained LTS version.

And this is what I would like to archive with this review.
So we do not break node for the user, we will unbreak it, because currently it is broken and you cannot use it for many dependencies.

Hi Matthias,
I guess it's my turn to not understand your comment.
I'm not arguing that it's a good idea to have lts as default version, but I want to split this change in two patches, because it's a breaking change, and more changes are needed.

I have many application out and most of them do not work with node 17 yet. Many dependencies have a block for node 17 in, so you cannot install them if you use node 17.

I don't see the problem. If your application doesn't work with node17, you can specify USES=nodejs:lts , as you proposed, already. But if you want to use yarn AND node17, you have to rebuild your portstree.
The reason to have yarn, yarn-node16 and yarn-node14 (same goes for npm) was to avoid to build the portstree, but to choose the version of nodejs you want to use by installing the proper package. With your patch, there is no way to use node17 but to rebuild packages.

So this review tries to unbreak node for most users. One application that is in the ports is www/gitlab-ce. It does not work anymore with node 17, but with the LTS version 16.

Hence, specify USES=nodejs:16 or USES=nodejs:lts (for now, see the last comment)

But I cannot just use yarn-node16 because we have other dependencies which depend on node and node16 and node cannot be installed as they install same files, so we have here a collision.

This is what I don't understand: how can you have other dependencies which depend on node and node16? You can only one node version installed at a time.
So, you cannot to use yarn-node16 and yarn, because of conflicting dependencies. It has been built in this way on purpose to chose, with the package, which node version you want to use.

The only way to really fix this (and most of the application we have in ports) define a default version and every port is then using this default version.

If you would like to use bleeding-edge version, you can overwrite the standard version.

But a normal user that is operating a FreeBSD cluster to run node application will never use the bleeding-edge version, but always an fully supported and maintained LTS version.

I'm not disagreeing with this, but if I want to try node17 with npm or yarn (because without package manager, there is little or no use for nodejs), I have to rebuild the packages, changing the default version.
npm-* and yarn-* were introduced to avoid it.

And this is what I would like to archive with this review.

So we do not break node for the user, we will unbreak it, because currently it is broken and you cannot use it for many dependencies.

IMO, the long-term solution, is to have a node-devel port for the version in development, node for the latest lts, and nodeXX for the older lts, as many other ports.
What I'm saying, here is to focus this patch on introducing USES=node and move the discussion of default version in a following review.

Maybe, to ease the process, yarn-lts and npm-lts can be introduced...

Dear Luca,

ok, I think I understood what you want.
At first (as already noted) I will split this commit into two, currently they are here:
https://gitlab.fechner.net/mfechner/Gitlab/-/commits/nodejs_default_version

Yes we should make yarn available with each version combination without forcing the user to compile anything. I suggest we make this possible by just adding www/yarn-node17:

# Created by: Luca Pizzamiglio <pizzamig@FreeBSD.org>

PKGNAMESUFFIX=  -node17

CONFLICTS_INSTALL=      yarn

USES=           nodejs:17,run

MASTERDIR=      ${.CURDIR}/../yarn

.include "${MASTERDIR}/Makefile"

I will also prepare an entry in UPDATING to explain there the change in a way a normal user will understand it.
I suggest to also add npm-node17 to be consitent.

I will update the commits and the attached diff here.

Added UPDATING entry, and new port www/npm-node17 and www/yarn-node17.

tcberner added a subscriber: tcberner.Nov 10 2021, 5:59 AM

tcberner added inline comments.

Mk/Uses/nodejs.mk
26	why not loop over `_VALID_NODEJS_VERSION` ?
54	^ nit pick: from the usage and wrt to for example line 45, it would likely more sense to call it `_NODEJS_VERSION_SUFFIX`
60	^ why not match `build` and `run`? . if ${nodejs_ARGS:Mrun} RUN_DEPENDS+=.... . endif . if ${nodejs_ARGS:Mbuild} BUILD_DEPENDS+=.... . endif instead of the combinatorial explosion :D
64	^ is that case even needed?

I will do some tests and will provide a new diff later.

Mk/Uses/nodejs.mk
54	Good idea, is changed.
60	We can change this, but the consequence is, if you define USES=nodejs,14 it will not work, so you must define USES=nodejs,14,run,build I think it is a valid requirement, I will update the diff.
64	as I define in line 22 now a default, this is not required anymore, will update in next diff.
Mk/bsd.default-versions.mk
167	It will not fix gitlab-ce if we do not change the default version for node, reason: gitlab-ce depends on www/yarn-node16 gitlab-ce depends on devel/rubygem-execjs I need yarn-node16 as gitlab will not work with node 17. yarn-node16 depends www/node16 rubygem-execjs depends on www/node So rubygem-execjs and yarn-node16 have now a conflict as you cannot install node16 and node at the same time. Therefor I cannot change the default version later.