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

helix: fix build with clang 16 #268191

Merged
merged 1 commit into from
Nov 18, 2023
Merged

helix: fix build with clang 16 #268191

merged 1 commit into from
Nov 18, 2023

Conversation

reckenrode
Copy link
Contributor

Description of changes

Fix an implicit int error in one of the tree-sitter grammars.

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.

Fix an implicit int error in one of the tree-sitter grammars.
Copy link
Contributor

@adamcstephens adamcstephens left a comment

Choose a reason for hiding this comment

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

Result of nixpkgs-review pr 268191 run on x86_64-linux 1

1 package built:
  • helix

Result of nixpkgs-review pr 268191 run on aarch64-darwin 1

1 package built:
  • helix

Result of nixpkgs-review pr 268191 run on aarch64-linux 1

1 package failed to build:
  • helix

Result of nixpkgs-review pr 268191 run on x86_64-darwin 1

1 package built:
  • helix

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 18, 2023
@zowoq
Copy link
Contributor

zowoq commented Nov 18, 2023

Thanks!

@zowoq zowoq merged commit d9c118d into NixOS:master Nov 18, 2023
26 of 27 checks passed
@beeb
Copy link
Contributor

beeb commented Nov 19, 2023

Not sure why but this broke the build at least on darwin x86 with home-manager:

error: builder for '/nix/store/nzs5j2qxl59kfic0x0rm7bbjjaj5n625-helix-23.10.drv' failed with exit code 101;
       last 10 log lines:
       >   /private/tmp/nix-build-helix-23.10.drv-0/source/runtime/grammars/sources/rescript/src/scanner.c:184:10: warning: unused variable 'is_unnested' [-Wunused-variable]
       >       bool is_unnested = state->parens_nesting == 0;
       >            ^
       >   1 warning and 1 error generated.
       >
       >
       >   --- stderr
       >   thread 'main' panicked at helix-term/build.rs:7:14:
       >   Failed to compile tree-sitter grammars: 1 grammars failed to build
       >   note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
       For full logs, run 'nix log /nix/store/nzs5j2qxl59kfic0x0rm7bbjjaj5n625-helix-23.10.drv'.
       ```

@reckenrode
Copy link
Contributor Author

reckenrode commented Nov 19, 2023

Not sure why but this broke the build at least on darwin x86 with home-manager

Can you check the log further up to see what the error was and post that? That warning shouldn’t be causing the failure.

FWIW, I tried building both from master and nixpkgs-unstable, and helix builds for me.

$ nix build 'github:NixOS/nixpkgs#legacyPackages.x86_64-darwin.helix'
$ nix-store -qR ./result
/nix/store/0ly5lpwanagc5qpmnqrw8aqxb4ivw7kf-bash-5.2-p15
/nix/store/4j38m9c97qzf7zhamm7i7gyrgbikcfhn-libiconv-50
/nix/store/pka340xbqzk4r46si0hjkadk00c69a18-libcxxabi-16.0.6
/nix/store/x4s0wi6d2z38wa0jb4yf769sl1c0c50z-libcxx-16.0.6
/nix/store/pcms9c977cgfgiynxgxk14ipyaiz6g0g-helix-23.10
$ nix build 'github:NixOS/nixpkgs/nixpkgs-unstable#legacyPackages.x86_64-darwin.helix'
$ nix-store -qR ./result
/nix/store/0ly5lpwanagc5qpmnqrw8aqxb4ivw7kf-bash-5.2-p15
/nix/store/4j38m9c97qzf7zhamm7i7gyrgbikcfhn-libiconv-50
/nix/store/pka340xbqzk4r46si0hjkadk00c69a18-libcxxabi-16.0.6
/nix/store/x4s0wi6d2z38wa0jb4yf769sl1c0c50z-libcxx-16.0.6
/nix/store/pcms9c977cgfgiynxgxk14ipyaiz6g0g-helix-23.10

@beeb
Copy link
Contributor

beeb commented Nov 19, 2023

I concede it's weird because it builds fine when I do nix shell nixpkgs#helix but when I add it to my home-manager config (there is a built-in programs.helix.enable) I get the error. It was building fine before the patch with home-manager. Could you try that maybe?

Here is the full error:

162 grammars built now
        ["astro", "awk", "bash", "bass", "beancount", "bibtex", "bicep", "blueprint", "c", "c-sharp", "capnp", "clojure", "cmake", "comment", "cpon", "cpp", "css", "cue", "d", "dart", "devicetree", "dhall", "diff", "docke>
  Failure 1/1: rescript Parser compilation failed.
  Stdout:
  Stderr: /private/tmp/nix-build-helix-23.10.drv-0/source/runtime/grammars/sources/rescript/src/scanner.c:134:9: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit->
    const in_string = state->in_quotes || state->in_backticks;
    ~~~~~ ^
    int
  /private/tmp/nix-build-helix-23.10.drv-0/source/runtime/grammars/sources/rescript/src/scanner.c:184:10: warning: unused variable 'is_unnested' [-Wunused-variable]
      bool is_unnested = state->parens_nesting == 0;
           ^
  1 warning and 1 error generated.


  --- stderr
  thread 'main' panicked at helix-term/build.rs:7:14:
  Failed to compile tree-sitter grammars: 1 grammars failed to build
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@beeb
Copy link
Contributor

beeb commented Nov 19, 2023

It might be a coincidence but I figured since the error originates from the patched file and they didn't update that derivation in home-manager since I had it working, it must be related.

@beeb
Copy link
Contributor

beeb commented Nov 19, 2023

Oh wait is it because home-manager doesn't include this patch? Or this just hasn't hit nixpkgs-unstable yet? So weird that I could build fine a few days ago...

EDIT: in all fairness I did update macOS today, not sure if related

@reckenrode
Copy link
Contributor Author

reckenrode commented Nov 19, 2023

I concede it's weird because it builds fine when I do nix shell nixpkgs#helix but when I add it to my home-manager config (there is a built-in programs.helix.enable) I get the error. It was building fine before the patch with home-manager. Could you try that maybe?

If I set programs.helix.enable in my Home Manager config and build it, it builds successfully. I’m currently tracking a local branch from master with a few other commits that haven’t been merged yet.

It might be a coincidence but I figured since the error originates from the patched file and they didn't update that derivation in home-manager since I had it working, it must be related.

Is this your flake? It has Home Manager following nixos-unstable, which is behind master and nixpkgs-unstabl. nixos-unstable doesn’t have the fix in this PR yet.

nix shell would work with that flake because nix.registry isn’t being set up for Darwin, and the default for nixpkgs is github:NixOS/nixpkgs/nixpkgs-unstable, which does have the change.

@beeb
Copy link
Contributor

beeb commented Nov 19, 2023

Okay I think I get it. It should be fixed once nixos-unstable gets this patch and the problem probably appeared when I updated macOS today. Sorry for the confusion! I'll try again in a few days. (or switch to nixpkgs-unstable)

@reckenrode
Copy link
Contributor Author

Okay I think I get it. It should be fixed once nixpkgs-unstable gets this patch and the problem probably appeared when I updated macOS today. Sorry for the confusion! I'll try again in a few days

If you switch to nixpkgs-unstable, Helix should build, but that may cause issues with NixOS since something is presumably preventing the nixos-unstable channel from advancing.

@beeb
Copy link
Contributor

beeb commented Nov 19, 2023

I'm actually not using nixOS at the moment so should be fine to switch, thanks!

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.

5 participants