-
Notifications
You must be signed in to change notification settings - Fork 32
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.
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
Build enhancements #26
Conversation
Build will happen in _build/$TARGET subdir, also: incremental build should be supported.
Actually, this will probably be changed tomorrow to branch instead of pull request, as github actions configured in pull request are not triggered. Unfortunately this PR introduces CI for 'push' and 'pull_request' only on main repo, it will look like that: |
|
||
sz=`du -k _boot/phoenix-armv7a7-imx6ull.jffs2 | awk '{ print $1 }'` | ||
sz=$(du -k "$IMG" | awk '{ print $1 }') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe cut
instead awk
sz=$(du -k "$IMG" | cut -f1)
build.project
Outdated
|
||
TARGET_FAMILY=$(echo "$TARGET" | awk -F- '{ print $1 }') | ||
TARGET_SUBFAMILY="$TARGET_FAMILY-$(echo "$TARGET" | awk -F- '{ print $2 }')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simpliefied to:
TARGET_FAMILY=$(echo "$TARGET" | cut -d- -f1)
TARGET_SUBFAMILY=$(echo "$TARGET" | awk -F- '{ print $1"-"$2 }')
or without creating processes:
TARGET_FAMILY=${TARGET%%-*}
tmp=${TARGET#*-}
TARGET_SUBFAMILY="$TARGET_FAMILY-${tmp%%-*}"
unset tmp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wanted to do something with that ugly one-liner but ended with using printf
in awk
- which was not better by any means.
I think we can use string substitution as You suggested (as it's POSIX sh compliant), but I would prefer to do it separately from this PR (probably introduce it as a function to phoenix-rtos-build/build.subr
?).
For now I've used Your version of awk
script as it's tidier than the previous solution.
d84d3fc
to
a8b87f2
Compare
_targets/build.project.ia32-generic
Outdated
@@ -39,35 +37,38 @@ b_build_target() { | |||
|
|||
mkdir -p "$PREFIX_ROOTFS"/dev "$PREFIX_ROOTFS"/local "$PREFIX_ROOTFS"/data "$PREFIX_ROOTFS"/mnt "$PREFIX_ROOTFS"/var |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shorter
mkdir -p "$PREFIX_ROOTFS/"{dev,/local,data,mnt,var}
|
||
cp riscv-pk/build-${TARGET}/bbl $PREFIX_BOOT/phoenix-riscv64-spike.bbl | ||
} | ||
|
||
|
||
b_image_target() { | ||
b_log "Creating image from $PREFIX_ROOTFS" | ||
size=`find ${PREFIX_ROOTFS} -type f -exec du -k {} \; | awk 'BEGIN { sum=0 }; { sum+=$1; }; END { print sum }'` | ||
size=`expr ${size} \* 150 / 100` | ||
size=$(find "${PREFIX_ROOTFS}" -type f -exec du -k {} \; | awk 'BEGIN { sum=0 }; { sum+=$1; }; END { print sum }') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of executing du
for each file, maybe it will be better:
size=$(find "${PREFIX_ROOTFS}" -type f -print0 | du -c -k --files0-from - | awk 'END{print $1}')
You can do arithmetic in awk:
awk 'END{print($1 * 150) / 100}'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think leaving it as is is better - the first line computes the real sum of file sizes and the second just estimates the partition size to be 1.5x larger than that.
Maybe it would increase readability to change variables naming to fileSizeSum
and partSize
, but it can be done in a separate PR if desirable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
du -k does not show the actual size of the files, but the size of the blocks used
e.g.
filesystem ETX4
du -b test.txt
1 test.txt
du -k test.txt
4 test.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right - I didn't dig into the ext2 image creation too much, the main point of this PR is to make all targets compilable, introduce docker-based build and basic CI using Github Actions.
The ext2 creation needs to be probably revised, as genext2fs
takes size -b
in blocks, and we should take into account that block size on host filesystem (eg. Your EXT4) might be different than on the target image.
For now I think we don't have the ability to mount ext2 partition on riscv64 targets, it would be better to focus on ia32-generic on which mounting ext2 works (can be tested under qemu).
This is not related to the buildsystem enhancements, should be done as a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit: I've checked that default block size in genext2fs
is 1024, so we should probably get the ceil of file_size / 1024, eg:
echo $((($(stat -c "%s" file.txt) + 1023) / 1024))
But I'm not proficient enough to make the change and currently don't have the means to test it, so I'm leaving it as-is.
I think @Maxez is more knowledgeable in this topic, maybe our comments would help him.
486ac15
to
46ff96b
Compare
46ff96b
to
d03b74f
Compare
WORK IN PROGRES | DO NOT MERGE
This PR includes many buildscript enhancements and fixes to make it possible building with docker environment.
This also adds github workflow to automatically test building for supported targets (experimental).