Skip to content

nodejs_18: fix build with clang 16#263847

Merged
vcunat merged 2 commits intoNixOS:staging-nextfrom
reckenrode:nodejs18-fix
Oct 31, 2023
Merged

nodejs_18: fix build with clang 16#263847
vcunat merged 2 commits intoNixOS:staging-nextfrom
reckenrode:nodejs18-fix

Conversation

@reckenrode
Copy link
Contributor

@reckenrode reckenrode commented Oct 27, 2023

Description of changes

While node v18 successfully built, it was crashing in staging-next #263535 while building packages. This patch reverts the previous attempt at a fix and builds nodejs_18 with clang 15 instead. paperless-ngx.frontend was confirmed to successfully build with this patch applied.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment label Oct 27, 2023
@reckenrode reckenrode mentioned this pull request Oct 27, 2023
Closed
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. labels Oct 27, 2023
@reckenrode reckenrode mentioned this pull request Oct 29, 2023
Closed
13 tasks
eliasnaur
eliasnaur reviewed Oct 29, 2023
View reviewed changes
pkgs/development/web/nodejs/v18.nix Outdated
Copy link
Contributor

@eliasnaur eliasnaur Oct 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: did you mean Node v18 here, or did I misunderstand something?

Copy link
Contributor Author

@reckenrode reckenrode Oct 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy-paste mistake. It’s the same solution as nodejs_14 copied into nodejs_18. I can push an update that corrects the comment.

Copy link
Contributor

@eliasnaur eliasnaur Oct 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. An update wasn't necessary; I'm just an eager onlooker from the peanut gallery.

@reckenrode
Copy link
Contributor Author

reckenrode commented Oct 29, 2023 •
edited
Loading

I should also note that #264091 is an alternative that addresses the clang issue more generally. That PR would be my preferred solution to building with other clangs (rather than per-derivation manual overrides as necessary), but it would be understandable if it’s too much right now.

Edit: That PR is past the breaking changes deadline for LLVM, so this is the preferred one for 23.11.

@reckenrode
Revert "nodejs_18: fix build with clang 16"
81d2db9 
This reverts commit 81e4ca5.
@reckenrode
nodejs_18: fix build with clang 16 mk2
b034e4c 
Trying to backport the fixes from v8 caused crashes with npm when
building other packages, so just build it with clang 15.
@reckenrode reckenrode mentioned this pull request Oct 31, 2023
Merged
13 tasks
@vcunat
Copy link
Member

vcunat commented Oct 31, 2023 •
edited
Loading

-Wenum-constexpr-conversion errors

Isn't normally the easiest approach to just avoid such errors by using a -Wno-error=enum-constexpr-conversion flag or similar?

EDIT: just as a quick question. I haven't tried to really review this PR in detail.

@reckenrode
Copy link
Contributor Author

reckenrode commented Oct 31, 2023 •
edited
Loading

-Wenum-constexpr-conversion errors

Isn't normally the easiest approach to just avoid such errors by using a -Wno-error=enum-constexpr-conversion flag or similar?

It’s (probably) going to be made a hard error in a future version of clang (see llvm/llvm-project#59036). Pinning the version to clang 15 avoids possible future breakage.

@vcunat vcunat merged commit 80d84b4 into NixOS:staging-next Oct 31, 2023
@vcunat
Copy link
Member

vcunat commented Oct 31, 2023

OK, I think there's been enough time for feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@eliasnaur eliasnaur eliasnaur left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@reckenrode @vcunat @eliasnaur