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

GitHub Actions: check editorconfig in PRs #87853

Merged
merged 1 commit into from May 20, 2020
Merged

GitHub Actions: check editorconfig in PRs #87853

merged 1 commit into from May 20, 2020

Conversation

@zowoq
Copy link
Contributor

zowoq commented May 15, 2020

Initial discussion in #86376 (comment)

Currently this is a completely untested draft.

@grahamc
Copy link
Member

grahamc commented May 15, 2020

I don't think the build is a good idea. We used to use TravisCI to do some builds, but almost every PR had failed bulids because it was too slow and nixpkgs builds were too big. Maybe instead ofborg could build the manual by default?

@zowoq
Copy link
Contributor Author

zowoq commented May 15, 2020

Maybe instead ofborg could build the manual by default?

👍 Whatever works best! I'll drop it from this PR.

@grahamc
Copy link
Member

grahamc commented May 15, 2020

For the editorconfig check, is there a way for users to do that validation without pushing to github? I guess I'm wondering how we'll know if we're going to merge this and have everything be broken.

@zowoq
Copy link
Contributor Author

zowoq commented May 15, 2020

is there a way for users to do that validation without pushing to github?

Pre-commit hook maybe? But then it's more of a suggestion rather than a requirement?

I guess I'm wondering how we'll know if we're going to merge this and have everything be broken.

This (should) only check files changed in PRs against master branch so it shouldn't break anything?

@grahamc
Copy link
Member

grahamc commented May 15, 2020

I'm not sure, over time it would require every file be updated to match. I'm not saying this is a bad thing or undesirable, but it is maybe a bigger change than you might be expecting :). For example, the first patch to all-packages.nix would probably be a big one!

@zowoq
Copy link
Contributor Author

zowoq commented May 15, 2020

Yeah, it will probably take a bit of work to get the repo cleaned up enough that we can merge this without breaking every PR but I think most of the changes can be made automatically and submitted in small batches for easier review?

all-packages.nix doesn't seem to need any changes to comply with the current .editorconfig.
indent issues.

@zowoq zowoq force-pushed the zowoq:actions branch from 203fe05 to fa914bf May 15, 2020
@zowoq zowoq changed the title GitHub Actions: editorconfig and manual GitHub Actions: check editorconfig in PRs May 15, 2020
@zowoq
Copy link
Contributor Author

zowoq commented May 15, 2020

indent_size for .nix files seems to be the the biggest problem by far:

nixpkgs/.editorconfig

Lines 17 to 19 in f6bfb37

[*.{nix,rb,xml}]
indent_style = space
indent_size = 2

I'd suggest disabling it for nix files as nixpkgs-fmt might be the better tool for this as lots of these files have somewhat odd patterns.

Using actions to enforce everything for non-nix files and indent_style/newlines/whitespace in nix files seems like decent win?

@Mic92
Copy link
Contributor

Mic92 commented May 15, 2020

indent_size for .nix files seems to be the the biggest problem by far:

nixpkgs/.editorconfig

Lines 17 to 19 in f6bfb37

[*.{nix,rb,xml}]
indent_style = space
indent_size = 2

I'd suggest disabling it for nix files as nixpkgs-fmt might be the better tool for this as lots of these files have somewhat odd patterns.

It is probably the most useful knob in this file as it sets up the tabwidth correctly (instead of 4 spaces). Is there a way to make to ignore it in CI?

@Mic92
Copy link
Contributor

Mic92 commented May 15, 2020

Also cc @zimbatm and @Rizary
nix-community/nixpkgs-fmt#56
who want to integrate nixpkgs-fmt into nixpkgs.

@zowoq
Copy link
Contributor Author

zowoq commented May 15, 2020

Is there a way to make to ignore it in CI?

Doesn't look like this is possible with any of the actions available via github marketplace.

We could make our own linter using editorconfig-checker or similar but I don't want to start on that unless there is a consensus that we'll actually going to use it.

@Mic92
Copy link
Contributor

Mic92 commented May 15, 2020

Is there a way to make to ignore it in CI?

Doesn't look like this is possible with any of the actions available via github marketplace.

We could make our own linter using editorconfig-checker or similar but I don't want to start on that unless there is a consensus that we'll actually going to use it.

How about just patching .editorconfig file in a step before the github ci check runs?

@zowoq
Copy link
Contributor Author

zowoq commented May 15, 2020

How about just patching .editorconfig file in a step before the github ci check runs?

😆 Yeah, that should work.

@Mic92
Mic92 approved these changes May 15, 2020
@zowoq
Copy link
Contributor Author

zowoq commented May 15, 2020

Hmmm, might be a good idea to do the same thing for docbook as well.

Even if we "fix" all of them first, using this to enforce a style without ofborg checking that it still builds seems like it might cause problems.

@Mic92
Copy link
Contributor

Mic92 commented May 15, 2020

@GrahamcOfBorg if I re-call the problem with travis-ci was that we tried to build every package changed by a pr, including mass-rebuilds. Building only the manual seems like a constant-size amount of work.

