CSpell checks files that are created by a GitLab CI job

Hi avpaderno,
Thanks for raising this following this slack thread. You are right that we should not be including the internal files when checking spelling.

One very quick and simple way to fix this is to add the get-file-via-curl.sh file into the $ignore_files array in scripts/prepare-cspell.php.

But the problem stems from the fact that we run cspell from the top-level build directory $CI_PROJECT_DIR (which is /builds/project/seven in this case). We are already having to ignore "$webRoot", 'vendor', 'node_modules' and some other files such as 'build.env'. A more robust solution would be to run the cspell job in the Drupal project folder $DRUPAL_PROJECT_FOLDER (which is /builds/project/seven/web/themes/custom/seven in this case). Here we do not have the extra scripts and scaffolding folders, so it would be a cleaner way to execute the job.

There may be a good reason why this was not done in the first place, or it may have been investigated and rejected, so I can't say for ceertain that we can do this, but it is worth investigating.

I think the$DRUPAL_PROJECT_FOLDER was just introduced later than the latest changes we did in cspell job.

We can either add the internal file to our ignore list for cspell or just try to run cspell on the $DRUPAL_PROJECT_FOLDER folder directly.

Probably, also the assets/internal/.cspell.json file needs to be updated. Its ignorePaths value could be changed, although I would consider a project repository can contain a node_modules or a vendor directory.

Thanks for opening the MR. I've made a couple of suggestions following the failed cspell jobs. I also pushed changes to not run all the unwanted jobs and variants. But I will leave the actual cspell work for you to continue on.

I think the $DRUPAL_PROJECT_FOLDER was just introduced later than the latest changes we did in cspell job.

Yes the variable is new, but we did do earlier work to run eslint, phpstan and styleliny in the lower-level folder, before we had the variable.

Another angle to try might be to run cspell where it is now, but specify $DRUPAL_PROJECT_FOLDER as the path to check in. This would only involve changing the value of $CSPELL_SEARCH or maybe pre-pending the path to it in the execution call.

I guess it makes sense to do the cd $DRUPAL_PROJECT_FOLDER to match what the other jobs do. We might have just forgotten about cspell when we did the change.

NW based on your suggestions.

We might have just forgotten about cspell when we did the change

I don't think we forgot, as it was not an overall review of every job and where they execute. We were fixing things as and when the problems occured. Yes, lets try the cd way first.

Whilst we are working on cspell, another feature that I've been wanting, and which would really help in testing this MR, is the ability to have the hard-coded --no-progress in the command. It should be a variable, which defaults to hiding the progress to maintain the current behavior, but can be set when doing testing like this, so that we see a list of the files being checked. Maybe it could be called CSPELL_SHOW_PROGRESS or CSPELL_VERBOSE or something. It defaults to 0 but can be set to 1 if required. Then the progress setting would be simply derived, in a similar way to what we do for CSPELL_SEARCH

I've run the downstream pipelines for d9-basic and d10-theme for you (this can only be done by a maintainer of the downstream project). Also left another comment in the MR. The two jobs fail for different reasons, which is interesting and needs a closer look at.

Thanks for adding the verbose flags. I've run the downstream job, and we still get CSpell: Files checked: 0, Issues found: 0 in 0 files. I think this is because cspell cannot work with symlinked files, and I have just tested this locally and that is exactly what is happening. I get the same message, when all the files are symlinked. But if I create a new non-symlink file it gets checked.

