Check for uncommited changes

[litetex]

What is it?

• Bugfix (user facing)
• Feature (user facing)
• Codebase improvement (dev facing)
• Meta improvement to the project (dev facing)

Description of the changes in your PR

As mentioned in #6708:

It would be a great idea if we could check on the CI if there are any changes like wrong formatted kotlin files that haven't been committed.

This should be doable by extending the GH actions CI file with this
...

Fixes the following issue(s)

• Fixes Check for uncommited changes (like formatting problems with kotlin files) #6708

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

• I read the contribution guidelines.

⚠️ Before merging ⚠️

Make sure that the files on the dev-branch are formatted correctly.

[litetex]

Demo is available at https://github.com/litetex/NewPipe/runs/3107590582?check_suite_focus=true https://github.com/litetex/NewPipe/runs/3151597417?check_suite_focus=true
Code changes to break the build: litetex@d01462c litetex@0e6e1c6

[Stypox]

not applicable

Feel free to remove the whole section ;-)

[XiangRongLin]

There isn't some sort of strict mode which fails on any sort of violation? Violation as in the file is not formatted correctly.

[litetex]

There isn't some sort of strict mode which fails on any sort of violation? Violation as in the file is not formatted correctly.

I don't understand the question completely...

Quick description what happens:
The script just checks if there are any uncommitted changes.
So e.g. if there are wrong formatted kotlin files it detects them - after the build was executed which formats these files - and fails with exit 1 -> the job will be marked as failed

[XiangRongLin]

What I meant is if the linter has a mode in which it fails the build instead of creating those uncommitted changes.
That mode is usually called strict mode

[XiangRongLin]

Also please write meaningfull commit messages and PR titles

[litetex]

@XiangRongLin

What I meant is if the linter has a mode in which it fails the build instead of creating those uncommitted changes.
That mode is usually called strict mode

The build.gradle already uses "strict-mode" (runKtlint in the following snippet). however it always works because the code is getting formatted before:

NewPipe/app/build.gradle

Line 168 in 39722a5

preDebugBuild.dependsOn formatKtlint, runCheckstyle, runKtlint

Maybe we could add a build profile where the formatting is not done in the CI build?
However I have no experience in implementing that in gradle (usually I use maven)

But I think the underlying problem here is that people sometimes disable the linter and checkstyle (because it annoys them) and then commit the changes.

[litetex]

Improved the commit messages 😄

[XiangRongLin]

Can you explain where it says that it's strict mode? From a bit googling it seems there isn't even a strict mode.

---

None the less after some rumonations I came to the idea of adding a step which runs gradle runKtlint before building the app.
By running gradle runKtlint before building the APK one also gets proper error messages of what is not properly linted. The current solutions seems like a hacky workaround

---

Can you also create a showcase PR which shows the build failing because of this.

[litetex]

@XiangRongLin

Can you explain where it says that it's strict mode? From a bit googling it seems there isn't even a strict mode.

runKtlint is the "strict mode"...
If you remove formatKtlint in

NewPipe/app/build.gradle

Line 168 in 39722a5

preDebugBuild.dependsOn formatKtlint, runCheckstyle, runKtlint

the the build results in:

Execution failed for task ':app:runKtlint'.
> Process 'command 'C:\Program Files\Android\Android Studio\jre\bin\java.exe'' finished with non-zero exit value 1

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

when there are any malformed kotlin files.

None the less after some rumonations I came to the idea of adding a step which runs gradle runKtlint before building the app.

Build the app by whom? The CI or the developers?

By running gradle runKtlint before building the APK one also gets proper error messages of what is not properly linted. The current solutions seems like a hacky workaround

No it doesn't (natively) do that. See above (I just see Execution failed for task ':app:runKtlint')
However the script shows you the exact changes that have to be done.

The current script still enforces the following statement - programming language and tool independent (and not just wrong formatted kotlin files).
"The committed files in git should not differ after a build by the CI; detected changes must be ignored (via .gitignore) or committed."

If you still consider it "hacky" please take a look at GitHub's action repositories:

• https://github.com/actions/setup-node/blob/main/.github/workflows/build-test.yml#L30-L32
• https://github.com/actions/setup-node/blob/main/__tests__/verify-no-unstaged-changes.sh
• https://github.com/actions/setup-java/blob/main/.github/workflows/build.yml#L31-L33
• https://github.com/actions/setup-dotnet/blob/main/.github/workflows/workflow.yml#L32-L34

Can you also create a showcase PR which shows the build failing because of this.

I demonstrated the changes already there:
#6709 (comment)

Updated it to also include the new messages 😄

[XiangRongLin]

Build the app by whom? The CI or the developers?

By CI

If you still consider it "hacky" please take a look at GitHub's action repositories:

If the others also see it that way, than I won't object. But I won't approve this (beside only for removing my "requested changes" status)

[Stypox]

I personally would prefer just adding gradle runKtlint step before gradle assembleDebug.

• It takes less lines of code
• It correctly puts out error messages
• When there is a badly-formatted kotlin file the CI fails right away, not after having built everything

[litetex]

I personally would prefer just adding gradle runKtlint step before gradle assembleDebug.

* It takes less lines of code

* It correctly puts out error messages

* When there is a badly-formatted kotlin file the CI fails right away, not after having built everything

That sound like a good and simple idea.
I will give it a try 😄

[litetex]

Tested it out and it looks good:
https://github.com/litetex/NewPipe/runs/3185209865?check_suite_focus=true
https://github.com/litetex/NewPipe/blob/test-2/.github/workflows/ci.yml

However some questions where

• Is something speaking against adding workflow_dispatch: by default to the CI workflow?
  If I do these kinds of PR, I always have to add it manually and change the default branch of my fork.
  Shouldn't do any harm in theory.
  If nothing speaks against it I will open a PR.
• Should we also run checkstyle before building?
• If we go with ./gradlew runKtlint should we keep the git status-check?

[Stypox]

If nothing speaks against it I will open a PR.

I'd say that's ok

Should we also run checkstyle before building?

Nope, that's run just at the beginning of the build. We need to run ktlint before building because otherwise formatKtlint would be run before, but there is no formatCheckstyle, so we are fine

If we go with ./gradlew runKtlint should we keep the git status-check?

I don't think that it's needed

[litetex]

Succeeded by #6808