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

check indentation of changes on GitHub #308

Merged
merged 11 commits into from Jul 29, 2021
Merged

check indentation of changes on GitHub #308

merged 11 commits into from Jul 29, 2021

Conversation

vogler
Copy link
Collaborator

@vogler vogler commented Jul 27, 2021

Stumbled upon this: https://pre-commit.ci
Not sure if we want automatic commits that fix indentation in PRs.
Also, https://pre-commit.com does not list OCaml, so we might as well just add another GitHub workflow for it.

Originally posted by @vogler in #236 (comment)

Setting up OCaml just to install ocp-indent to check the code seems like a lot overhead, but maybe this can be cached.

@vogler
Copy link
Collaborator Author

vogler commented Jul 27, 2021

Worked locally, but the action failed with

Run git reset --soft HEAD~ && ./scripts/hooks/pre-commit
fatal: ambiguous argument 'HEAD~': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

@sim642
Copy link
Member

sim642 commented Jul 27, 2021

This only checks the last commit, right? So if you push multiple commits, then it won't check for misindentations introduced by earlier commits that this workflow didn't run for.

@sim642 sim642 added the setup deps, CI, release label Jul 27, 2021
@vogler
Copy link
Collaborator Author

vogler commented Jul 27, 2021

This only checks the last commit, right? So if you push multiple commits, then it won't check for misindentations introduced by earlier commits that this workflow didn't run for.

Ah, yes, good point.
Confusing docs, maybe reset to ${{ github.event.push.before }} then.
https://docs.github.com/en/actions/reference/events-that-trigger-workflows#push
https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#push

@vogler
Copy link
Collaborator Author

vogler commented Jul 27, 2021

Ok, no, that's empty: git reset --soft

@vogler
Copy link
Collaborator Author

vogler commented Jul 27, 2021

Ok, ${{ github.event.before }} works.

Setup OCaml + ocp-indent still takes 1m21s + 30s.
So it seems the caching by ocaml/setup-ocaml@v2 is not that good (or missing some argument).
Something to consider for #296.

@vogler
Copy link
Collaborator Author

vogler commented Jul 27, 2021

With ocaml/setup-ocaml@v1 and actions/cache@v2 the run-time decreased from ~2m to 31-35s.
https://github.com/ocaml/setup-ocaml#post says they intentionally don't cache in post.
Ideally this would be even faster. Maybe just download & cache ocp-indent from somewhere instead of setting up opam.

ubuntu-latest is 20.04 LTS which has ocp-indent 1.7.0 instead of 1.8.1 from opam.
@vogler
Copy link
Collaborator Author

vogler commented Jul 29, 2021

Ok, with apt install ocp-indent instead of via opam install ocp-indent it's only 12s instead of 31s.
(ubuntu-latest is 20.04 LTS which has ocp-indent 1.7.0 instead of 1.8.1 from opam.)

At least this detour was helpful for some opam cache experiments for #296.

This is now fast enough (5s for installing ocp-indent), but maybe helpful for something else:
https://stackoverflow.com/questions/59269850/caching-apt-packages-in-github-actions-workflow
They also mention docker. Alternative for apt install ocp-indent: container: unibeautify/ocp-indent (https://hub.docker.com/r/unibeautify/ocp-indent) but I guess that'd be slower.

@vogler vogler merged commit a4efec0 into master Jul 29, 2021
@vogler vogler deleted the indent-workflow branch July 29, 2021 20:38
@michael-schwarz
Copy link
Member

In the run https://github.com/goblint/analyzer/runs/3199968802, we have the error message

ocp-indent file: src/analyses/arinc.ml, lines: 122
ocp-indent: internal error, uncaught exception:
            Failure("Config file not found - neither /etc/ocamlfind.conf nor the directory /etc/ocamlfind.conf.d")

@vogler vogler restored the indent-workflow branch July 30, 2021 09:56
vogler added a commit that referenced this pull request Jul 30, 2021
This reverts commit df2d401.

#308 (comment):

In the run https://github.com/goblint/analyzer/runs/3199968802, we have the error message

```
ocp-indent file: src/analyses/arinc.ml, lines: 122
ocp-indent: internal error, uncaught exception:
            Failure("Config file not found - neither /etc/ocamlfind.conf nor the directory /etc/ocamlfind.conf.d")
```
@vogler
Copy link
Collaborator Author

vogler commented Jul 30, 2021

So the apt package is broken? What does it need ocamlfind for? Strange.
I'll just revert to opam and merge.

@sim642
Copy link
Member

sim642 commented Jul 30, 2021

It's still somehow broken, cannot find ocp-indent: https://github.com/goblint/analyzer/runs/3201066169?check_suite_focus=true#step:6:8.

@vogler
Copy link
Collaborator Author

vogler commented Jul 30, 2021

I added a eval $(opam env) before in 0f86542 - hope that helps. I thought ocaml-setup should take care of modifying .profile/.bashrc...

I can't resurrect this PR anymore? I committed it on its branch, but it's not showing those commits anymore after the merge...

@michael-schwarz
Copy link
Member

https://github.com/goblint/analyzer/pull/310/checks?check_run_id=3212969178 fails with

Run git reset --soft 0000000000000000000000000000000000000000 && eval $(opam env) && ./scripts/hooks/pre-commit
  git reset --soft 0000000000000000000000000000000000000000 && eval $(opam env) && ./scripts/hooks/pre-commit
  shell: /usr/bin/bash -e {0}
  env:
    OPAMJOBS: 4
    OPAMYES: 1
fatal: Could not parse object '0000000000000000000000000000000000000000'.
Error: Process completed with exit code 128.

@vogler
Copy link
Collaborator Author

vogler commented Aug 2, 2021

Strange that it's all 0.
${{ github.event.before }}: 'The SHA of the most recent commit on ref before the push.'(doc).
Looks like you pushed 2 commits on Aug 1, and there's a commit before on Jun 16, correct? So it should evaluate to this commit instead of all 0.
I could change it to fallback to HEAD~ or prob. better HEAD^ if it's all 0, but it'd be better to know what's the reason for it first.
Is this before scoped to a workflow and it's 0 because there was no indentation check before? But then it should fail for any new commit and I think other branches were fine.
Was there a force push, branch rename or something like that?

@michael-schwarz
Copy link
Member

branch rename

Yes, I renamed the branch, but that was entirely local before it was published to GitHub. I'm not sure if this is visible to GitHub even, one would think the name of the local branch should not matter...
But maybe this is the cause

@sim642
Copy link
Member

sim642 commented Aug 3, 2021

If the branch was new on GitHub, then there was no most recent commit on it (the ref) before the push. Technically there were the commits on master that it merges, but maybe the actions events are directly branch-based. Or maybe they just don't follow merges because it would make the value ambiguous.

This is probably more work but there's also this:

Key Type Description
commits[][distinct] boolean Whether this commit is distinct from any that have been pushed before.

@vogler
Copy link
Collaborator Author

vogler commented Aug 3, 2021

Just confirmed on https://github.com/goblint/analyzer/runs/3230967934 that the first push on a new branch has all 0.

@vogler
Copy link
Collaborator Author

vogler commented Sep 2, 2021

0ce4a7d#diff-6491407272b29329ba5425cc471e9018652b90c17e6581dd1c0769eeb8a7f56dR16
This means the first push on a new branch won't be checked but only the following changes?

run: git reset --soft ${{ github.event.before }} && eval $(opam env) && ./scripts/hooks/pre-commit

Did you try git reset --soft HEAD^ if it's all 0?

@sim642
Copy link
Member

sim642 commented Sep 3, 2021

This means the first push on a new branch won't be checked but only the following changes?

I suppose so. I just wanted those zero commit failures to go away because they made the indentation workflow too easy to ignore for false alarms.

Did you try git reset --soft HEAD^ if it's all 0?

That's not fully accurate either because you might push multiple commits to a new branch, but this would only consider the last of those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
setup deps, CI, release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants