Closed lhernanz closed 6 years ago
Cool! Thanks for taking the time. It's a good idea to share your progress here.
@StefanScherer, sorry for the delay. I have committed the rest of the code. It should be working on Linux (all the current test pass) and probably it is not working on Darwin as I do not have any means to test it. Can you have a look and do some initial testing on Darwin?
Some comments
Thanks
When I started with Darwin tests I only knew that Travis supports Darwin. Well, we could switch over to Circle and use a matrix build there. But I don't have a problem using two CI servers for two platforms. :-)
Yes, CI is 💚. I'll try this commit 4ec6cfb on my Mac for some manual tests.
Next steps would be
For step 2 maybe we prepend/append "linux" / "darwin" to each function so we can have them in one script and call the right function. WDYT?
I have merged everything in a single script. Please, have a look. Feel free to change whatever is needed to make it to work on the Mac. I have been unable to test the code there, so probably it will require some fine-tunning to make it work.
Thanks @lhernanz
So the Linux/flash
script is now the merged script, right?
Maybe it's better to move this script up without a Darwin or Linux folder and remove the other script as well.
The bats tests currently use ./$OS/flash
which then should be just ./flash
to run the same script in both CI platforms.
Will try the Linux/flash
on my Mac for some manual tests now.
Yes, that is right. I agree with you, but I wanted you to test the script on Mac before doing those more radical changes. But I can do when you tell me.
Ha, that's very wise. I've pushed the changes into another test branch pr116 to run both Travis and Circle with the moved ./flash
script in top level directory.
Linux is still green https://circleci.com/gh/hypriot/flash/56 Darwin still waiting for a machine to start. https://travis-ci.org/hypriot/flash/builds/353998952
I think we should switch over to Circle for the Darwin build as Travis sometimes is really slooow. :-)
Let's see how it works.
A first manual test on macOS:
$ ./flash https://65-115249728-gh.circle-artifacts.com/0/home/circleci/project/pi-gen/deploy/image_2018-03-15-hypriotos-lite.zip
/usr/bin/curl
Downloading https://65-115249728-gh.circle-artifacts.com/0/home/circleci/project/pi-gen/deploy/image_2018-03-15-hypriotos-lite.zip ...
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 425M 0 425M 0 0 4391k 0 --:--:-- 0:01:39 --:--:-- 5588k
/usr/bin/unzip
Uncompressing /tmp/image.img.zip ...
Archive: /tmp/image.img.zip
inflating: /tmp/2018-03-15-hypriotos-lite.img
Use /tmp/2018-03-15-hypriotos-lite.img
./flash: line 475: autodetect_device: command not found
I think we should look at the Travis results first so I won't crash my machine ;-)
Oh, CircleCI's macOS needs a paid plan https://circleci.com/gh/organizations/StefanScherer/settings#osx-pricing starting at $39/month. So I have to stay with Travis + Circle combo.
I ran the next local test. After fixing the typo autodetect_devices
-> autodetect_device
the script listed the correct SD card slot on macOS:
$ ./flash https://65-115249728-gh.circle-artifacts.com/0/home/circleci/project/pi-gen/deploy/image_2018-03-15-hypriotos-lite.zip
/usr/bin/curl
Downloading https://65-115249728-gh.circle-artifacts.com/0/home/circleci/project/pi-gen/deploy/image_2018-03-15-hypriotos-lite.zip ...
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 425M 0 425M 0 0 4222k 0 --:--:-- 0:01:43 --:--:-- 4897k
/usr/bin/unzip
Uncompressing /tmp/image.img.zip ...
Archive: /tmp/image.img.zip
inflating: /tmp/2018-03-15-hypriotos-lite.img
Use /tmp/2018-03-15-hypriotos-lite.img
Is /dev/disk2 correct? y
But after entering y
the script just exited.
Oh, the script terminates on macOS due to the return 1
in check_device_is_writable
. When I change it to
check_device_is_writable() {
readonlymedia=$(diskutil info "${disk}" | grep "Read-Only Media" | awk 'NF>1{print $NF}')
set +e
if [[ $readonlymedia == "No" ]] ; then
return 1
else
return 0
fi
}
and
check_device_is_writable "${disk}"
writable=$?
set -e
I can go ahead this check function.
Some other macOS issues.
The
stat -c %s
in line 529 is
stat -f %z
on macOS.
For Travis CI tests I have to move the code from prepare_raw_disk
into wait_for_disk
as this code should run after the dd
command on mac, moving the written *.dmg
file to a temporary file. This code is only to make CI tests work.
I'll push that into the pr116 branch for further tests on Travis.
I am sorry that you are finding trouble. It was hard to me to make sure the code was ok without being able to properly test it. Please, make the changes you consider necessary. In some cases, as you state, you will need to move some code from one method to another one.
The case of the pipe is an interesting one. The linux version set a set -eo pipefail
at the beginning of the script. This is what is causing that code to fail. We will need to figure out whether that configuration is the best one for both versions or maybe we need to limit it to when we are running Linux.
Thanks for your effort!
No worries. Thank you for push this so hard to have this in the current state. This looks so promising to get this working.
Haha, another small diff: dd on Linux wants 1M
and macOS wants 1m
.
Interesting nuances! Having into account the stat and dd changes, probably you will need to encapsulate the whole block in another method that will have different implementations.
You can find the latest commits in https://github.com/hypriot/flash/commits/pr116 and also the CI status. I hope the latest commit should work much better on Travis, it worked locally with a real SD card.
@lhernanz I've updated the flash script for macOS, moved the flash
script to top-level folder, updated the bats tests. Both Linux and macOS CI are green.
Thank you so much to make this happen.
Can you run some local tests on Linux?
I'll do some final tests on macOS.
But then I think we're ready to merge 🎉
Nasty sed -i -e ...
creates new files on macOS. We have to call sed -i "" -e ...
to activate in-place changes. Moved the sed commands into a sed_i
function.
@lhernanz We're back to green for macOS CI + Linux CI. I flashed a lot today and found/remembered this sed difference. Added a BATS test for that.
Hi,
this is a first attempt to have a single script for Linux and Darwin. Keeping the 2 versions in sync is going to be a huge effort with little benefit and, even today, there are features that are only in one of the scripts.
This is still WIP. Although the current version completes all the Linux tests successfully, it still does not include all the functionality that was in the Darwin version of the script.
But I have preferred to share it earlier so that you can see what I am working on and you can make suggestions about how to improve the current code.