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 shellcheck and pre-commit to CI jobs #3002

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

terhorstd
Copy link
Contributor

This is an addendum to #2880 adding a pre-commit run also to the CI pre-build checks.
The shellcheck is also added separately to address language specific linters on the same level.

Related PRs are:

@terhorstd terhorstd added T: Enhancement New functionality, model or documentation S: Normal Handle this with default priority I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Nov 27, 2023
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

See comments and suggestions.

run: |
pip install pre-commit

- name: "Run shellcheck..."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: "Run shellcheck..."
- name: "Run pre-commit ..."

.github/workflows/nestbuildmatrix.yml Show resolved Hide resolved
@terhorstd
Copy link
Contributor Author

The checks now are properly added and can be reviewed. The full review contains a ton of white-space changes, so I'd suggest to focus on changes introduced in ac35773 and the following first. There are a couple of intricate changes that were necessary to handle quoted strings correctly, including some bugs like test <cond> && <cond>
The shellcheck pre-commit hook was changed to shellcheck-py to allow also docker-less run on local machines.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

@terhorstd This looks good, but see the few inline comments. Did you run the shell scripts that have been modified significantly and which are not run by the CI, e.g., run_micrcocircuit and run_examples?

.github/workflows/nestbuildmatrix.yml Show resolved Hide resolved
.github/workflows/nestbuildmatrix.yml Outdated Show resolved Hide resolved
doc/htmldoc/static/img/EBRAINS.svg Outdated Show resolved Hide resolved
@heplesser
Copy link
Contributor

I am exploring the test failure on macOS.

@terhorstd
Copy link
Contributor Author

The reason for the MacOS failure is described in https://stackoverflow.com/a/752893. And I think I found a workaround.

@heplesser
Copy link
Contributor

The reason for the MacOS failure is described in https://stackoverflow.com/a/752893. And I think I found a workaround.

Works nicely on macOS now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

2 participants