espressif / esp-protocols

Collection of ESP-IDF components related to networking protocols
165 stars 115 forks source link

Installing pre-commit hooks failed (IDFGH-12997) #592

Closed SohKamYung-Espressif closed 2 weeks ago

SohKamYung-Espressif commented 3 weeks ago

Answers checklist.

What component are you using? If you choose Other, provide details in More Information.

Other

component version

No response

IDF version.

mdns-v1.3.2-3-g7f248bd

More Information.

I followed the instructions in CONTRIBUTING.md and executed pip install pre-commit && pre-commit install-hooks && pre-commit install --hook-type commit-msg --hook-type pre-push.

However, I encountered an error at the pre-commit install-hooks step. See the attached pre-commit.log.

pre-commit.log

I am running a Fedora 39 system.

david-cermak commented 3 weeks ago

Could you please try to install the hooks in some virtual environment, preferably the one used by IDF tools?

$ source $IDF_PATH/export.sh 
$ pip install pre-commit && pre-commit install-hooks
SohKamYung-Espressif commented 3 weeks ago

try to install the hooks in some virtual environment

@david-cermak Could you clarify what you mean by virtual environment?

I am currently running Fedora 39 under VirtualBox, but I don't think this is what you mean by virtual environment.

david-cermak commented 3 weeks ago

oh, sorry, I meant python virtual environment or venv. But you'll enter one automatically once you export IDF related tools.

In other words, does it work if you run the pre-commit install commands after exporting, as suggested in https://github.com/espressif/esp-protocols/issues/592#issuecomment-2161106215 ?

SohKamYung-Espressif commented 3 weeks ago

I meant python virtual environment or venv

Oh. I see. That's new to me. :-)

I did the steps:

$ cd ~/projects/esp-protocols
$ source ~/esp/esp-idf/export.sh 
$ pip install pre-commit
$ pre-commit install-hooks

But I still get the same error at pre-commit install-hooks

I am running the esp-idf master branch. Does that make a difference?

SohKamYung-Espressif commented 3 weeks ago

@david-cermak I did some debugging.

First, I installed esp-idf on Windows 11 (ESP-IDF v5.2.2). Using this, pip install pre-commit && pre-commit install-hooks && pre-commit install --hook-type commit-msg --hook-type pre-push works as expected on esp-protocols.

On Fedora 39, I edited the .pre-commit-config.yaml file and found that if I removed this:

  - repo: https://github.com/igrr/astyle_py.git
    rev: c0013808882a15a0c0c2c1a9b5c903866c53a653
    hooks:
    -   id: astyle_py
        args: ['--style=otbs', '--attach-namespaces', '--attach-classes', '--indent=spaces=4', '--convert-tabs', '--align-pointer=name', '--align-reference=name', '--keep-one-line-statements', '--pad-header', '--pad-oper', '--exclude-list=ci/ignore_astyle.txt']

Then my pre-commit install-hooks runs successfully.

For now, I will use Windows to contribute and mark this as a possible Fedora 39 issue to look at later.

Can consider this issue as closed?

david-cermak commented 3 weeks ago

Thanks for the investigation, this is really helpful.

I read that pyyaml 6.0.0 is broken on some platforms and that's the version I see in your error log. Also seen that Ivan's fixed it in his repo. Could you please check if making this change solves the problem?

  - repo: https://github.com/igrr/astyle_py.git
-    rev: c0013808882a15a0c0c2c1a9b5c903866c53a653
+    rev: aff82856299e94ffabad96b5ab73370166b17a3c
    hooks:
    -   id: astyle_py
SohKamYung-Espressif commented 3 weeks ago

@david-cermak Yes. I can confirm that if I change the rev:, pre-commit install-hooks works as expected on Fedora 39.

david-cermak commented 2 weeks ago

Thanks for the confirmation! Will fix this in https://github.com/espressif/esp-protocols/pull/597