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

Add formatting check to CI pipeline #9106

Merged
merged 2 commits into from Jun 14, 2021
Merged

Add formatting check to CI pipeline #9106

merged 2 commits into from Jun 14, 2021

Conversation

Vexu
Copy link
Member

@Vexu Vexu commented Jun 13, 2021

Reopening of #8219 now that the parsing issue has been fixed. I also added a check-ast on the self hosted compiler main.zig since that should also test the standard library.

@g-w1
Copy link
Contributor

g-w1 commented Jun 13, 2021

Ast-check does not follow imports, so you should do find .. -name "*.zig" | xargs ./zig ast-check or something similar.
Also, ./zig fmt --check .. should work and it is more simple.

@@ -59,6 +59,10 @@ unset CXX

make $JOBS install

# look for formatting errors
release/bin/zig fmt --check ../lib/std ../doc ../src ../tools ../test ../build.zig
release/bin/zig ast-check ../src/main.zig
Copy link
Member

Choose a reason for hiding this comment

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

I suggest we only do the zig fmt check for now, for a few reasons:

  • This doesn't actually check any files other than src/main.zig
  • std lib AST checking is already happening with every stage2 test
  • It's an unrelated change to adding zig fmt checking
  • we may want to consider taking advantage of the thread pool and cache, which is already happening with std lib AST checking

Copy link
Member Author

Choose a reason for hiding this comment

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

* This doesn't actually check any files other than src/main.zig

Solved with find . -name "*.zig" ! -path "*/zig-cache/*" ! -path "*/zig-out/*" | xargs -n1 release/bin/zig ast-check.

* std lib AST checking is already happening with every stage2 test

Std files can be easily excluded in the command.

* It's an unrelated change to adding zig fmt checking

Sure, it did find a some bugs in stage2 and a lot of nonconforming code everywhere else so it might be better to do separately.

* we may want to consider taking advantage of the thread pool and cache, which is already happening with std lib AST checking

It takes less than 15 seconds to check all ~900 files on my laptop on a debug build, I don't think performance is going to be that big of an issue.

ci/azure/linux_script Outdated Show resolved Hide resolved
ci/azure/linux_script Outdated Show resolved Hide resolved
@@ -34,7 +34,7 @@
// - Only supports round-to-zero
// - Does not handle denormals

const std = @import("../std.zig");
const std = @import("std");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed? I think ../std.zig is more correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a pretty much even divide between @import("std.zig") and @import("std"), I changed this because I wanted to test this file specifically.

Copy link
Member

Choose a reason for hiding this comment

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

Generally, std lib should import std.zig by file path, setting an example for other packages. For zig test you can use --main-pkg-path to test files that import with "..".

@Vexu
Copy link
Member Author

Vexu commented Jun 13, 2021

$ hyperfine "zig-out/bin/zig fmt . --ast-check --check" --warmup 1
Benchmark #1: zig-out/bin/zig fmt . --ast-check --check
  Time (mean ± σ):      6.056 s ±  0.022 s    [User: 2.499 s, System: 3.552 s]
  Range (min … max):    6.022 s …  6.094 s    10 runs

Not too bad for ~860 files.

@andrewrk andrewrk merged commit 86ebd4b into ziglang:master Jun 14, 2021
@Vexu Vexu deleted the fmt branch June 14, 2021 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants