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

Fix phase2 failures and improve QA #26

Merged
merged 7 commits into from
Apr 13, 2024
Merged

Fix phase2 failures and improve QA #26

merged 7 commits into from
Apr 13, 2024

Conversation

ynezz
Copy link
Member

@ynezz ynezz commented Dec 23, 2023 •
edited
Loading

Due to the stray ) in commit f0faed2 ("phase2: compute checksums") we have currently we've failing checksum step in phase2:

cd bin/packages/mipsel_24kc_24kf; find . -type f -not -name 'sha256sums' -printf "%P
" | sort | xargs -r ../../../staging_dir/host/bin/mkhash -n sha256 | sed -ne 's!^\(.*\) \(.*\)$!� *�!p' > sha256sums)
 in dir /builder/mipsel_24kc_24kf/build/sdk (timeout 1200 secs)
 watching logfiles {}
 argv: b'cd bin/packages/mipsel_24kc_24kf; find . -type f -not -name \'sha256sums\' -printf "%P\n" | sort | xargs -r ../../../staging_dir/host/bin/mkhash -n sha256 | sed -ne \'s!^\\(.*\\) \\(.*\\)$!\x01 *\x02!p\' > sha256sums)'
 environment:
...snip...
 using PTY: False
/bin/sh: 2: Syntax error: ")" unexpected
program finished with exit code 2
elapsedTime=0.007477

so lets fix it and as well try to detect such regressions in the future by improving the QA in the CI pipeline.

It seems, that flake8 is able to spot this issues:

phase2/master.cfg:739:28: W605 invalid escape sequence '\('
phase2/master.cfg:739:32: W605 invalid escape sequence '\)'
phase2/master.cfg:739:35: W605 invalid escape sequence '\('
phase2/master.cfg:739:39: W605 invalid escape sequence '\)'

@ynezz
Copy link
Member Author

ynezz commented Dec 23, 2023

It seems, that flake8 is able to spot this issues:

phase2/master.cfg:739:28: W605 invalid escape sequence '\('
phase2/master.cfg:739:32: W605 invalid escape sequence '\)'
phase2/master.cfg:739:35: W605 invalid escape sequence '\('
phase2/master.cfg:739:39: W605 invalid escape sequence '\)'

It seems I was too fast and its probably that sed-fu triggering this.

@f00b4r0
Copy link
Contributor

f00b4r0 commented Dec 23, 2023

Please don't reformat phase2 yet. It's only going to make my work harder. Let's save that for "the end" as we did on phase1. Thx

@ynezz ynezz changed the title Fix checksum step failure in phase2 and improve QA Fix phase2 failures and improve QA Dec 24, 2023
@ynezz
Copy link
Member Author

ynezz commented Jan 29, 2024

@f00b4r0 can you check the proposed fixes?

@ynezz
phase2: dlsha2rsyncpl: fix sha2rsync.pl script location
cff3aab 
Fixes following error:

 File 'sha2rsync.pl' not available at master

Fixes: c3ddb0d ("phase2: use sha2rsync.pl for 'targetupload'")
Signed-off-by: Petr Štetiar <[email protected]>
@ynezz
phase2: fix stray closing parenthesis
56f1ba5 
Due to the stray closing parenthesis introduced in commit f0faed2
("phase2: compute checksums") we have currently we've failing checksum
step in phase2:

 argv: b'cd bin/packages/mipsel_24kc_24kf; find . -type f -not -name \'sha256sums\' -printf "%P\n" | sort | xargs -r ../../../staging_dir/host/bin/mkhash -n sha256 | sed -ne \'s!^\\(.*\\) \\(.*\\)$!\x01 *\x02!p\' > sha256sums)'
 environment:
  ...snip...
  using PTY: False
 /bin/sh: 2: Syntax error: ")" unexpected
 program finished with exit code 2

Fixes: f0faed2 ("phase2: compute checksums")
Reported-by: Hannu Nyman <[email protected]>
Signed-off-by: Petr Štetiar <[email protected]>
@ynezz
Add linting with flake8 tool
e99b029 
flake8 is a Python tool that glues together pycodestyle, pyflakes,
mccabe, and third-party plugins to check the style and quality of some
Python code.

Currently we've issue in phase2 in checksum step:

 /bin/sh: 2: Syntax error: ")" unexpected

And it seems, that flake8 is able to spot places which might lead to
such issues during runtime:

 phase2/master.cfg:733:151: W605 invalid escape sequence '\('
 phase2/master.cfg:733:155: W605 invalid escape sequence '\)'
 phase2/master.cfg:733:158: W605 invalid escape sequence '\('
 phase2/master.cfg:733:162: W605 invalid escape sequence '\)'

So lets enable flake8 checking on the CI so we can spot similar places
in the future and address them before deployment.

We dont want to make current ongoing work on phase2 code harder, thus we
don't touch that part yet, so we whitelist most of the checks for now.

References: f0faed2 ("phase2: compute checksums")
Signed-off-by: Petr Štetiar <[email protected]>
@ynezz
phase2: checksums: split long command line
0b70ae6 
To make it more readable, diffs smaller, easier to review etc.

Signed-off-by: Petr Štetiar <[email protected]>
@ynezz
phase2: fix invalid escape sequence flake8 warning in checksums step
0f42b18 
Resolved a flake8 linting warning related to an invalid escape sequence
in the ShellCommand for calculating checksums:

 phase2/master.cfg:739:28: W605 invalid escape sequence '\('
 phase2/master.cfg:739:32: W605 invalid escape sequence '\)'
 phase2/master.cfg:739:35: W605 invalid escape sequence '\('
 phase2/master.cfg:739:39: W605 invalid escape sequence '\)'

The warning was caused by the use of unescaped parentheses in a regular
expression within a sed command.

Use a raw string (with an 'r' prefix) to treat backslashes as literal
characters, ensuring that the regular expression is correctly
interpreted and flake8 does not raise a warning.

This fix ensures that the code adheres to Python's string handling best
practices and maintains the integrity of the regular expression
functionality.

Fixes: f0faed2 ("phase2: compute checksums")
Signed-off-by: Petr Štetiar <[email protected]>
@ynezz
requirements-dev: use black 23.12.1 and ruff 0.1.9
8686448 
Fix ruff's recommendation:

 phase2/master.cfg:328:16: E721 Do not compare types, use `isinstance()`

 "Unlike a direct type comparison, isinstance will also check if an
  object is an instance of a class or a subclass thereof."

Signed-off-by: Petr Štetiar <[email protected]>
@ynezz
phase2: fix relative paths for scripts and files
fe54cb8 
Currently `buildlist` step fails with following:

  ../../../sha2rsync.pl ../../arch-sha256sums bin/packages/aarch64_generic/sha256sums rsynclist
   in dir /builder/aarch64_generic/build/sdk (timeout 1200 secs)
   watching logfiles {}
   argv: [b'../../../sha2rsync.pl', b'../../arch-sha256sums', b'bin/packages/aarch64_generic/sha256sums', b'rsynclist']
   environment:
    ...
    PWD=/builder/aarch64_generic/build/sdk
    ...
  Upon execvpe b'../../../sha2rsync.pl' [b'../../../sha2rsync.pl', b'../../arch-sha256sums', b'bin/packages/aarch64_generic/sha256sums', b'rsynclist'] in environment id 139847367136832
  :Traceback (most recent call last):
  ...
  FileNotFoundError: [Errno 2] No such file or directory: b'../../../sha2rsync.pl'

due to relative paths being off by one:

 worker work dir = /builder/aarch64_cortex-a72/build
 workerdest = "../sha2rsync.pl"
   is absolute path /builder/aarch64_cortex-a72/sha2rsync.pl

thus relative path from:

  FileNotFoundError: [Errno 2] No such file or directory: b'../../../sha2rsync.pl'

in following context:

  PWD=/builder/aarch64_generic/build/sdk
  b'../../../sha2rsync.pl'

is wrong absolute path `/builder/sha2rsync.pl` by one directory level,
thus adjust all those paths accordingly.

Fixes: c3ddb0d ("phase2: use sha2rsync.pl for 'targetupload'")
Signed-off-by: Petr Štetiar <[email protected]>
@ynezz ynezz force-pushed the ynezz/fix-p2-checksum-step branch from e312f11 to fe54cb8 Compare April 13, 2024 04:13
@ynezz ynezz merged commit 7d371c4 into openwrt:master Apr 13, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ynezz @f00b4r0