So that means the whole approach of cd $DRUPAL_PROJECT_FOLDER needs to be undone, as we cannot run the command there. We will use the other approach (as mentioned in #2 and #3 above) to add the script name into the $ignore_files array.

Let me know if you still want to do this MR, you just need to revert the path changes in the cspell: section (but leave in the --progress change and the skip_ variables for now). Then you'll nee to make a change to prepare-cspell.php to add the .sh script name. Or if you do not want to do all this, that's fine, just say and we can pick it up instead. It's great having others collaborate on Gitlab Templates but equally there's no pressure to continue, just say.

cspell cannot work with symlinked files

That might have been the reason why the approach on this job was different to the others.

100% agree with #15 about reverting and trying the other approach (and 100% on the grateful-but-no-pressure part too).

We should definitely add a comment by that cd saying this job has to run in the top-level folder because cspell does not work with symlinked files. When I saw the "0 files checked" I had a vague recollection that we did know this once upon a time.

avpaderno → changed the visibility of the branch 3529479-check-only-project-files to hidden.

Thanks avpaderno, I have triggered the 'd9-basic' downstream branch.

We also have a mechanism to force failures in some of the GTD testing branches. You may like to try adding the variable

variables:
  BEFORE_SCRIPT_ACTIONS: 'cspell-use-madeupword'

into the '→ GTD D9 Basic': downstream job in .gitlab-ci.yml, to see that it fails.

Should not also the lines setting the SKIP_ variables be removed before merging the issue fork?

Yes that's correct, those will be removed. I didn't make a comment about them in the MR because its more clear that they have to be removed. But the --verbose could get missed. But thanks for checking.

Would you like to add the new CSPELL_SHOW_PROGRESS variable as mentioned in #11? Happy to leave that for you, but likewise I can pick it up if you don't have time.

I can do it, but not before July 10. (I am a little busy these days.)

OK, thanks. This issue is not too urgent, as it only affects the edge case when running cspell and also setting Drupal Core to 9.5. I think we're happy to wait for you to have time, if you want to do it and get some experience here, as it is good to have another contributor in the Gitlab Templates queue :-)

Hi @avpaderno
I have a project which runs tests on Drupal9 and the cspell job ends amber for the same reason as in this issue. I regularly get e-mail alerts that the pipeline has failed, but as the e-mail shows red not amber I keep forgetting that this is due to the spelling and think there is something else wrong. It would be nice to get this resolved. Are you OK if I take this one and finish the MR?

@jonathan1055 I thought I could find the time to complete this merge request, but I still have no time. You can complete this merge request.
I apologize: I did not contribute to this issue and I just delayed it.

There is no need to apologize at all. You found the problem in the first place, and that was a very worthwhile contribution. We totally understand that time constraints change, so please don't worry about that. It was actually my second request that delayed the issue, not you :-)

Downstream d10-plus using new variable _CSPELL_SHOW_PROGRESS: 1
We see the files listed
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/63...

Downstream d9-basic, also showing progress but with a forced failure.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/63...

Then following another commit, I tested d10-plus with _CSPELL_SHOW_PROGRESS: 0. We get no filenames shown
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/63...

and also d9-basic but setting DRUPAL_CORE: 9.5.11 to match the scenario that was the cause of this issue
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

I also used this technique in a web UI pipeline to demonstrate that d9-basic with 9.5.11 fails without this change.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

This is ready for review and feedback

The changes and the tests look good to me. Once the final cleanup of the code is done (there is a thread and a TEMP... comment on the code) I can RTBC.

Thanks, that's good.

One other thing I noticed, if there is a spelling fault, so that a second call to cspell is run to get an updated dictionary artifact, we get one extra file tested. In the first d9 job linked above we initially get

CSpell: Files checked: 8, Issues found: 1 in 1 file.

then the second run has

CSpell: Files checked: 9, Issues found: 1 in 1 file.

This is confusing and not good information for the developer. So I committed a temporary change to show the progress in the second call - and we see that the new empty file _cspell_updated_project_words.txt is the extra one included
https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/63...
So I will also add this file to the $ignore_files array.

Oh yeah, good call.

Adding _cspell_updated_project_words.txt to the $ignore_files array fixed this, and we now get 8 file checked on both runs. We were already adding _cspell_unrecognized_words.txt so I consolidated the lines.

All temporary code now removed, but I decided to leave in _CSPELL_SHOW_PROGRESS: '1' for the downstream pipelines, as we have done a similar thing with phpcs and phpstan to show more verbose output in the GTD tests.

I ran all the GTD downstream pipelines this time, and I noticed that apart from d9-basic, the jobs are checking the recipes/README.txt file. I recall this was mentioned before somewhere? Was it another linting job? It is not part of the project, so really we should be ignoring it. If you agree then we might as well fix it in this MR.

Regarding recipes/README.txt I was thinking of 🐛 PHPCS error in contributed module caused by core recipe.README.txt Active where we fixed it for the symlinked project folder. But of course, we have to run cspell in the top-level directory, so cspell should ignore everything in the recipes/ directory.

Added 'recipes' to the $non_project_directories array. Downstream jobs show we no longer test that file.
d10-plus shows 8 files not 9
d10-theme shows 4 files not 5

All debug and temporary changes have been removed so this is ready for final review.

It all looks good. I ran some more downstream pipelines https://git.drupalcode.org/issue/gitlab_templates-3529479/-/pipelines/58...

RTBC.

No code changes, but NR for this commit of docs update, which can be viewed here

The new changes look good to me as well. Back to RTBC. Thanks for the extra docs!

Merged.

Thanks for merging.
Could you also close MR369 which was the first attempt on this issue.

fjgarlin → closed merge request !369

I knew there was an existing issue about /recipes 📌 CSPELL is checking recipes/readme.txt - change to run in project folder Active . I have no idea why my search in #34 failed to find this issue. It also has some other things to fix in cspell so those could still be fixed over there.

Great. We can repurpose that other issue for the remaining tasks.