Skip to content

Target specific scripts in "Check Scripts" template #773

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.

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

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

per1234
Copy link
Contributor

@per1234 per1234 commented Oct 15, 2024

Background

Previously, the approach taken shell script tasks was to recursively search the entire repository for shell scripts. There were several problems with that system:


The -regextype flag used in the find commands of the shell:check and shell:check-mode tasks is not supported by the BSD/macOS version of find, meaning the tasks would fail if a contributor using a macOS machine tried to run them:

find: -regextype: unknown primary or operator

The shell:check and shell:check-mode tasks only provided coverage for files with .sh and .bash file extensions, whereas the practice of omitting a file extension on shell scripts is unfortunately quite common.


There was no obvious way to exclude paths of externally maintained files from coverage by the shell:format task.

Alternative Solutions

The first of the problems listed above could be overcome by configuring the tasks to adjust the find commands to use different flags depending on the operating system of the machine (-regextype posix-extended when running in a GNU shell and -E when running in a BSD shell).

The second could be overcome by using shfmt --files (which searches recursively for shell scripts by checking for the presence of a shebang inside files and outputs a list of the paths) as a replacement for find.

The third could be overcome by documenting the use of shfmt's poorly advertised feature of ignoring the paths assigned an ignore attribute in the .editorconfig file.

Chosen Solution

After careful consideration, the decision was made to abandon the previous approach of attempting to automatically discover script files and instead use the strategy of configuring the template with the specific paths of the scripts to be checked for each project it is installed into.

Although such an approach would not be appropriate in the case of a check on files that tend to be present in greater abundance and regularly added and moved in a project, this is not the case for shell scripts in Arduino projects. A project is more likely to contain a few scripts at most and their paths tend to be reasonably stable.

Code Duplication in Workflow

In order to make it easier to interpret results, navigate logs, and reduce duration of workflow runs, a separate workflow job is used for each of the distinct checks. Unfortunately it is necessary for the project maintainer to configure the script paths redundantly in the matrix of each of the three jobs.

Intuitively we would expect this could be avoided by defining the paths at workflow scope via the env key. However, this is not feasible due to the env context not being available for use in the jobs.<job name>.strategy.matrix key.

It is technically possible to accomplish this by adding a job that converts the data from the env context into a job output (the needs context is available for use in the jobs.<job name>.strategy.matrix key). However the significant increase in complexity this would bring to the workflow outweighs the benefit of avoiding duplication.

@per1234 per1234 added topic: infrastructure Related to project infrastructure type: imperfection Perceived defect in any part of project labels Oct 15, 2024
@per1234 per1234 self-assigned this Oct 15, 2024
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.05%. Comparing base (4fb2baa) to head (ad800ef).
Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #773   +/-   ##
=======================================
  Coverage   90.05%   90.05%           
=======================================
  Files          44       44           
  Lines        6800     6800           
=======================================
  Hits         6124     6124           
  Misses        553      553           
  Partials      123      123           
Flag Coverage Δ
unit 90.05% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Background
----------

Previously, the approach taken shell script tasks was to recursively search the entire repository for shell scripts.
There were several problems with that system:

The `-regextype` flag used in the `find` commands of the `shell:check` and `shell:check-mode` tasks is not supported by
the BSD/macOS version of `find`, meaning the tasks would fail if a contributor using a macOS machine tried to run them:

```
find: -regextype: unknown primary or operator
```

The `shell:check` and `shell:check-mode` tasks only provided coverage for files with `.sh` and `.bash` file extensions,
whereas the practice of omitting a file extension on shell scripts is unfortunately quite common.

There was no obvious way to exclude paths of externally maintained files from coverage by the `shell:format` task.

Alternative Solutions
---------------------

The first of the problems listed above could be overcome by configuring the tasks to adjust the `find` commands to use
different flags depending on the operating system of the machine.

The second could be overcome by using `shfmt --files` (which searches recursively for shell scripts by checking for the
presence of a shebang inside files and outputs a list of the paths) as a replacement for `find`.

The third could be overcome by documenting the usage of shfmt's poorly advertised feature of ignoring the paths assigned
an `ignore` attribute in the .editorconfig file.

Chosen Solution
---------------

After careful consideration, the decision was made to abandon the previous approach of attempting to automatically
discover script files and instead instead use the strategy of configuring the template with the specific paths of the
scripts to be checked for each project it is installed into. Although such an approach would not be appropriate in the
case of a check on files that tend to be present in greater abundance and regularly added and moved in a project, this
is not the case for shell scripts in Arduino projects. A project is more likely to contain a few scripts at most and
their paths tend to be reasonably stable.

Code Duplication in Workflow
----------------------------

In order to make it easier to interpret results, navigate logs, and reduce duration of workflow runs, a separate
workflow job is used for each of the distinct checks. Unfortunately it is necessary for the project maintainer to
configure the script paths redundantly in the matrix of each of the three jobs.

Intuitively we would expect this could be avoided by defining the paths at workflow scope via the env key. However, this
is not feasible due to the env context not being available for use in the jobs.<job name>.strategy.matrix key.

It is technically possible to accomplish this by adding a job that converts the data from the `env` context into a job
output (the `needs` context is available for use in the `matrix` key). However the significant increase in complexity
this would bring to the workflow outweighs the benefit of avoiding duplication.
@per1234 per1234 force-pushed the check-shell-matrix branch from 5dae1b6 to ad800ef Compare October 15, 2024 13:47
@per1234 per1234 merged commit 00377b5 into arduino:main Oct 15, 2024
67 checks passed
@per1234 per1234 deleted the check-shell-matrix branch October 15, 2024 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: infrastructure Related to project infrastructure type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant