$ 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?
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:
/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’
]
}
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.
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
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!
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…
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.
pre-commit is just supposed to help contributors, as I said you don’t need to use it. But we do prefer to have the code formatted and checked for possible problems to simplify the maintenance, since we do maintain other contributors’ code. If you decide you want to keep your style, we can do per-file excludes in the configuration, ideally with a comment explaining the reason behind it and whether it’s just a temporary solution or it’s intended to stay like that.
These things are evolving, so thank you for your comment, I will try to clarify it in the CONTRIBUTING file, which needs some revision anyway.
Hi Anna,
thanks a lot for the clarification.
As it often happens after writing my post, I took the day to try to better understand what it means to adapt the code to the grass style rules.
And I have to admit, the pre-commit actually went quite smooth regarding formatting and trimming. Its installation worked under ubuntu using apt and not pip, just for the record.
I actually thought it would complain also about other things like for example camelcase named functions or missing doxygen on functions.
But that does not seem the case. So I have to apologize for bringing up something that is actually not as bad as I thought
Anna, I have a question about what happens when you ignore the pre-commit check result if it failed. I just tried a PR for a very minor change to CONTRIBUTING.md, and the Markdown check failed. Even looking at the log, I couldn’t really tell why, and it renders fine in Github, so I squashed and merged it anyway. But Edouard made a comment that the error will now be shown again after each commit? How do I rectify this? And, if it’s sometimes “OK” to ignore minor pre-commit errors, how would I prevent that situation from happening again?
The CI does not prevent you from merging it even with a failure (in addons). I guess the expectation is that contributors with write access will check that and fix it. But maybe we should change the github setting to not allow merging in that case?
Thanks to both of you. I will try to make a fix early next week (tied up with other things at the moment). Out of curiosity, is there a reason why such tight control of markdown formatting is required? Since this doc is just rendered in github, the additional space, while technically not correct, doesn’t prevent the page from rendering correctly?
FWIW, I made the changes in the online code editor directly on Github, and so the log files of pre-commit were not easy to pull up and check. I know I probably shouldn’t have done it that way, but since it was just literally to add two lines I thought it might be a simpler approach.
