Closed ShadowCurse closed 8 months ago
For reviewers: don't approve before the CI is finished and it is clear that this PR fixes the issue.
The CI seems finished, can you confirm that everything is fine?
What about failing the build if something is wrong?
For reviewers: don't approve before the CI is finished and it is clear that this PR fixes the issue.
The CI seems finished, can you confirm that everything is fine?
What about failing the build if something is wrong?
I'm actually quite confused why it doesn't fail if something is wrong. The bash script has set -e
, and the Dockerfile ends with RUN ...
, so really it should propagate any errors :thinking:
So as I can see the problematic step finished without errors: https://github.com/rust-vmm/rust-vmm-container/actions/runs/7412574298/job/20169661451?pr=100#step:8:1500
What about failing the build if something is wrong?
I'm actually quite confused why it doesn't fail if something is wrong. The bash script has
set -e
, and the Dockerfile ends withRUN ...
, so really it should propagate any errors 🤔
This is a good point! Does anyone know why CI did not fail here, but executed the next command?
What about failing the build if something is wrong?
I'm actually quite confused why it doesn't fail if something is wrong. The bash script has
set -e
, and the Dockerfile ends withRUN ...
, so really it should propagate any errors 🤔This is a good point! Does anyone know why CI did not fail here, but executed the next command?
Mhh, I think it has something to do with the &&
. Testing locally, executing
#!/bin/bash
set -e
false
echo "hi"
prints nothing, while
#!/bin/bash
set -e
false && echo "hello"
echo "hi"
prints "hi". Bash :confounded:
Given that we have set -e
at the top of the script, would it make sense to avoid using &&
for chaining, and instead just split the commands across multiple lines always? That should fix it, I think.
Given that we have
set -e
at the top of the script, would it make sense to avoid using&&
for chaining, and instead just split the commands across multiple lines always? That should fix it, I think.
Yeah, I agree. Or we should add || exit 1
, but I don't see the point while using set -e
Added couple of commits to remove &&
and move $(uname -m)
to one global var.
Everything seems to work when I build locally.
Added couple of commits to remove
&&
and move$(uname -m)
to one global var. Everything seems to work when I build locally.
Just to check that the build now fails if something in the script is wrong, can we try to remove the definition of ARCH
and observe the build failing?
Without ARCH
the output:
...
+ pushd /usr/include/-linux-musl
/opt/src/scripts/build_container.sh: line 21: pushd: /usr/include/-linux-musl: No such file or directory
The command '/bin/sh -c /opt/src/scripts/build_container.sh' returned a non-zero code: 1
Summary of the PR
In the previous commits/PR (#99, #98) the ARCH env variable was introduced to the Dockerfile and build_container.sh scripts. It was meant to help with configuration of linux headers for musl-tools, but did not work for some reason. Considering the build_container.sh was already using
uname -m
calls to get the current arch this commit also uses it, so there is no need for ARCH anymore.Also removed
&&
from the scripts and moved$(uname -m)
to one global var.For reviewers: don't approve before the CI is finished and it is clear that this PR fixes the issue.
Requirements
Before submitting your PR, please make sure you addressed the following requirements:
git commit -s
), and the commit message has max 60 characters for the summary and max 75 characters for each description line.unsafe
code is properly documented.