Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Consolidate naming and responsibility of dev scripts #1716

Closed
jotaen4tinypilot opened this issue Jan 12, 2024 · 3 comments
Closed

Consolidate naming and responsibility of dev scripts #1716

jotaen4tinypilot opened this issue Jan 12, 2024 · 3 comments
Assignees
@jotaen4tinypilot
Labels
enhancement New feature or request

Comments

@jotaen4tinypilot
Copy link
Contributor

jotaen4tinypilot commented Jan 12, 2024 •
edited

The build-python and build-javascript dev scripts technically don’t build anything, but they rather execute tests and static analysis procedures. Therefore, the prefix build- might be a bit surprising. To me, “build” would indicate that something is compiled or assembled. I’d probably find test- more intuitive here.

We also have several separate “check”-prefixed bash scripts that do static analysis. So it might not be totally clear where to put things. E.g., Python style checking is performed via build-python, whereas JavaScript style checking is performed via check-style and not via build-javascript. As we are about to add tests for bash scripts, we will be having the same duality with check-bash and build-bash.

Seeing that we have 20 dev scripts by now, I suggest we take a step back and review their structure altogether, and try to find a more consistent naming and responsibility scheme.
@jotaen4tinypilot jotaen4tinypilot added the enhancement New feature or request label Jan 12, 2024
@jotaen4tinypilot jotaen4tinypilot mentioned this issue Jan 12, 2024
Merged
@mtlynch
Copy link
Contributor

mtlynch commented Jan 12, 2024

Yeah, the naming is not super well-thought-out. It's mainly that the scripts match conventions I use in previous projects, and I'm used to the naming at this point, so I don't re-evaluate.

I agree that it's confusing because there's not a clear pattern on when to use build vs check vs lint.

I think we should come up with a small, simple set of rules that we feel comfortable following and adapting our scripts to match.

