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

[clojure] Add linter cljstyle #2115

Merged

Conversation

practicalli-johnny
Copy link
Contributor

@practicalli-johnny practicalli-johnny commented Nov 29, 2022

Proposed Changes

Add cljstyle to check and fix the format of Clojure code

As discussed in #2011

Followed the Megalinger contributing steps

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

Add cljstyle to check and fix the format of Clojure code

As discussed in oxsecurity#2011
@codecov-commenter
Copy link

Codecov Report

Merging #2115 (98f6859) into main (a927ca4) will increase coverage by 0.67%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2115      +/-   ##
==========================================
+ Coverage   81.71%   82.39%   +0.67%     
==========================================
  Files         158      158              
  Lines        3413     3413              
==========================================
+ Hits         2789     2812      +23     
+ Misses        624      601      -23     
Impacted Files Coverage Δ
megalinter/reporters/UpdatedSourcesReporter.py 89.74% <0.00%> (+2.56%) ⬆️
...alinter/tests/test_megalinter/helpers/utilstest.py 89.01% <0.00%> (+8.05%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nvuillam
Copy link
Member

@practicalli-john looks great, but you need to add test files then build the project so it generates test classes :)

https://megalinter.io/latest/contributing/#add-a-new-linter

@practicalli-johnny
Copy link
Contributor Author

practicalli-johnny commented Nov 30, 2022

@practicalli-john looks great, but you need to add test files then build the project so it generates test classes :)

https://megalinter.io/latest/contributing/#add-a-new-linter

There are two clojure test files already in https://github.com/oxsecurity/megalinter/tree/main/.automation/test/clojure

  • clojure_bad_1.clj fails when running cljstyle check clojure_bad_1.clj
  • clojure_good_1.clj passes when running cljstyle check clojure_good_1.clj

After running build.sh (although doc says bash build.py which does not exist) a test class does seem to have been created for cljstyle megalinter/tests/test_megalinter/linters/clojure_cljstyle_test.py

Do I need to provide anything else?

Should I add more tests to the existing clojure test files or write another good / bad set of test files specifically for cljstyle?

There are more tests I could think up over the next week or so, but its unclear if I am testing all the rules or simply testing if a linter is passing a good test and failing a bad test.

I am unclear how MegaLinter knows which test files are for which linter, I assume it tries all good / bad files with any linter activated, is that correct?

I've looked at the Markdown tests, but not really any the wiser, sorry.

@practicalli-johnny
Copy link
Contributor Author

practicalli-johnny commented Nov 30, 2022

Should commit everything that is generated by build.sh ?

I'll push them as a separate commit to this PR and remove them if they are not wanted.

@echoix
Copy link
Collaborator

echoix commented Dec 1, 2022

Should commit everything that is generated by build.sh ?

Yep, that way the docs are updated to match the changes brought by the PR, and all the Dockerfiles, and other files to build the images are in sync with the changes in the definitions.

I know, build artifacts committed seems weird, but here the build artifacts are really the images, and the build scripts are a tool to parse the collection of linters and create the many Dockerfiles and all the code needed to wrap the different linters together.

All auto-generated files from running `build.sh`
@practicalli-johnny
Copy link
Contributor Author

practicalli-johnny commented Dec 9, 2022

The automated files have been committed

Clojure linter tests have been updated and extended for clj-kondo and cljstyle

Unfortunately the libz.so.1 library seems to be missing from the Operating System used in the MegaLinter checks. cljstyle tests are failing with the warning:

❌ Linted [CLOJURE] files with [cljstyle]: Found 2 error(s) - (0.0s)
- Using [cljstyle vERROR] https://megalinter.io/20efb375515216797c6a762ed45688042868ef73/descriptors/clojure_cljstyle
- MegaLinter key: [CLOJURE_CLJSTYLE]
- Rules config: identified by [cljstyle]
[cljstyle] .automation/test/clojure/clojure_good_1.clj - ERROR - 1 error(s)
--Error detail:
cljstyle: error while loading shared libraries: libz.so.1: cannot open shared object file: No such file or directory

Assumption: adding zlib pakage seems to provide libz.so.1 library, to alpine linux OS will enable cljstyle to run correctly.
Alternatively, zlib-dev package

@Kurt-von-Laven
Copy link
Collaborator

I think it’s correct to add installation of zlib to the MegaLinter Dockerfiles in this PR. Be warned that most of the Dockerfiles are generated by a build script that needs to be run manually.

@practicalli-johnny practicalli-johnny marked this pull request as draft December 9, 2022 17:12
@practicalli-johnny
Copy link
Contributor Author

Converted PR to draft until I have time to figure out how to add libz.so.1 from the zlib Alpine Linux package.

I havent found enough documentation to proceed and this is taking days to figure out. I've review when I have more time.

@practicalli-johnny practicalli-johnny force-pushed the clojure-linters-add-cljstyle branch 2 times, most recently from 5177e40 to 47138e7 Compare December 17, 2022 19:18
@practicalli-johnny
Copy link
Contributor Author

Build & Deploy - DEV/Tests+Deploy Docker Image - Dev (pull_request) action failed to build in what seemed to be unrelated to the Clojure or Java images. So a comment was added to the clojure.megalinger-descriptor.yml and changed pushed to re-run the GitHub actions.

@practicalli-johnny
Copy link
Contributor Author

practicalli-johnny commented Dec 17, 2022

cljstyle still failing to find libz.so.1 - considering using a different Docker image to copy the cljstyle from, or have a RUN command to copy and extract straight from the cljstyle repository

Initial action is to add zlib-dev package as well and see if that makes any difference. Also update wording of comment so spell check does not fail

Failing output for cljstyle

MegaLinter now collects the files to analyse
Listing all files in directory [/tmp/lint/.automation/test/clojure], then filter with:
Root dir content:
- /tmp/lint/.automation/test/clojure/clojure_bad_1.clj
- /tmp/lint/.automation/test/clojure/clojure_bad_2.clj
- /tmp/lint/.automation/test/clojure/clojure_good_1.clj
- /tmp/lint/.automation/test/clojure/clojure_good_2.clj
All found files before filtering:
- /tmp/lint/.automation/test/clojure/clojure_bad_1.clj
- /tmp/lint/.automation/test/clojure/clojure_bad_2.clj
- /tmp/lint/.automation/test/clojure/clojure_good_1.clj
- /tmp/lint/.automation/test/clojure/clojure_good_2.clj
- /tmp/lint/.automation/test/clojure/reports/ERROR-CLOJURE_CLJ_KONDO.txt
- /tmp/lint/.automation/test/clojure/reports/SUCCESS-CLOJURE_CLJ_KONDO.txt
- /tmp/lint/.automation/test/clojure/reports/expected-CLOJURE.tap.ignored
- File extensions: .clj, .cljc, .cljs, .edn
- Including regex: (good)
- Excluding regex: \/megalinter-reports\/
Unable to list git ignored files ()
Kept [2] files on [7] found files
Kept files before applying linter filters:
- /tmp/lint/.automation/test/clojure/clojure_good_1.clj
- /tmp/lint/.automation/test/clojure/clojure_good_2.clj
[Filters] {'name': 'CLOJURE_CLJSTYLE', 'filter_regex_include': None, 'filter_regex_exclude': None, 'files_sub_directory': None, 'lint_all_files': False, 'lint_all_other_linters_files': False, 'file_extensions': ['.clj', '.cljs', '.cljc', '.edn'], 'file_names_regex': [], 'file_names_not_ends_with': [], 'file_contains_regex': []}
CLOJURE_CLJSTYLE linter kept 2 files after applying linter filters:
- /tmp/lint/.automation/test/clojure/clojure_good_1.clj
- /tmp/lint/.automation/test/clojure/clojure_good_2.clj

+----MATCHING LINTERS---+-----------------------+----------------+------------+
| Descriptor | Linter   | Criteria              | Matching files | Format/Fix |
+------------+----------+-----------------------+----------------+------------+
| CLOJURE    | cljstyle | .clj|.cljs|.cljc|.edn | 2              | no         |
+------------+----------+-----------------------+----------------+------------+

MegaLinter flavor is "all", no need to check match with linters
[cljstyle] command: ['cljstyle', '/tmp/lint/.automation/test/clojure/clojure_good_1.clj']
[cljstyle] CWD: /
[cljstyle] result: 127 cljstyle: error while loading shared libraries: libz.so.1: cannot open shared object file: No such file or directory

[cljstyle] command: ['cljstyle', '/tmp/lint/.automation/test/clojure/clojure_good_2.clj']
[cljstyle] CWD: /
[cljstyle] result: 127 cljstyle: error while loading shared libraries: libz.so.1: cannot open shared object file: No such file or directory

Linter version command: ['/usr/bin/cljstyle', '--version']
Linter version result: 127 /usr/bin/cljstyle: error while loading shared libraries: libz.so.1: cannot open shared object file: No such file or directory

Unable to get version for linter [cljstyle]
/usr/bin/cljstyle --version returned output: (127) /usr/bin/cljstyle: error while loading shared libraries: libz.so.1: cannot open shared object file: No such file or directory

Unable to extract version with regex re.compile('\\d+(\\.\\d+)+') from ERROR
❌ Linted [CLOJURE] files with [cljstyle]: Found 2 error(s) - (0.0s)
- Using [cljstyle vERROR] https://megalinter.io/b409df4f594267ecf02a7f7dfa42ca4a2f5a662c/descriptors/clojure_cljstyle
- MegaLinter key: [CLOJURE_CLJSTYLE]
- Rules config: identified by [cljstyle]
[cljstyle] .automation/test/clojure/clojure_good_1.clj - ERROR - 1 error(s)
--Error detail:
cljstyle: error while loading shared libraries: libz.so.1: cannot open shared object file: No such file or directory

[cljstyle] .automation/test/clojure/clojure_good_2.clj - ERROR - 1 error(s)
--Error detail:
cljstyle: error while loading shared libraries: libz.so.1: cannot open shared object file: No such file or directory


[Text Reporter] Generated TEXT report: /tmp/f9b77b3f-0d96-4c95-8737-d9e3915ff24c/linters_logs/ERROR-CLOJURE_CLJSTYLE.log
[Post] No commands declared in user configuration
[Azure Comment Reporter] No Azure Token found, so skipped post of PR comment

+----SUMMARY-+----------+------+-------+-------+--------+--------------+
| Descriptor | Linter   | Mode | Files | Fixed | Errors | Elapsed time |
+------------+----------+------+-------+-------+--------+--------------+
| ❌ CLOJURE | cljstyle | file |     2 |       |      2 |         0.0s |
+------------+----------+------+-------+-------+--------+--------------+

@nvuillam
Copy link
Member

nvuillam commented Dec 18, 2022

@practicalli-john if you need additional stuff , like apk packages, just add an apk property with package identifiers in install property of the descriptor, then run bash build.sh and it will automatically update dockerfiles

Maybe the following ?

install:
  apk:
    - zlib

@practicalli-johnny
Copy link
Contributor Author

Adding zlib and zlib-dev packages have not worked. I'll revisit this issue at the weekend.

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this pull request should stay open, please remove the O: stale 🤖 label or comment on the pull request.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Jan 19, 2023
@github-actions github-actions bot closed this Feb 3, 2023
@nvuillam nvuillam reopened this Feb 3, 2023
@nvuillam nvuillam removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Feb 3, 2023
@practicalli-johnny
Copy link
Contributor Author

Due to illness I haven't looked at this PR for a while. Hopefully I will pick it up later in February

I don't know why the cljstyle binary is failing to run.

I will try to create a new docker image that contains cljstyle (and maybe other Clojure linters like zprint) and see if that improves the issue

I also am not sure if I am passing in the cljstyle check or fix command option when MegaLinter is calling cljstyle

@nvuillam
Copy link
Member

nvuillam commented Feb 3, 2023

@practicalli-john Take your time, with open source there is no deadline ^^
I'll try to also have a look about the issues when i find time !

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2023

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this pull request should stay open, please remove the O: stale 🤖 label or comment on the pull request.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Mar 6, 2023
@github-actions github-actions bot closed this Mar 20, 2023
@bdovaz bdovaz reopened this Mar 20, 2023
@bdovaz bdovaz removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Mar 20, 2023
@nvuillam
Copy link
Member

@practicalli-john I added some commits to try to revive this PR :)

@practicalli-johnny
Copy link
Contributor Author

Thank you for the commit updates.

I've had an issue running the tests locally. I modified the example script to run tests in a container and the script is failing

"docker run" requires at least 1 argument.
See 'docker run --help'.

The script used:

docker buildx build -f linters/clojure_cljstyle/Dockerfile . --tag clojure_cljstyle
TEST_KEYWORDS_TO_USE="clojure_cljstyle"
docker run -e TEST_CASE_RUN=true -e OUTPUT_DETAIL=detailed -e TEST_KEYWORDS="${TEST_KEYWORDS_TO_USE}" -e MEGALINTER_VOLUME_ROOT="." -v "/var/run/docker.sock:/var/run/docker.sock:rw" -v $(pwd):/tmp/clojure_cljstyle

I'm using Ubuntu Linux 22.04 and Docker Desktop 4.18.0 (104112)

Full Output

The full output from running the script

❯ bash test-cljstyle-linter.sh
[+] Building 3.8s (26/26) FINISHED                                                                                                                                            
 => [internal] load build definition from Dockerfile                                                                                                                     0.0s
 => => transferring dockerfile: 32B                                                                                                                                      0.0s
 => [internal] load .dockerignore                                                                                                                                        0.0s
 => => transferring context: 35B                                                                                                                                         0.0s
 => resolve image config for docker.io/docker/dockerfile:1                                                                                                               1.9s
 => [auth] docker/dockerfile:pull token for registry-1.docker.io                                                                                                         0.0s
 => CACHED docker-image://docker.io/docker/dockerfile:1@sha256:39b85bbfa7536a5feceb7372a0817649ecb2724562a38360f4d6a7782a409b14                                          0.0s
 => [internal] load build definition from Dockerfile                                                                                                                     0.0s
 => [internal] load .dockerignore                                                                                                                                        0.0s
 => [internal] load metadata for docker.io/library/python:3.11.3-alpine3.17                                                                                              1.5s
 => [auth] library/python:pull token for registry-1.docker.io                                                                                                            0.0s
 => [ 1/16] FROM docker.io/library/python:3.11.3-alpine3.17@sha256:507818d46649f8543e58d19a00e3a1a428bb7e87c0bf7f7d1ffe7b076cda11be                                      0.0s
 => [internal] load build context                                                                                                                                        0.0s
 => => transferring context: 21.41kB                                                                                                                                     0.0s
 => CACHED [ 2/16] RUN apk add --update --no-cache                 bash                 ca-certificates                 curl                 gcc                 git     0.0s
 => CACHED [ 3/16] RUN mkdir -p /go/src /go/bin || true &&     yarn config set ignore-engines true || true                                                               0.0s
 => CACHED [ 4/16] RUN curl --retry 5 --retry-delay 5 -sLO https://raw.githubusercontent.com/greglook/cljstyle/main/script/install-cljstyle     && chmod +x install-clj  0.0s
 => CACHED [ 5/16] COPY megalinter /megalinter                                                                                                                           0.0s
 => CACHED [ 6/16] RUN PYTHONDONTWRITEBYTECODE=1 python /megalinter/setup.py install     && PYTHONDONTWRITEBYTECODE=1 python /megalinter/setup.py clean --all     && rm  0.0s
 => CACHED [ 7/16] COPY megalinter/descriptors /megalinter-descriptors                                                                                                   0.0s
 => CACHED [ 8/16] COPY TEMPLATES /action/lib/.automation                                                                                                                0.0s
 => CACHED [ 9/16] RUN mkdir /root/docker_ssh && mkdir /usr/bin/megalinter-sh                                                                                            0.0s
 => CACHED [10/16] COPY entrypoint.sh /entrypoint.sh                                                                                                                     0.0s
 => CACHED [11/16] COPY sh /usr/bin/megalinter-sh                                                                                                                        0.0s
 => CACHED [12/16] COPY sh/megalinter_exec /usr/bin/megalinter_exec                                                                                                      0.0s
 => CACHED [13/16] COPY sh/motd /etc/motd                                                                                                                                0.0s
 => CACHED [14/16] RUN find /usr/bin/megalinter-sh/ -type f -iname "*.sh" -exec chmod +x {} ; &&     chmod +x entrypoint.sh &&     chmod +x /usr/bin/megalinter_exec &&  0.0s
 => CACHED [15/16] RUN export STANDALONE_LINTER_VERSION="$(python -m megalinter.run --input /tmp --linterversion)" &&     echo $STANDALONE_LINTER_VERSION                0.0s
 => exporting to image                                                                                                                                                   0.1s
 => => exporting layers                                                                                                                                                  0.0s
 => => writing image sha256:67203c6edb86c5b2e3e6b46190b2f53fdb227da40ac2b0729c2ffa1ea59d4266                                                                             0.0s
 => => naming to docker.io/library/clojure_cljstyle                                                                                                                      0.0s
"docker run" requires at least 1 argument.
See 'docker run --help'.

Usage:  docker run [OPTIONS] IMAGE [COMMAND] [ARG...]

Create and run a new container from an image

@nvuillam nvuillam marked this pull request as ready for review May 13, 2023 18:14
Copy link
Member

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

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

6 months later... we did it ! 😃

Thanks for the great PR @practicalli-john :)

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.

6 participants