Skip to content

building docker image is failing since 5.8.0 #1798

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bourgeoa opened this issue Nov 6, 2024 · 8 comments
Open

building docker image is failing since 5.8.0 #1798

bourgeoa opened this issue Nov 6, 2024 · 8 comments

Comments

@bourgeoa
Copy link
Member

bourgeoa commented Nov 6, 2024

@angelo
building docker image is failing since 5.8.0
the build has been updated to node 20 alpine and related test
Could it be the use of node --experimental-require-module ?

@bourgeoa bourgeoa changed the title docker image is failing building docker image is failing Nov 6, 2024
@bourgeoa bourgeoa changed the title building docker image is failing building docker image is failing since 5.8.0 Nov 6, 2024
@bourgeoa bourgeoa changed the title building docker image is failing building docker image is failing since 5.8.0 Nov 6, 2024
@melvincarvalho
Copy link
Contributor

Could it be the use of node --experimental-require-module

Yes—we shouldn’t be running with this experimental flag. It was deprecated in Node 20 and removed entirely in Node 21. Using experimental flags makes the Docker build fragile and incompatible with modern Node versions. Docker is important to NSS, and this breaks it.

This feels like a breaking change that was merged too early. I’d strongly favour reverting it, or at the very least, moving it to a separate branch until it’s properly supported and integrated.

We’re already on node:lts (Node 20.x), so it’s time to drop or isolate this.

@melvincarvalho
Copy link
Contributor

I suggest reverting to 5.7 and making a new experimental branch for experimental features that can be checked out and run on an instance with instructions on how to do that.

@csarven
Copy link
Member

csarven commented Apr 10, 2025

Let's play logical fallacies again.

Straw man: The codebase already uses require() consistently so this isn't a new practice or introduced in v5.8. And there is no proof that the issue is entirely caused by the use of --experimental-require-module. The actual issue with Docker may stem from Node 20's deprecation/removal behavior, not any new code.

False cause: If the Docker breakage stems from Node 20 removing support for that flag, the root issue is environmental, not a code regression in 5.8.

False dilemma: revert everything and isolate in a separate branch are extreme solutions. There are other options, ... patch docker to work w/o deprecated flags... migrate the codebase to use ESM properly... add logic for for dev/prod builds. So, no need to prematurely ignore middle-ground solutions.

Appeal to consequences: Is there an evaluation on 5.8 introducing the break? Docker configs can be updates to modern Node or whatever, or other long term solutions

Overgeneralization: assuming "experimental" equals to inherently unstable or unacceptable, regardless of pragmatic use.


But look, docker is not the only way to run NSS. The codebase hasn't fundamentally changed in its module style.

If there's still concern about 5.8 or suggestions for improvement, feel free to kick off a PR.

@bourgeoa
Copy link
Member Author

node --experimental-require-module may have been the issue for docker.
It shall not be anymore an issue because require-module has been backported in node 20.19.0 and node 22.14.0

Branch fix/issue#1798 has been created to remove the need for experimental-require-module parameter.
Locally tests passes without any issue.
But CI is failing see #1817

If you have any hint I need help.
For now we cannot make any patch to NSS with CI

@CxRes
Copy link
Collaborator

CxRes commented Apr 10, 2025

--experimental-require-module was introduced in Node 22.0.0 and was later back ported to Node 20.17.0, the Active LTS branch at the time. The flag (not the feature) was dropped in Node 23.0.0 because require ESM has reached RC status. The "experimental" warning has been turned off by default as of 23.5.0.

https://nodejs.org/api/modules.html#loading-ecmascript-modules-using-require

All of this information is available in the NodeJS documentation. The claims made regarding the flag contradict the facts and represents a failure to understand the NodeJS development cycle.

@melvincarvalho
Copy link
Contributor

The sarcastic and dismissive tone here — e.g. “let’s play logical fallacies again” and “failure to understand” — is not okay. It crosses into personal attack and violates the expectations of respectful collaboration set out in the W3C Code of Conduct. Let’s please keep this space professional and welcoming to all contributors.

On the technical issue:

  • The Docker image has been broken since 5.8.0 — that’s a regression in real-world usage.
  • It was caused by reliance on a deprecated, experimental flag removed in Node 21+.
  • Whether or not it’s technically backported doesn’t change the fact that it broke builds.

IMHO, we should revert to 5.7.x as the stable release, and treat anything involving unstable flags as experimental until it’s proven solid. Happy to help with that if there’s agreement.

@michielbdejong
Copy link
Member

michielbdejong commented Apr 14, 2025

The sarcastic and dismissive tone here — e.g. “let’s play logical fallacies again” and “failure to understand” — is not okay.

I totally agree, it's maybe not the only reason that we are, as a group, struggling to make peaceful progress in the Solid project, but sarcasm (especially in written comments) does not seem to be a useful tool for us, and it's probably doing more harm than good to the Solid project at this point.

@csarven from now on, please help to keep sarcasm and bitterness out of written interactions if you can?

@csarven
Copy link
Member

csarven commented Apr 14, 2025

I am bowing out of this thread after this comment.


@michielbdejong , if you're genuinely keen on putting effort to making "peaceful progress", I'm so with you, but then I need to request that you ask yourself why you're rushing here attempting to tone police, focusing only on my comments and not others', and why you are not making any remarks on logical fallacies as pointed out, refraining from requesting citations on unsubstantiated claims, or calling out argumentum ad nauseam. Let me know when you'd like to have that rational discussion in good faith.


As for any remaining technical discussions on this issue, there has not been a strong argument put forward to revert anything. If anything, reverting is likely to cause more concerns - after all, 5.8 is out there already, which includes other fixes and features - than making efforts towards improving whatever needs to be done. Reverting is out of the question as I see it (or in any open source project in a similar situation). If anyone cares enough, and willing to put their time, they can focus on putting their energy on fixing or improving the broken bits. Alternatively, they can consider refraining from repeating their request (argumentum ad nauseam) - we understood it the first time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants