Skip to content

tree-sitter-grammar: ensure enough space when updating id of shared lib on Darwin#271974

Closed
c4710n wants to merge 1 commit intoNixOS:stagingfrom
plastic-forks:zd/fix-darwin-tree-sitter
Closed

tree-sitter-grammar: ensure enough space when updating id of shared lib on Darwin#271974
c4710n wants to merge 1 commit intoNixOS:stagingfrom
plastic-forks:zd/fix-darwin-tree-sitter

Conversation

@c4710n
Copy link
Contributor

@c4710n c4710n commented Dec 4, 2023

This PR is trying to fix the error when updating install names on Darwin:

error: install_name_tool: changing install names or rpaths can't be redone for: [...]
(the program must be relinked, and you may need to use -headerpad or -headerpad_max_install_names)

This technique is also used by:

It should be safe to add it. I have tested on x86_64-darwin.

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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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.

Add a 👍 reaction to pull requests you find important.

@c4710n c4710n changed the title tree-sitter: ensure enough space when update install names on Darwin tree-sitter-grammar: ensure enough space when update install names on Darwin Dec 4, 2023
@c4710n c4710n force-pushed the zd/fix-darwin-tree-sitter branch from 702e789 to 6d366fa Compare December 4, 2023 04:17
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

change looks good -- one nit described inline.
a couple of things:

  1. how do i build this on darwin to reproduce this error?
  2. i started to run a nix run .\#nixpkgs-review -- pr 271974 from the nixpkgs src root and it seems like this causes 1.4k rebuilds, so the change might need to target the staging branch -- i've never done this but there are some pointers in CONTRIBUTING.md

@c4710n c4710n force-pushed the zd/fix-darwin-tree-sitter branch from 6d366fa to 4a7cb40 Compare December 4, 2023 05:52
@c4710n c4710n changed the base branch from master to staging December 4, 2023 05:57
@c4710n c4710n requested a review from a user December 4, 2023 05:57
@github-actions github-actions bot added 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: kernel The Linux kernel 8.has: documentation This PR adds or changes documentation 8.has: changelog This PR adds or changes release notes 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: emacs Text editor 6.topic: policy discussion Discuss policies to work in and around Nixpkgs 6.topic: vscode A free and versatile code editor that supports almost every major programming language. labels Dec 4, 2023
@c4710n c4710n force-pushed the zd/fix-darwin-tree-sitter branch from 4a7cb40 to 57f27dc Compare December 4, 2023 05:58
@github-actions github-actions bot removed 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: kernel The Linux kernel 8.has: documentation This PR adds or changes documentation labels Dec 4, 2023
@github-actions github-actions bot removed 8.has: changelog This PR adds or changes release notes 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: emacs Text editor 6.topic: policy discussion Discuss policies to work in and around Nixpkgs 6.topic: vscode A free and versatile code editor that supports almost every major programming language. labels Dec 4, 2023
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Dec 4, 2023
@c4710n
Copy link
Contributor Author

c4710n commented Dec 4, 2023

Hey, @a-n-n-a-l-e-e Thanks for your guidance.

I have:

  • moved the change to env variables
  • changed the target to staging branch

And, I changed the title of PR and commit message to more descriptive ones.


how do i build this on darwin to reproduce this error?

This error won't occur if we use the output directly. But when we want to further process the output, the error will occur. For example, let's build tree-sitter-clojure:

$ nix build .#legacyPackages.x86_64-darwin.tree-sitter-grammars.tree-sitter-clojure

# create a lab
$ mkdir lab && cd lab

$ cp ../result/parser  ./parser

# changes identification name of the dynamic shared library to a very long name
$ install_name_tool \
  -id /nix/store/dwh6vsxb1mr2rjzmr11hn51ih76ilxdj-tree-sitter-grammars/lib/libtree-sitter-clojure.dylib \
  ./parser
error: install_name_tool: changing install names or rpaths can't be redone for: ./parser (for architecture x86_64) because larger updated load commands do not fit (the program must be relinked, and you may need to use -headerpad or -headerpad_max_install_names)

In short, tree-sitter-grammar isn't broken, but it doesn't support changing the identification name of a dynamic shared library to a very long name, which is necessary in my case. This PR is trying to add this kind of support, without effecting others.

@c4710n c4710n changed the title tree-sitter-grammar: ensure enough space when update install names on Darwin tree-sitter-grammar: ensure enough space when updating id of shared lib on Darwin Dec 4, 2023
@c4710n c4710n force-pushed the zd/fix-darwin-tree-sitter branch from 57f27dc to c790805 Compare December 4, 2023 06:36
@ghost
Copy link

ghost commented Dec 4, 2023

i see -- could this be solved by using overrideAttrs to modify the build locally? doing something like ...

nix build --impure --expr \
'with import ./. {}; tree-sitter-grammars.tree-sitter-clojure.overrideAttrs(prev: { env = (prev.env or {} ) // { NIX_LDFLAGS = "-headerpad_max_install_names"; };})'

or

nix build --impure --expr \
'with import <nixpkgs> {}; tree-sitter-grammars.tree-sitter-clojure.overrideAttrs(prev: { env = (prev.env or {} ) // { NIX_LDFLAGS = "-headerpad_max_install_names"; };})'

that wouldn't require updating nixpkgs?


i am not sure if the migration to staging went well -- it seems like perhaps a bunch of people got pulled and might ask to close the PR (can open a new one). I am not entirely sure as i've never delt with this on nixkgs but this link might be helpful:
https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#something-went-wrong-and-a-lot-of-people-were-pinged

@c4710n
Copy link
Contributor Author

c4710n commented Dec 4, 2023

Thanks, @a-n-n-a-l-e-e. Your solution works, it's better to scope special code to my own case.

I'll close this PR. Thank you, again.

@c4710n c4710n closed this Dec 4, 2023
@c4710n c4710n deleted the zd/fix-darwin-tree-sitter branch December 4, 2023 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: darwin Running or building packages on Darwin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant