always build grammars with c++14 and c11 #6792
Conversation
pascalkuthe
added
C-bug
|
Sorry if this is not strictly related to this PR, but on Linux (I have not tested on another OS), if there is no c++ compiler installed when using |
It does seem like we only print to stdout for error and don't actually return a proper error This would be easy to fix but would change the error output slightly from: to (ignore the exact errors here I just made the build fail quickly this would work the same for any errors). Should probably be a separate PR but not sure if we want to change the formatting a bit. Anyway, this patch fixes what you described: diff --git a/helix-loader/src/grammar.rs b/helix-loader/src/grammar.rs
index 060d9332..09ced91f 100644
--- a/helix-loader/src/grammar.rs
+++ b/helix-loader/src/grammar.rs
@@ -1,4 +1,4 @@
-use anyhow::{anyhow, Context, Result};
+use anyhow::{anyhow, Context, Result, bail};
use serde::{Deserialize, Serialize};
use std::fs;
use std::time::SystemTime;
@@ -178,10 +178,10 @@ pub fn build_grammars(target: Option<String>) -> Result<()> {
if !errors.is_empty() {
let len = errors.len();
- println!("{} grammars failed to build", len);
for (i, error) in errors.into_iter().enumerate() {
println!("\tFailure {}/{}: {}", i, len, error);
}
+ bail!("{} grammars failed to build", len);
}
Ok(()) |
pascalkuthe
changed the title
@pascalkuthe Great news you fixed it so fast! I have no opinion on the formatting, but I hope the fix can get into |
|
It turns out that the grammar build was actually failing in CI too but because of the bug pointed out by @David-Else it didn't cause a build failure. With #6795 the macros build with master fails in CI but succeeds with this branch so this is actually reproducible. The CI also showed an oversight in this PR, only on macOS, turns out I also took the chance to cleanup the build flags a bit. |
|
Hmm I remember we used to build with C++14 because the haskell grammar had more complicated requirements but that failed to build on some systems. Eventually the haskell grammar was simplified and we dropped the requirement. |
|
That was probably because the cross images used a very old compiler, I think that has since been resolved |
|
If its really an issue in the future we can lower the c++ version down to something more compatible and patch the grammars. c++14 support is pretty widespread these days so I hope it won't be an issue. The only distro wehrre this may cause problems that is still supported is redhl 7 based stuff and that's nearing EOL and has an easy way to install a newer compiler trough the support packagss. Fxing the exact c standard version probably a good idea regardless even if we would habe to lower the version again to make sure builds are consistent across different systems |
Commit ca65d31 ("always build grammars with c++14 and c11 (helix-editor#6792)") turned on semi-strict standard mode for C and C++ by accident. Some grammars use non-standard functions such as isascii (from <ctype.h>) and fail to build as a result, particularly with future compilers which do not support implicit function declarations. (Ideally, the build environment would only raise the standard versions relative to the toolchain default, not lower them, but this change does not implement that.)
Closes #6788
Usually, modern compilers always default to c++ 14 or c++17 anyway. Clang defaults to c++14 and gcc to c++17. MSVC++ even dropped support for older standards in msvc17 (and defaults to c++14) However, it seems that apple-clang sometimes (but not always) defaults to c++98. By explicitly setting the argument we can work around such cases.
This also ensures more consistent builds across various systems (that may default to newer c++ standards in the future) and provides more easily understood error messages on ancient systems that don't support c++14 yet.