@zowoq
Copy link
Contributor Author

zowoq commented May 15, 2020

I guess the editorconfig and the manual build (if we add it here) would need to also check PRs made against staging (and staging-next maybe?) so that those changes don't end up being a mess that needs to be dealt with by whoever tries to merge staging-next back to master.

Keeping the config in sync across branches might be annoying as well, at least initially until it stabilises.

I suppose this something that nixpkgs-fmt would need to deal with as well.

@jtojnar
Copy link
Contributor

jtojnar commented May 15, 2020

DocBook is actually indented with a single space (default value for xmlformat).

@Mic92
Copy link
Contributor

Mic92 commented May 15, 2020

Let's leave manual builds for a different PR.

@zimbatm
Copy link
Member

zimbatm commented May 15, 2020

Also cc @zimbatm and @Rizary who want to integrate nixpkgs-fmt into nixpkgs.

IMO both are complementary. As long as the indent for nix is set to 2 spaces both tools shouldn't conflict. So 👍 for this initiative.

@zowoq zowoq force-pushed the zowoq:actions branch from 233fba6 to a66d4f1 May 15, 2020
@zowoq
Copy link
Contributor Author

zowoq commented May 16, 2020

I've split the docbook indent size out into #87919 so it can be merged without waiting for this.

@Mic92 Mic92 merged commit acfcd44 into NixOS:master May 20, 2020
1 check was pending
1 check was pending
grahamcofborg-eval Checking new out paths
Details
@zowoq zowoq deleted the zowoq:actions branch May 20, 2020
@zowoq
Copy link
Contributor Author

zowoq commented May 20, 2020

haskell-modules/hackage-packages.nix has 230755 indentation errors + 66 whitespace errors.

The rest of the repo only has 19455 errors total including indentation errors.

So I guess it either needs to be excluded from this check or fixed in hackage2nix somehow.

Opened a draft PR for fixes. #88431

Opened another draft PR for the manuals. #88488

@zowoq zowoq mentioned this pull request May 21, 2020
@marcus7070
Copy link
Contributor

marcus7070 commented May 22, 2020

Could someone with access consider adding a checkbox in the PR template about editorconfig? The first I heard about it was when my PR failed (and GitHub failed to display the error log), which wasn't the nicest first impression.

@Mic92
Copy link
Contributor

Mic92 commented May 22, 2020

Do you really want more checkboxes? What was not clear about the error message given by the check? Maybe we should improve on that so that in the common case we don't overwhelm contributors with checkboxes.

@marcus7070
Copy link
Contributor

marcus7070 commented May 22, 2020

The error message did not load. Just a spinning little GitHub symbol even though the test was finished and failed. I could access both the earlier setup logs and the later postrun actions logs, but not the actual "editorconfig check" logs. Sorry I can't be more specific, I've fixed and force pushed my PR and can't get back to the error message. I can see editorconfig error messages on other PRs, so it was something specific to mine.

Incase it helps, the error message should have been something along the lines of

pkgs/development/python-modules/cadquery/default.nix:
	25: Trailing whitespace
	38: Trailing whitespace
	71: Trailing whitespace
	71: Wrong amount of left-padding spaces(want multiple of 2)
	78: Trailing whitespace
	78: Wrong amount of left-padding spaces(want multiple of 2)
	82: Trailing whitespace
	82: Wrong amount of left-padding spaces(want multiple of 2)
	95: Trailing whitespace
	95: Wrong amount of left-padding spaces(want multiple of 2)
	97: Trailing whitespace
	97: Wrong amount of left-padding spaces(want multiple of 2)

12 errors found
@Mic92
Copy link
Contributor

Mic92 commented May 22, 2020

I also just tested it in #88594

This is the error message:

default.nix:
	27: Trailing whitespace

1 errors found
##[error]Process completed with exit code 1.

We maybe could rename it from check editorconfig to check codingstyle based on editorconfig to make it a bit more clearer as well as add a command to locally reproduce this error. Apart from that the error message looks understandable to me.

@marcus7070
Copy link
Contributor

marcus7070 commented May 22, 2020

Just found an email with a link to the logs: https://github.com/NixOS/nixpkgs/runs/698397328
They work fine now.

add a command to locally reproduce this error

Yeah, that's the most important part to me.

@LnL7
Copy link
Member

LnL7 commented May 22, 2020

This spawned a bunch of discussion in #nixos-dev yesterday.

The problem here is that now github actions and oforg both have their own unrelated checks which means if one or the other doesn't run everything still looks good (which happened today). I'm not aware of any particular problems with GitHub actions but after giving it some thought I'm not in favour of depending on multiple CI systems for status checks.

@grahamc
Copy link
Member

grahamc commented May 22, 2020

I would like to revert this PR and close #88488. Not because I don't like github actions or feel like my pet project ofborg is threatened, but because I think these checks are actually harming the clearness of the checks. I'll explain:

The editorconfig check puts a big green check next to a commit, and hides the list of checks which passed. If ofborg is down or behind, it is likely people will think ofborg has approved the PR before it did. This could cause, well, all the problems ofborg is there to prevent.

The concrete problem here is this green check appears very early and is misleading, and there is no poka yoke to help people do the right thing.

One fear of #88488 is that it will introduce red X's if it takes a long time to build the manual, or something in the target branch is failing to build. This is reminiscent of the Travis times for me: Travis gave out so few green checkmarks, that if Travis gave a PR a green check-mark, it inspired more "okay what is broken that let travis pass it?" than "okay good to merge".

The goal of ofborg, and ever since ofborg became a thing, the "rule" has been green checkmark -> good to merge and red X -> don't merge.

The editorconfig check errodes the clearness of the green checkmark signal.

The big-rebuild failures we'll certainly have with #88488 errodes the clearness of the red X signal.

The clearness of this signal has been my guiding light on the design of ofborg for years now, and I am very afraid of threatening it.

A few suggestions that have been brought up to improve this:

  1. make ofborg faster I'd love to do this, but it doesn't solve this problem if ofborg is seriously delayed or down.
  2. require the ofborg evaluation check for merges this is really the only solution, but not something we can do today, because of some workflow issues we need to resolve.

That makes me think that, today, we should be undoing this check and closing #88488.

@zimbatm
Copy link
Member

zimbatm commented May 22, 2020

A related fix is to enable this option in the branch protections:

image

One of the benefits of doing that is that it lets the user know in advance, before ofborg or GitHub Actions has even executed, which checks are needed to pass before they can merge the PR.

@grahamc
Copy link
Member

grahamc commented May 22, 2020

That is not an option right now, @zimbatm. Let's talk about this later? I have information about this, but not a schedule compatible with spending much time on it right now.

@grahamc
Copy link
Member

grahamc commented May 22, 2020

note I included that in my message:

require the ofborg evaluation check for merges this is really the only solution, but not something we can do today, because of some workflow issues we need to resolve.

Mic92 added a commit to Mic92/nixpkgs that referenced this pull request May 22, 2020
We cannot run this check now, as it gives marks CI as green even though ofborg has not evaluated it yet.
In future we might be able to mark ofborg as a required test:
NixOS#87853 (comment)
Mic92 added a commit to Mic92/nixpkgs that referenced this pull request May 22, 2020
We cannot run this check now, as it marks CI as green even though ofborg has not
evaluated it yet. In future we might be able to mark ofborg as a required test:
NixOS#87853 (comment)
zimbatm pushed a commit that referenced this pull request May 22, 2020
…88608)

We cannot run this check now, as it marks CI as green even though ofborg has not
evaluated it yet. In future we might be able to mark ofborg as a required test:
#87853 (comment)
@Mic92
Copy link
Contributor

Mic92 commented May 22, 2020

Maybe we can integrate this check into ofborg itself.

@zimbatm
Copy link
Member

zimbatm commented May 22, 2020

Got reverted in #88608

Sorry @zowoq, I know you put effort into this. Do you mind opening a new PR to re-introduce it? I still think it's a good idea to use it.

@zowoq
Copy link
Contributor Author

zowoq commented May 22, 2020

Seems I picked a bad time to step away from a computer for a couple of hours.

Do you mind opening a new PR to re-introduce it?

That is not an option right now ... I have information about this, but not a schedule compatible with spending much time on it right now.

I think I'll wait to hear more from @grahamc first before doing anything.

Thanks @Mic92 and everyone else for you help on this.

@grahamc
Copy link
Member

grahamc commented May 22, 2020

@emilazy
Copy link
Member

emilazy commented May 22, 2020

Would it be possible to have ofborg always run some "additional checks" build specified in nixpkgs? Then we could add things like the manual build and editorconfig checks within the nixpkgs repo as Nix derivations run for every PR without relying on changing/redeploying ofborg.

@Mic92
Copy link
Contributor

Mic92 commented May 22, 2020

Makes sense probably even some of the existing once could be moved over from being hard coded in ofborg to nixpkgs.

@zowoq
Copy link
Contributor Author

zowoq commented Jun 28, 2020

I'd like to look at adding checks for formatting/linting in non-nix files as well, currently we have ~20000 loc in ~500 bash/sh files, ~6500 loc in ~40 python files and ~5000 loc in ~30 perl files.

@grahamc Do you know when you'll be able to provide some detail on the workflow issues that are blocking us from using the required status checks with ofborg?

@Mic92
Copy link
Contributor

Mic92 commented Jun 28, 2020

The alternative would be to have ofborg to build derivations described in nixpkgs itself so that we can add features to the CI without having to modify ofborg itself.

@zowoq
Copy link
Contributor Author

zowoq commented Jun 28, 2020

The alternative would be to have ofborg to build derivations described in nixpkgs itself so that we can add features to the CI without having to modify ofborg itself.

TBH, I don't think adding more things (making us more dependant) on ofborg is a good idea.

@Mic92
Copy link
Contributor

Mic92 commented Jun 29, 2020

I don't see a different simple way of fixing the UX problem though. ofborg could run in github actions, however this requires major refactorings I believe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.