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.

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

Merged
merged 10 commits into from
Oct 21, 2020
Merged

Conversation

nalajcie
Copy link
Member

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).

@nalajcie
Copy link
Member Author

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:
nalajcie#1


sz=`du -k _boot/phoenix-armv7a7-imx6ull.jffs2 | awk '{ print $1 }'`
sz=$(du -k "$IMG" | awk '{ print $1 }')
Copy link
Contributor

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 Show resolved Hide resolved
build.project Outdated

TARGET_FAMILY=$(echo "$TARGET" | awk -F- '{ print $1 }')
TARGET_SUBFAMILY="$TARGET_FAMILY-$(echo "$TARGET" | awk -F- '{ print $2 }')"
Copy link
Contributor

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

Copy link
Member Author

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.

@nalajcie nalajcie force-pushed the build_enhancements branch 2 times, most recently from d84d3fc to a8b87f2 Compare October 21, 2020 09:07
@@ -39,35 +37,38 @@ b_build_target() {

mkdir -p "$PREFIX_ROOTFS"/dev "$PREFIX_ROOTFS"/local "$PREFIX_ROOTFS"/data "$PREFIX_ROOTFS"/mnt "$PREFIX_ROOTFS"/var
Copy link
Contributor

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 }')
Copy link
Contributor

@kemonats kemonats Oct 21, 2020

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}'

Copy link
Member Author

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.

Copy link
Contributor

@kemonats kemonats Oct 21, 2020

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

Copy link
Member Author

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.

Copy link
Member Author

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.

@nalajcie nalajcie force-pushed the build_enhancements branch 4 times, most recently from 486ac15 to 46ff96b Compare October 21, 2020 15:18
@nalajcie nalajcie merged commit 101b94e into phoenix-rtos:master Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants