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.

Sign up for GitHub

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

Jump to bottom

nodejs: Remove the requirement for dir-prefix from includes #57131

Closed
wants to merge 1 commit into from
Closed

nodejs: Remove the requirement for dir-prefix from includes #57131

wants to merge 1 commit into from

Conversation

clefru
Copy link
Contributor

@clefru clefru commented Mar 9, 2019

Motivation for this change

"npm install scrypt" fails to compile with the compiler not being able to find "node_version.h" in the "nan" dependency. Looking at our installation, we find that we have /nix/store/.../nodejs.../include/node containing the node_version.h, so #include <node/node_version.h> would work but #include <node_version.h> doesn't. Strangely this step only fails with CXX step not with the CC.

Searching github for what's the correct way seems to find that a lot of projects just do #include <node_version.h> hence I found it ok to make our nix installation strip the node path.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@matthewbauer
Copy link
Member

@GrahamcOfBorg build nodejs

matthewbauer
matthewbauer reviewed Mar 4, 2020
View reviewed changes
pkgs/development/web/nodejs/nodejs.nix Outdated
# support packages including just <node_version.h>
mv $out/include/node/* $out/include/
rmdir $out/include/node
ln -s . $out/include/node
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ln -s . $out/include/node
ln -s $out/include $out/include/node

@stale
Copy link

stale bot commented Nov 11, 2020

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 11, 2020
@SuperSandro2000
Copy link
Member

@clefru friendly ping

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 24, 2020
@clefru
Copy link
Contributor Author

clefru commented Nov 29, 2020

I updated the PR as requested and tested that "npm install scrypt" compiles with nodejs-10_x (and python+gnumake+gcc+binutils installed).

@ofborg ofborg bot requested review from gilligan, cko and marsam November 29, 2020 10:45
gilligan
gilligan reviewed Nov 29, 2020
View reviewed changes
Copy link
Contributor

@gilligan gilligan left a comment

Choose a reason for hiding this comment

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

I’m a bit confused as to what the correct header installation is. Creating a symlink might not be that big a deal, but aren’t some packages simply using incorrect incorrect include statements? In that case they just have to fail and nixpkgs should not be providing workarounds for that.

Do other nodejs distribution packages generally provide both locations? I would really appreciate some background on this if possible.

@clefru
Copy link
Contributor Author

clefru commented Dec 6, 2020

@gilligan All those are valid questions. But I am sorry, I won't have time to study this topic further and my focus switched away from NodeJS.

Feel free to close this PR unmerged.

@clefru
Copy link
Contributor Author

clefru commented May 13, 2021

Closing. I lost context on this PR and moved on.

@clefru clefru closed this May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer matthewbauer left review comments

@gilligan gilligan gilligan left review comments

@doronbehar doronbehar doronbehar approved these changes

@cillianderoiste cillianderoiste Awaiting requested review from cillianderoiste

@cko cko Awaiting requested review from cko

@marsam marsam Awaiting requested review from marsam

Assignees
No one assigned
Labels
6.topic: nodejs 10.rebuild-darwin: 101-500 10.rebuild-linux: 101-500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@clefru @matthewbauer @SuperSandro2000 @gilligan @doronbehar @prusnak @GrahamcOfBorg