cleanup secureboot #97
Conversation
nkraetzschmar
commented
Sep 5, 2024
nkraetzschmar
commented
- deprecate secureboot and make_repart_* support from builder (move into feature image scripts)
- this simplifies the build process by moving more design decisions into the feature configs
builder/make_get_artifact_rules
Outdated
| done | ||
| done | ||
|
|
||
| # printf 'DEBUG: [%s] %s -> %s\n' "$0" "$*" "${extensions[*]}" >&2 |
There was a problem hiding this comment.
Do we need this in the future? Otherwise I would remove this.
builder/make_get_image_dependencies
Outdated
| done | ||
|
|
||
| if [ -z "$script" ]; then | ||
| printf 'no image or convert script found to build %s\n' "${extension#.}" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # printf 'DEBUG: [%s] %s -> %s %s\n' "$0" "$*" "$script" "$input" >&2 |
There was a problem hiding this comment.
builder/make_list_build_artifacts
Outdated
| done | ||
| done | ||
|
|
||
| if [ "${#artifacts[@]}" = 3 ] && [ -n "$(./parse_features --feature-dir "features" --cname "$cname" platforms)" ]; then | ||
| artifacts+=(".build/$cname-$COMMIT.raw") | ||
| fi | ||
|
|
||
| # printf 'DEBUG: [%s] %s -> %s\n' "$0" "$*" "${artifacts[*]}" >&2 |
There was a problem hiding this comment.
|
applied comments and merged in latest changes from main |
| @@ -11,8 +11,8 @@ artifacts=(".build/$cname-$COMMIT.tar" ".build/$cname-$COMMIT.release" ".build/$ | |||
|
|
|||
| for feature in "${features[@]}"; do | |||
| for i in "features/$feature/"{image,convert}.*; do | |||
| extension="$(grep -E -o '(\.[a-z][a-zA-Z0-9\-_]*)*$' <<< "$i")" | |||
| artifacts+=(".build/$cname-$COMMIT$extension") | |||
There was a problem hiding this comment.
When I see this right, we have this part of the code duplicated within this project? Not only is this code difficult to read/understand for someone who doesn't know the regex by hard. It also adds the issue that it is challenging to maintain.
Please put it into a function that can be called with some documentation to it. Since you've changed the extension that is included, this should be part of the documentation as well. I couldn't tell why we suddenly needed the tilde.
| @@ -7,8 +7,9 @@ extensions=(release manifest raw) | |||
|
|
|||
| for feature in "features/"*; do | |||
| for i in "$feature/"{image,convert}.*; do | |||
| extension="$(grep -E -o '(\.[a-z][a-zA-Z0-9\-_]*)*$' <<< "$i")" | |||
| extensions+=("${extension:1}") | |||
| extension="$(grep -E -o '(\.[a-z][a-zA-Z0-9\-_~]*)*$' <<< "$i")" | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
As we talked about this. Please write down what this grep does and why it's necessary. Lastly, write down where the same function is used as well.
| @@ -11,8 +11,8 @@ artifacts=(".build/$cname-$COMMIT.tar" ".build/$cname-$COMMIT.release" ".build/$ | |||
|
|
|||
| for feature in "${features[@]}"; do | |||
| for i in "features/$feature/"{image,convert}.*; do | |||
| extension="$(grep -E -o '(\.[a-z][a-zA-Z0-9\-_]*)*$' <<< "$i")" | |||
| artifacts+=(".build/$cname-$COMMIT$extension") | |||
| extension="$(grep -E -o '(\.[a-z][a-zA-Z0-9\-_~]*)*$' <<< "$i")" | |||
There was a problem hiding this comment.
Also, I think the usage of here-string should be stated, not everyone is familiar with this concept of bash. Someone could misread this and only thing of << instead of three. Maybe we could make this a bit more obviously?
