Pre-commit error trying to update an addon

Hi folks, I am trying to make some bugfixes to my r.viewshed.cva addon, and following the instructions for forking and merging here (grass-addons/CONTRIBUTING.md at grass8 · OSGeo/grass-addons · GitHub), I get the following error after trying to commit the changes:

$ git commit -m “r.viewshed.cva: Add flag -z to r.series call to avoid memory error when many viewpoints are input”

An error has occurred: InvalidManifestError:
==> File /home/iullah/.cache/pre-commit/repomgchj3c0/.pre-commit-hooks.yaml
==> At Hook(id=‘clang-format’)
==> At key: types_or
==> At index 10
=====> Type tag ‘metal’ is not recognized. Try upgrading identify and pre-commit?
Check the log at /home/iullah/.cache/pre-commit/pre-commit.log

The log file is attached. I am stumped since I don’t do this very regularly, and was hoping one of you might know what to do?

~Isaac
pre-commit.log (5.4 KB)

Looks like you have a old pre-commit installed. Usually it will update your tool versions without problem.

Try running

pip install -U pre-commit

To update pre-commit itself

Thanks Edouard! That was the first error. Updating pre-commit and identify helped move to another error suggesting the NPM and node were not at the correct version. So I have upgraded first using pip and then npm install, but am still getting errors. Log file is attached again, but error text is here:

An unexpected error has occurred: CalledProcessError: command: (‘/home/iullah/.cache/pre-commit/repo9idkiudq/node_env-system/bin/node’, ‘/usr/local/bin/npm’, ‘install’, ‘–include=dev’, ‘–include=prod’, ‘–ignore-prepublish’, ‘–no-progress’, ‘–no-save’)
return code: 1
stdout: (none)
stderr:
internal/modules/cjs/loader.js:818
throw err;
^

Error: Cannot find module ‘node:path’
Require stack:

  • /usr/local/lib/node_modules/npm/lib/cli.js
  • /usr/local/lib/node_modules/npm/bin/npm-cli.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:815:15)
    at Function.Module._load (internal/modules/cjs/loader.js:667:27)
    at Module.require (internal/modules/cjs/loader.js:887:19)
    at require (internal/modules/cjs/helpers.js:85:18)
    at Object. (/usr/local/lib/node_modules/npm/lib/cli.js:10:18)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions…js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Module.require (internal/modules/cjs/loader.js:887:19) {
    code: ‘MODULE_NOT_FOUND’,
    requireStack: [
    ‘/usr/local/lib/node_modules/npm/lib/cli.js’,
    ‘/usr/local/lib/node_modules/npm/bin/npm-cli.js’
    ]
    }
(attachments)

pre-commit.log (4.35 KB)

Turns out I had to completely remove npm manually from my system, and now pre-commit works ok. Might be useful to update the https://github.com/OSGeo/grass-addons/blob/grass8/CONTRIBUTING.md with information about ensuring to update pre-commit and identify and also that npm might need to be removed completely.

Hopefully the rest of the pipeline goes ok!

~Isaac

Do you mean you installed pre-commit from pip AND from npm?

We could always accept a PR to improve the contributing docs, but I think using really old deps might be out of scope (I don’t know what node version you had, but probably one that was EOL (end of life)).

I had to update pre-commit and identify (i used pip). That was the initial error, which was surpassed after that update. The next error had to do with a bunch of npm and node version problems, which I eventually solved simply by completely removing npm, which you have to do manually by deleting the various directories and files. Using ‘pip uninstall npm’ and ‘apt remove npm’ were not enough. Trying to upgrade npm and node via ‘pip install’ or via ‘npm install’ weren’t enough either.

I think the contributing doc could mention the need to check version of pre-commit and version of identify before trying to commit anything, as I didn’t realized these would go out of date.

I don’t really understand what was the problem with my npm and node install nor why only completely uninstalling them was the only way to move forward.

Since it is mentionned that node support does not need any system level support, pre-commit (and we saw it in the logs, that it created a cached env), probably only using

pre-commit clean

Or

pre-commit gc 

Would have worked for your second problem
https://pre-commit.com/#pre-commit-clean

Thanks. That is good to know. For those of us who are more occasional contributors and not overly-familiar with tools such as pre-commit, I think it would be useful to mention some of these things in the contributing document so that others can know a few things to check when they get errors during pre-commit, etc. For me, I send in maybe a couple of pull requests every year or two as I try to maintain the few addons I’ve contributed over the years. But I started way back when addons were managed by SVN, and though I have my own git repos, I don’t frequently work with big teams, branches, forks, and use of tools like pre-commit. I need to re-read the contributing docs each time I try to make a new contribution so that I am doing things right, and even then i clearly am making mistakes! :slight_smile:

Cheers,

Isaac

I agree with @echoix that this is probably out of scope, contributing documentation can’t capture solutions to all the different problems one can have. Forums are better for this.

I just submitted a minor pull-request for CONTRIBUTING.md. I’m not asking for a full debugging section, just a couple of lines about the need to install and update pre-commit and identify, which as far as I can tell are now requirements for folks who want to contribute code to the addons. This was not mentioned in the document yet, and so, for example, I had no idea what was going on when that error came up. Digging into what pre-commit does, it makes sense to have this in place for all pull requests, but I think it’s only fair to write up a reasonably detailed set of instructions for new contributors or even for old-time ones who are not as familiar with git and associated tools and workflows. Speaking from my own perspective, I want to ensure that I follow the expected processes and to make everyone else’s lives easier by not bugging the lists each time I run into something basic that I could probably figure out on my own if there were good instructions available…

Cheers,

~Isaac

Well, I think the current contributing file needs more work, it should pretty much be the same as the one in main grass core repo, which points to contributing guidelines and mention pre-commit. Could you look at that and see whether it is more clear there?

But in any case we need to avoid specific instructions how to install it, there are different ways how to do it (operating systems, virtual environments, integrations with IDEs etc), for example pip won’t work on newest ubuntu. The setup of these auxiliary tools is getting complicated.

I guess it’s time for me to ask for a bit of clarification. Some months back I tried to add a minor update to the main GRASS repository. I ran into problems because I was not clear on one component of the fork, PR, review/revise, merge workflow. This was a misunderstanding on my part that could be improved in the docs for this. However, I did all of this using my GitHub installation for the Mac, including its accompanying command line tools.

Is the issue in this thread of auxiliary tools only relevant for Linux distros or do I now need to search for additional Mac software in addition to Git in order to contribute anything to GRASS?

pre-commit is relevant for any operating system, it is optional, you can still contribute a PR without it. The CI checks might complain about formatting etc but you can fix that manually.

Since this discussion kind of reflects my usecase (very occasional contributor) I wanted to also shed some light on my doubts.

I wonder what happens in situations in which someone kind of wants to keep a coding style that is not the one grass requires. An example is when you have to migrate a larger external program with several functionalities, that will be ported in pieces once resources are made available to do so. So between the version 1 and 2 of the module a year might pass.
In these cases, I always tend to change as few as possible of the original code in order to be able to do parallel debug if I need to find nasty bugs introduced during the port (or cleanup of at least global variables and gotos).

From what I read here, such a contribution is not welcome and would be blocked by pre-commit and, while I will try to find a working solution for my case, I find that this can refrain people from contributing great modules.
I thought add-ons would be more relaxed and a contributor’s responsibility.

Thanks for any comment on this.