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

Build failure on Cabal and Just grammars #6788

Closed
heliostatic opened this issue Apr 17, 2023 · 10 comments · Fixed by #6792
Closed

Build failure on Cabal and Just grammars #6788

heliostatic opened this issue Apr 17, 2023 · 10 comments · Fixed by #6792
Labels
C-bug Category: This is a bug E-has-instructions Call for participation: Has instructions for fixing the issue and opening a PR

Comments

@heliostatic
Copy link
Contributor

Summary

Getting build failures with the new Cabal (#6485) grammar and Just. Appears to be C++ related?

Reproduction Steps

I tried this:

  1. hx --grammar build

I expected this to happen:

Grammars built with no errors

Instead, this happened:

Errors building just and cabal grammars.

grammar build log
Building 149 grammars
147 grammars already built
2 grammars failed to build
        Failure 0/2: Parser compilation failed.
Stdout:
Stderr: /Users/bc/.config/helix/runtime/grammars/sources/just/src/scanner.cc:15:24: warning: default member initializer for non-static data member is a C++11 extension [-Wc++11-extensions]
  uint32_t prev_indent = 0;
                       ^
/Users/bc/.config/helix/runtime/grammars/sources/just/src/scanner.cc:47:3: warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions]
  auto curr = std::to_string(state->prev_indent);
  ^
/Users/bc/.config/helix/runtime/grammars/sources/just/src/scanner.cc:64:14: error: expected expression
    *state = {};
             ^
/Users/bc/.config/helix/runtime/grammars/sources/just/src/scanner.cc:88:3: warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions]
  auto advance = [lexer] { lexer->advance(lexer, false); };
  ^
/Users/bc/.config/helix/runtime/grammars/sources/just/src/scanner.cc:88:18: error: expected expression
  auto advance = [lexer] { lexer->advance(lexer, false); };
                 ^
/Users/bc/.config/helix/runtime/grammars/sources/just/src/scanner.cc:89:3: warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions]
  auto skip = [lexer] { lexer->advance(lexer, true); };
  ^
/Users/bc/.config/helix/runtime/grammars/sources/just/src/scanner.cc:89:15: error: expected expression
  auto skip = [lexer] { lexer->advance(lexer, true); };
              ^
/Users/bc/.config/helix/runtime/grammars/sources/just/src/scanner.cc:91:3: warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions]
  auto get_column = [lexer] { return lexer->get_column(lexer); };
  ^
/Users/bc/.config/helix/runtime/grammars/sources/just/src/scanner.cc:91:21: error: expected expression
  auto get_column = [lexer] { return lexer->get_column(lexer); };
                    ^
/Users/bc/.config/helix/runtime/grammars/sources/just/src/scanner.cc:128:5: warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions]
    auto indent = get_column();
    ^
6 warnings and 4 errors generated.

        Failure 1/2: Parser compilation failed.
Stdout:
Stderr: /Users/bc/.config/helix/runtime/grammars/sources/cabal/src/scanner.cc:26:60: error: expected expression
        for_each(indent_lvls.cbegin(), indent_lvls.cend(), [buffer, &i](uint8_t const n) { buffer[i++] = n; });
                                                           ^
/Users/bc/.config/helix/runtime/grammars/sources/cabal/src/scanner.cc:70:9: warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions]
        auto cur_indent_lvl = indent_lvls.back();
        ^
1 warning and 1 error generated.

Helix log

~/.cache/helix/helix.log

Platform

macOS

Terminal Emulator

Alacritty

Helix Version

helix 23.03 (4cdba7c)

@heliostatic heliostatic added the C-bug Category: This is a bug label Apr 17, 2023
@pascalkuthe
Copy link
Member

seems like your c++ compiler might be very old and doesn't use c++11 by default. You can try c++ --version to figure out your compiler version. Macos ships with clang which defaults to c++14 since version 6.0 (which is 5 years old)

@heliostatic
Copy link
Contributor Author

That was my thought, but I'm not sure what would be better?

Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: arm64-apple-darwin22.4.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bi

@pascalkuthe
Copy link
Member

pascalkuthe commented Apr 17, 2023

Maybe apple-clang has a different default? Googling seems to turn up other people with a different problem, but other people always claim it works and the manpage definitely says gnu++14 is default even on mac... Odd. To enforce a uniform standard we could pass -std=c++14 manually but that's not quite portable (because windows does flags differently...) sadly the cc crate doesn't have builtin functionality for that but we could use the workaround suggested here rust-lang/cc-rs#565 (comment) for now.

This is not quite the same as the normal clang default gnu++14 but the gnu extensions aren't huge and I don't think grammars should be using gnu extensions anyway.

@pascalkuthe pascalkuthe added the E-has-instructions Call for participation: Has instructions for fixing the issue and opening a PR label Apr 17, 2023
@h1t
Copy link
Contributor

h1t commented Apr 17, 2023

May need to add scanner.cc to the sources part of binding.gyp:

      "sources": [
        "bindings/node/binding.cc",
        "src/parser.c",
        # Add this line:
        "src/scanner.cc"
      ],

@pascalkuthe
Copy link
Member

that shouldn't matter we invoke the c compiler directly tough the CC crate. binding.gyp is for integration with node.js and not used by helix in any way

@h1t
Copy link
Contributor

h1t commented Apr 17, 2023

We can't compile C and Cpp files in one command using -std=c99 for C and -std=c++11 for Cpp.
We have to do it with two separate commands.

@the-mikedavis
Copy link
Member

We probably don't need to restrict C to c99, I think we can just use c++11 if there is a scanner

@h1t
Copy link
Contributor

h1t commented Apr 17, 2023

clang 14 on macos

$ clang -xc ./parser.c -std=c++11 ./scanner.cc
error: invalid argument '-std=c++11' not allowed with 'C'

@pascalkuthe
Copy link
Member

To use -std=c++14 you need to use the clang++. They are both the same binary but the clang++ driver behaves differently. #6792 should fix the problem. Please try building with that PR @heliostatic

@pascalkuthe
Copy link
Member

Ok this was interesting, #6792 fixed this problem and with #6795 this actually fails in CI too (it already does right now) as we were simply ignoring grammar build failures by accident in the past.

With those two PRs the problem is fixed and can be automatically tested in CI (so similar problems don't reappear in the future)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug E-has-instructions Call for participation: Has instructions for fixing the issue and opening a PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants