Bat: cargo install failing due to a compilation problem of onig_sys crate

[rekka]

cargo install bat failing on Ubuntu 18.04 due to https://github.com/rust-onig/rust-onig/issues/109 (bat version 0.12.1)

[tuxruffian]

Build fails on onig_sys on CentOS 7 as well.

[alaaibrahim]

On fix I had for this to install is to modify the Cargo.toml file and fix the syntect version to 3.2.0

--- a/Cargo.toml
+++ b/Cargo.toml
@@ -33,7 +33,7 @@ default-features = false
 features = []

 [dependencies.syntect]
-version = "3.2.1"
+version = "=3.2.0"
 default-features = false
 features = ["parsing", "yaml-load", "dump-load", "dump-create"]

I was able to install afterwards

[sharkdp]

@alaaibrahim Thank you for your feedback. Unfortunately, pinning the version to 3.2.0 would re-introduce this bug: #626 (build failure on certain architectures).

[l1a]

FYI... fails on Debian 9 but compiles fine (without clang/llvm) on Fedora 30.

[vmedea]

apt-get install libclang-dev made it build for me on Debian 10.1 aarch64

i'm reaally surprised, that a cli tool to view files has a hard dependency on libclang, though

[sharkdp]

i'm reaally surprised, that a cli tool to view files has a hard dependency on libclang, though

That's a build-time-only dependency which (although maybe not optimal) is certainly not too surprising. bat has some C-code dependencies and libclang can be used to automatically generate C-to-Rust bindings.

If you want, you can actually get zero-dependency versions of bat (statically linked musl builds on the Release page), so I don't think there is any reason to complain about runtime dependencies.

[vmedea]

That's a build-time-only dependency which (although maybe not optimal) is certainly not too surprising.

oh good that it's build-time-only, hadn't noticed that !

[sharkdp]

This can be resolved once https://github.com/rust-onig/rust-onig/pull/126 is merged. This would also remove the need for libclang to be installed at build time.

Another option would be that https://github.com/trishume/syntect/pull/270 gets merged, in which case onig would not be a dependency anymore.

[trishume]

The fancy-regex mode was just merged and released as v4.0.0! You may want to try using it to fix this issue, although it does halve performance so you may only want to use it on platforms where it's needed.

Still hoping that onig's build will eventually improve but for now fancy-regex is a good option.

[sharkdp]

Thank you very much for the update!

Unfortunately, updating is not completely trivial because syntect::parsing::ParseSyntaxError does not implement Send anymore:

• https://docs.rs/syntect/3.3.0/syntect/parsing/enum.ParseSyntaxError.html
• https://docs.rs/syntect/4.0.0/syntect/parsing/enum.ParseSyntaxError.html

(due to the Box<dyn Error> in the RegexCompileError variant).

This could be fixed by using dyn Error + Send in a few places in syntect

diff --git a/src/parsing/regex.rs b/src/parsing/regex.rs
index 064c232..b0aeac2 100644
--- a/src/parsing/regex.rs
+++ b/src/parsing/regex.rs
@@ -32,7 +32,7 @@ impl Regex {
     }

     /// Check whether the pattern compiles as a valid regex or not.
-    pub fn try_compile(regex_str: &str) -> Option<Box<dyn Error>> {
+    pub fn try_compile(regex_str: &str) -> Option<Box<dyn Error + Send>> {
         regex_impl::Regex::new(regex_str).err()
     }

@@ -142,7 +142,7 @@ mod regex_impl {
     }

     impl Regex {
-        pub fn new(regex_str: &str) -> Result<Regex, Box<dyn Error>> {
+        pub fn new(regex_str: &str) -> Result<Regex, Box<dyn Error + Send>> {
             let result = onig::Regex::with_options(
                 regex_str,
                 RegexOptions::REGEX_OPTION_CAPTURE_GROUP,
@@ -210,7 +210,7 @@ mod regex_impl {
     }

     impl Regex {
-        pub fn new(regex_str: &str) -> Result<Regex, Box<dyn Error>> {
+        pub fn new(regex_str: &str) -> Result<Regex, Box<dyn Error + Send>> {
             let result = fancy_regex::Regex::new(regex_str);
             match result {
                 Ok(regex) => Ok(Regex { regex }),
diff --git a/src/parsing/yaml_load.rs b/src/parsing/yaml_load.rs
index dd66503..6353bb4 100644
--- a/src/parsing/yaml_load.rs
+++ b/src/parsing/yaml_load.rs
@@ -18,7 +18,7 @@ pub enum ParseSyntaxError {
     /// Some keys are required for something to be a valid `.sublime-syntax`
     MissingMandatoryKey(&'static str),
     /// Invalid regex
-    RegexCompileError(String, Box<dyn Error>),
+    RegexCompileError(String, Box<dyn Error + Send>),
     /// A scope that syntect's scope implementation can't handle
     InvalidScope(ParseScopeError),
     /// A reference to another file that is invalid

or we have to work around this in bat somehow, which would also be possible.

I wanted to run bats little benchmark script with fancy-regex enabled, but unfortunately it leads to a crash when trying to highlight test-src/jquery-3.3.1.js (or any other JS file). This could be due to the custom JS syntax we use in bat (i.e. not the one from "Packages"):

> ./bat-fancy test-src/jquery-3.3.1.js
thread 'main' panicked at 'regex string should be pre-tested: InvalidEscape', /home/shark/Dropbox/Informatik/rust/syntect/src/parsing/regex.rs:70:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Interestingly, the fancy-regex version is quite a bit faster when highlighting miniz.c:

bat --style=full --color=always --paging=never test-src/miniz.c

| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|:---|---:|---:|---:|---:|
| onig | 80.1 ± 0.8 | 78.9 | 83.3 | 1.23 ± 0.02 |
| fancy | 65.3 ± 1.0 | 64.1 | 70.4 | 1.00 |

[trishume]

Ah dang. I'll try and do a point release with the Send fix this evening.

I think the crash is due to something I forgot to put in the release notes and maybe should have better monitoring for in general but you need to re-generate your pack files for the new version. The format is the same so nothing immediately fails but the regexes get rewritten to be compatible with fancy-regex before being saved in the pack file, to reduce loading time.

And yah seems plausible that sometimes fancy-regex will be much faster. regex is a really good regex engine, just in the Javascript and Rust benchmarks I use it's slower.

[sharkdp]

Ah dang. I'll try and do a point release with the Send fix this evening.

That sounds great, but there is absolutely no hurry! :smile: I have opened a PR: https://github.com/trishume/syntect/pull/285

I think the crash is due to something I forgot to put in the release notes and maybe should have better monitoring for in general but you need to re-generate your pack files for the new version.

Oh :man_facepalming:. I thought about it shortly, but apparently came to the wrong conclusion :smile:

[sharkdp]

This can be resolved once rust-onig/rust-onig#126 is merged. This would also remove the need for libclang to be installed at build time.

The PR has been merged today. A new version of onig has been released. We are now waiting for this to be included in syntect: https://github.com/trishume/syntect/pull/293 (thanks to @Keats).

Another option would be that trishume/syntect#270 gets merged, in which case onig would not be a dependency anymore.

This has been merged a while ago and we have made it possible to use it within bat, but using fancy-regex is currently not (yet) an option, as it can not handle all regex features in the custom syntaxes included in bat, see https://github.com/trishume/syntect/issues/287

[sharkdp]

closed via #1012.

[sharkdp]

Released in bat v0.15.2. Feedback would be appreciated!

[enizor]

Install worked for me via cargo install-update. Somehow it even worked yesterday (updating to 0.15.1 but #1012 had landed).
Re-updating to 0.15.2 worked too :)