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.

Sign up for GitHub

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

Jump to bottom

pre-commit-clang-format not working on windows #370

Open
pkspyder007 opened this issue Jan 17, 2023 · 13 comments · May be fixed by #390
Open

pre-commit-clang-format not working on windows #370

pkspyder007 opened this issue Jan 17, 2023 · 13 comments · May be fixed by #390
Labels
bug Something isn't working devops Related to DevOps infrastructure of the Core good first issue Good for newcomers

Comments

@pkspyder007
Copy link
Contributor

🐛 Bug Report

Fix pre-commit-clang-format git hook to detect clang on windows and add v15 to the major version list.

Expected Behavior

Current Behavior

Clang-format is not detected even being installed already
image

image

Possible Solution

For adding v15 just add it to the this line https://github.com/metacall/core/blob/develop/githooks/pre-commit-clang-format#L27

Regarding the hook not detecting the clang-format executable, the current script is in bash if it could be ported to a .bat or .ps1 script for windows.
@pkspyder007 pkspyder007 added the bug Something isn't working label Jan 17, 2023
@giarve giarve added the good first issue Good for newcomers label Jan 17, 2023
@giarve
Copy link
Member

giarve commented Jan 17, 2023

Worth keeping up to date with https://github.com/godotengine/godot/blob/master/misc/hooks/pre-commit-clang-format,

as they already solved adding v15.

@viferga
Copy link
Member

viferga commented Jan 17, 2023

Also making the script polyglot should be a nice option too: https://gist.github.com/prail/24acc95908e581722c0e9df5795180f6

@viferga viferga added the devops Related to DevOps infrastructure of the Core label Jan 18, 2023
@Coder-Manan
Copy link

Will porting the bash script to .ps1 or .bat fix the issue?

@pkspyder007
Copy link
Contributor Author

@Coder-Manan check this out #370 (comment)
@giarve can answer this better

@parteekcoder
Copy link

@viferga is this issue solved or not ?
if not please assign it to me

@viferga
Copy link
Member

viferga commented Feb 3, 2023

It is not resolved, try to port the bash script to ps1 or bat, then we will add a polyglot launcher to make it work in mac/linux and windows, as suggested in the issue.

@viferga
Copy link
Member

viferga commented Feb 3, 2023

Will porting the bash script to .ps1 or .bat fix the issue?

Yes, but we will need to add a common launcher for multiple platforms.

@viferga
Copy link
Member

viferga commented Feb 3, 2023

@parteekcoder I am not going to assign this to anybody, feel free to PR it if you want, and I will close the issue, this is open to anybody to be resolved.

@Samir433
Copy link

is this issue solved or not ?
if not please assign it to me

@pkspyder007
Copy link
Contributor Author

@Samir433 you can work on it if you want. If your solution is good we'll merge it.
#370 (comment)

@Coder-Manan Coder-Manan linked a pull request Feb 26, 2023 that will close this issue
Draft
12 tasks
@Coder-Manan
Copy link

in the find_clang_format function should I check only for versions 11-15 or for any version >= 11? (I am porting the script to ps script)

@adityathapa009
Copy link

for windows
repos:

  • repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.0.1
    hooks:
    • id: trailing-whitespace
    • id: end-of-file-fixer
    • id: check-yaml
    • id: check-json
    • id: check-executables-have-shebangs
    • id: check-astro
    • id: check-bash
    • id: check-symlinks
    • id: check-secure-shell
    • id: check-perl
    • id: check-python-attributes
    • id: check-python-variable-arity
    • id: check-json-max-depth
    • id: check-xml
    • id: check-javascript
    • id: check-toml
    • id: check-markdown-ast
    • id: check-for-merge-conflicts
    • id: require-hooks
    • id: check-merge-conflict
    • id: check-casechange
    • id: check-attributes
    • id: check-variable-name-case
    • id: check-yaml-documents
    • id: check-ordered-dict
    • id: check-dikt-style
    • id: transform-ini-extensions
    • id: check-date
    • id: check-all-capitals
      paste above configuration into ".pre-commit-config.yaml"
      file and execute "pre-commmit install" after updating the config..

@Jatin00001
Copy link

If the issue remains unresolved, kindly delegate it to me for further attention and resolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working devops Related to DevOps infrastructure of the Core good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

WIP: fix pre-commit-clang-format not working on Windows Coder-Manan/core
8 participants
@viferga @pkspyder007 @giarve @Coder-Manan @parteekcoder @Samir433 @Jatin00001 @adityathapa009