jotaen4tinypilot added a commit that referenced this issue Jan 17, 2024
@jotaen4tinypilot @jotaen
Set up automated bash tests with bats (#1721)
a141353 
Resolves #1714.
Related #1710.
Stacked onto #1720.

This PR sets up bash tests with
[bats](https://bats-core.readthedocs.io/en/stable/), and adds a first
test suite for [the `strip-marker-sections`
script](#1720).

## Notes
- Similar to how we do it with other test files, I put the `.bats` test
file next to the script under test. The directive in `.dockerignore`
[removes it from the bundle / `.deb`
file](https://github.com/tiny-pilot/tinypilot/assets/83721279/7f52ca3e-17d9-486a-83a5-ea1c6132eb36).
- The `build_bash` dev script is called “build” for consistency with
`build_python` and `build_javascript`, even though we technically don’t
build something. I’ve created
#1716 for us to
potentially reconsider this overall.
- In `build_bash`, it somehow felt reasonable to me to search all places
that are likely to contain `.bats` files, even though we currently only
have them in `/opt/tinypilot-privileged/scripts/`. I don’t feel strongly
about this, though.
- For running the tests, there theoretically would be [an official bats
Docker
image](https://bats-core.readthedocs.io/en/stable/installation.html#running-bats-in-docker),
however that doesn’t play nicely with our bash files: the docker image
doesn’t have a symlink at `/bin/bash`, so it fails to execute any of our
bash scripts that have a `#!/bin/bash` shebang (which is effectively all
of them). We instead would either have to change all our shebangs to the
(most portable) `#!/usr/bin/env bash` shebang, or we just install bats
manually like done here (in the Circle conf).
<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1721"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>

---------

Co-authored-by: Jan Heuermann <jan@jotaen.net>
@jotaen4tinypilot
Copy link
Contributor Author

jotaen4tinypilot commented Apr 24, 2024 •
edited

I’ve dabbled a bit with the structure of the scripts, and to me, the following changes would seem sensible:

(Note, the branch doesn’t reflect all of these suggestions yet, it’s more like a rough initial exploration.)

  • Rename all “check” scripts to have a check- prefix (regardless of whether they are linting, testing, or style checking)
  • Add missing invocations to check-all script (previously: build), which doesn’t cover all scripts currently
  • Introduce check-debian-pkg script that contains the inlined bash from the CircleCI YAML
  • Move the enable-passwordless-sudo and the 2 install- scripts to a new folder dev-scripts/remote/, to make it clear that they are supposed to run on device (in contrast to all other scripts, which are made to run on the dev machine)
  • Ensure that all dev scripts have a brief, initial docstring comment.
  • Adjust all paths of where we invoke any of the renamed scripts

@jdeanwallace what would you think about that proposed structure?

@jdeanwallace
Copy link
Contributor

@jotaen4tinypilot - These changes look good to me.

Move the enable-passwordless-sudo and the 2 install- scripts to a new folder dev-scripts/remote/, to make it clear that they are supposed to run on device (in contrast to all other scripts, which are made to run on the dev machine)

(nit) The word "remote" makes me think of a device not in my local network. Alternative suggestion: "device"?

This was referenced Apr 25, 2024
Merged
Merged
Merged
Merged
jotaen4tinypilot added a commit that referenced this issue Apr 26, 2024
@jotaen4tinypilot @jotaen
Add “check” prefix to all testing/linting/code-style dev scripts (#1791)
80c0420 
Related #1716.

This PR introduces a consistent `check-` prefix for all dev scripts that
are concerned with anything related to checking (i.e., testing, linting,
code-style).

- I consolidated `check-bash`+`build-bash` and
`lint-frontend`+`build-javascript` into one script respectively. To me,
that would make more sense than having too many fine-granular scripts.
We do it the same way in the [`check-python`
script](https://github.com/tiny-pilot/tinypilot/blob/master/dev-scripts/build-python)
already.
- I also renamed all CircleCI jobs to reflect the dev script names.
- I renamed `build` to `check-all`, and, while on it, also added the
missing invocations. The only script that I wouldn’t cover here is
`check-e2e`, because it’s noticeably resource-heavy than all other
scripts, and requires non-trivial prerequisites.
<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1791"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>

---------

Co-authored-by: Jan Heuermann <jan@jotaen.net>
jotaen4tinypilot added a commit that referenced this issue May 15, 2024
@jotaen4tinypilot @jotaen
Extract dev script for running lintian (#1792)
d3fb5b1 
Related #1716. Stacked on
#1791.

This PR pulls out the inlined `lintian` invocation to its own dev
script.

I had to apply one fix, because [shellcheck complained about `<(ls
*.deb)`, which should be `<(ls --
*.deb)`](https://www.shellcheck.net/wiki/SC2035).
<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1792"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>

---------

Co-authored-by: Jan Heuermann <jan@jotaen.net>
jotaen4tinypilot added a commit that referenced this issue May 15, 2024
@jotaen4tinypilot @jotaen
Move install-related dev scripts to device/ folder (#1793)
ef838a4 
Related #1716. Stacked on
#1791.

This PR moves both install-related dev scripts to a new subfolder
`device/`. This is mostly for clarity, to communicate that these two
scripts are not supposed to be executed on a dev machine.

Initially I thought we should also move the [`enable-passwordless-sudo`
script](https://github.com/tiny-pilot/tinypilot/blob/master/dev-scripts/enable-passwordless-sudo)
there as well, but I’m not so sure about that anymore, because I think
it also would be possible to run it in a dev environment. E.g., we also
[run it on CI for the e2e
tests](https://github.com/tiny-pilot/tinypilot/blob/3323b235e53d2306c5daba58a1d855d5d1575d91/.circleci/config.yml#L162-L164).
<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1793"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>

---------

Co-authored-by: Jan Heuermann <jan@jotaen.net>
jotaen4tinypilot added a commit that referenced this issue May 15, 2024
@jotaen4tinypilot @jotaen
Add missing docstring comments for check scripts (#1794)
a444cbd 
Related #1716. Stacked on
#1791.

This PR adds missing docstring comments to check-scripts. It also adds
missing `#` comment markers to leading blank lines in all docstring
comments, for consistency.

<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1794"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>

---------

Co-authored-by: Jan Heuermann <jan@jotaen.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jotaen4tinypilot jotaen4tinypilot

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jdeanwallace @mtlynch @jotaen4tinypilot