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.

Sign up for GitHub

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

Jump to bottom

mklive.sh: start using lib.sh functions. #322

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

mklive.sh: start using lib.sh functions. #322

wants to merge 7 commits into from

Conversation

mobinmob
Copy link
Contributor

No description provided.

@mobinmob mobinmob changed the title mklive: start using lib.sh functions. [wip] mklive: start using lib.sh functions. Feb 25, 2023
@mobinmob mobinmob changed the title [wip] mklive: start using lib.sh functions. [wip] mklive.sh: start using lib.sh functions. Feb 25, 2023
@mobinmob
mklive.sh: add some comments, fix the comment fro trap.
2fe20f1
@mobinmob
mklive.sh.in: replace die() with bailout, also:
028edf6 
- add more tools to REQTOOLS
- small change to bailout to be appropriate for both unchecked
exceptions and errors
- remove CURRENT_STEP and STEP_COUNT, no longer used
- small changes in the builddir check suggested by shellcheck.
@mobinmob
Copy link
Contributor Author

mobinmob commented Feb 27, 2023 •
edited
Loading

No longer [wip], as I am no longer going to work on it - I have completed the changes I set out to make. I will hapilly justify my changes and fix what is suggested if it relates to them.

@mobinmob mobinmob changed the title [wip] mklive.sh: start using lib.sh functions. mklive.sh: start using lib.sh functions. Feb 27, 2023
paper42
paper42 reviewed Mar 4, 2023
View reviewed changes
mklive.sh.in
XBPS_REPOSITORY="$XBPS_REPOSITORY --repository=https://repo-default.voidlinux.org/current --repository=https://repo-default.voidlinux.org/current/musl"

# mklive only supports x86_64 and i686 with glibc or musl, abort on other archs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this comment is really needed, I think the code is pretty clear about this.

mklive.sh.in
@@ -316,8 +301,13 @@ while getopts "a:b:r:c:C:T:Kk:l:i:I:S:s:o:p:v:h" opt; do
esac
done
shift $((OPTIND - 1))

# Check for the existance of required executables in the host system.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also I think that the check_tools name already tells people what it does

mklive.sh.in
umount_pseudofs
[ -d "$BUILDDIR" -a -z "$KEEP_BUILDDIR" ] && rm -rf "$BUILDDIR"
exit "${1:=0}"
# This source pulls in all the functions from lib.sh.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as below, I don't think this is necessary to document

mklive.sh.in Outdated Show resolved Hide resolved
@mobinmob
Copy link
Contributor Author

mobinmob commented Mar 5, 2023 •
edited
Loading

@paper42 Thank you for taking the time to review this PR.
I tried to add very few comments, because other shell scripts in the project have excellent comments and this one had almost none. My instinct is to be more verbose actually...
So, I am not going to remove them.

@classabbyamp
Copy link
Member

classabbyamp commented Mar 5, 2023 •
edited
Loading

why remove die/error_out and add bailout? there's a die() in lib.sh. and why no longer use print_step? IMO if we start putting => in front of info_msgs that should go into the function so it's uniform everywhere

I agree with paper on the comments, most of them are made totally redundant by reading the code. leave comments for complex or nonobvious code

@mobinmob
Copy link
Contributor Author

mobinmob commented Mar 5, 2023 •
edited
Loading

why remove die/error_out and add bailout? there's a die() in lib.sh. and why no longer use print_step? IMO if we start putting => in front of info_msgs that should go into the function so it's uniform everywhere

die() from lib.sh will remove $ROOTFS allways.
There is a need to keep the whole of $BUILDDIR because a user may need to inspect it in case of a problem, if they have specified -K .
I replaced error_out() with bailout() because a) there is a function named bailout already in the repo - in mknet.sh.in - and b) error_out() was working only with a different die() function than that which exists in lib.sh.
print_step() was removed because it is removed elsewhere in the repo (mknet.sh.in) when support for the lib.sh was introduced.
Introducing => in messages in lib.sh may be a good thing. It is used in both xbps-src and the runit-void scripts, so that will be consistent.

I agree with paper on the comments, most of them are made totally redundant by reading the code. leave comments for complex or nonobvious code

I can add more substantive comments (and more verbose) if that is the goal. I was prompted to add something due to the work that has been done in other scripts years ago by @the-maldridge ... Granted, my changes are much less extensive - the bare minimum to have lib.sh functions in mklive and have some consistency with other scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paper42 paper42 paper42 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mobinmob @classabbyamp @paper42