Skip to content
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

nodejs_18: fix build with clang 16 #263847

Merged
merged 2 commits into from
Oct 31, 2023
Merged

Conversation

reckenrode
Copy link
Contributor

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.


let
# Clang 16+ cannot build Node v14 due to -Wenum-constexpr-conversion errors.
Copy link
Contributor

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

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

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

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.

Trying to backport the fixes from v8 caused crashes with npm when
building other packages, so just build it with clang 15.
@vcunat
Copy link
Member

vcunat commented Oct 31, 2023

-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

-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
9 checks passed
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants