Syntax, structure, indent and unneeded logic fixes #60
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -225,7 +225,7 @@ check_lxd_accessible () { | |
| # /bin/lxd is replaced with a script which tells you to install the lxd snap | ||
| # on later versions of Ubuntu. | ||
| LXD=`which lxd` | ||
| if (grep 'snap install lxd' "$LXD" > /dev/null) && [ ! -f /snap/bin/lxd ] ; then | ||
| $LXD | ||
| exit 1 | ||
| fi | ||
|
|
@@ -315,24 +315,24 @@ setup_lxd () { | |
| echo "this for you." | ||
| echo "Press Enter to start" | ||
| read KEY | ||
| unset KEY | ||
| # Ubuntu 16.04 specific workaround to set up | ||
| sudo dpkg-reconfigure -p medium lxd | ||
| fi | ||
|
|
||
| if [ -n "$ENCRYPTED_HOME" ] ; then | ||
| echo -e -n "${ERROR_COLOR}Your home folder is encrypted. $PROGRAM_NAME will use privileged " | ||
| echo -e -n "LXD containers and the default storage backend (slower).\n${NC}" | ||
| sudo lxd init --auto | ||
| else | ||
| echo -e -n "${POSITIVE_COLOR}Would you like to setup LXD with ZFS in your home directory? (y/n) \n${NC}" | ||
| echo -n "This is recommended for faster operation, and also in case there is not enough " | ||
| echo -e "space in your / partition." | ||
| read REPLY | ||
| echo | ||
| if [ "$REPLY" = y ] | ||
| then | ||
| if ! which zpool > /dev/null 2>&1; then | ||
| echo -e "${POSITIVE_COLOR}Installing ZFS.${NC}" | ||
| if which apt-get > /dev/null 2>&1 ; then | ||
| echo "sudo apt-get install -y zfsutils-linux" | ||
|
|
@@ -369,22 +369,20 @@ config_container_dir_mount () { | |
| } | ||
|
|
||
| start_container () { | ||
| STATUS=$(lxc query "/1.0/containers/${LXD_CONTAINER}/state" | jq --raw-output '.status') | ||
| if [ $STATUS = 'Stopped' ]; then | ||
| # lxc start may give a failure code. It also may not, so we check | ||
| # again in a bit. | ||
| if ! lxc start $LXD_CONTAINER; then | ||
| echo -e $LXD_CONTAINER_FAILURE_MSG | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| # Unfortunately we need to check again. We check for Started this time | ||
| # because the monitor may be hung with the container failing, which will | ||
| # give us much different output | ||
| STATUS=$(lxc query "/1.0/containers/${LXD_CONTAINER}/state" | jq --raw-output '.status') | ||
jezek marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if [ "$STATUS" != 'Running' ]; then | ||
| echo $LXD_CONTAINER_FAILURE_MSG | ||
| exit 1 | ||
|
|
@@ -401,7 +399,7 @@ nonsdk_container_setup () { | |
| focal) ubports_repo_line="deb http://repo2.ubports.com/ $container_ubuntu_version main" ;; | ||
| esac | ||
| if [ -n "$ubports_repo_line" ]; then | ||
| exec_container_root "echo '$ubports_repo_line' > /etc/apt/sources.list.d/ubports.list" | ||
| fi | ||
|
|
||
| # Skip multiarch setup if not crossbuilding | ||
|
|
@@ -423,7 +421,7 @@ nonsdk_container_setup () { | |
| exec_container_root "sed -E \ | ||
| -e 's:(archive|security)\.ubuntu\.com/ubuntu/:ports.ubuntu.com/ubuntu-ports/:' \ | ||
| -e 's:^deb :deb [arch=${TARGET_ARCH}] :' \ | ||
| /etc/apt/sources.list > /etc/apt/sources.list.d/ports.list" | ||
| exec_container_root "sed -i -E \ | ||
| -e 's:^deb :deb [arch=${HOST_ARCH}] :' \ | ||
| /etc/apt/sources.list" | ||
|
|
@@ -434,7 +432,7 @@ nonsdk_container_setup () { | |
| -e 's:ports\.ubuntu\.com/ubuntu-ports/ ([a-z]+)-security :security.ubuntu.com/ubuntu/ \1-security :' \ | ||
| -e 's:ports\.ubuntu\.com/ubuntu-ports/ ([a-z-]+):archive.ubuntu.com/ubuntu/ \1 :' \ | ||
| -e 's:^deb :deb [arch=${TARGET_ARCH}] :' \ | ||
| /etc/apt/sources.list > /etc/apt/sources.list.d/non-ports.list" | ||
| exec_container_root "sed -i -E \ | ||
| -e 's:^deb :deb [arch=${HOST_ARCH}] :' \ | ||
| /etc/apt/sources.list" | ||
|
|
@@ -482,7 +480,7 @@ create_container () { | |
| exec_container_root "add-apt-repository -y ppa:ubports-developers/overlay" | ||
| exec_container_root "add-apt-repository 'deb http://repo.ubports.com vivid main' >> /etc/apt/sources.list" | ||
| fi | ||
| wget -qO - "https://repo.ubports.com/keyring.gpg" | exec_container_root 'cat > /etc/apt/trusted.gpg.d/ubports-keyring.gpg' | ||
| if ! echo "$LXD_IMAGE" | grep -q "ubuntu-sdk"; then | ||
| nonsdk_container_setup | ||
| fi | ||
|
|
@@ -704,7 +702,7 @@ install_foreign () { | |
| exec_container "mkdir -p foreign && cd foreign && apt-get download $dep && dpkg-deb -R $dep* tmp" | ||
| exec_container "grep -vi '^multi' foreign/tmp/DEBIAN/control > foreign/tmp/DEBIAN/control.tmp" | ||
| exec_container "mv foreign/tmp/DEBIAN/control.tmp foreign/tmp/DEBIAN/control" | ||
| exec_container "echo 'Multi-Arch: foreign' >> foreign/tmp/DEBIAN/control" | ||
| exec_container "cd foreign && dpkg-deb -b tmp $dep*" | ||
| exec_container_root "dpkg -i $SOURCE_PATH_CONTAINER/foreign/$dep*" | ||
| exec_container "rm -r foreign/" | ||
|
|
@@ -760,8 +758,8 @@ copy_build_to_container () { | |
| lxc file push $SCRIPT_DIR/$CREATE_REPO_SCRIPT $LXD_CONTAINER$SOURCE_REPOSITORY/ | ||
| exec_container $SOURCE_REPOSITORY/$CREATE_REPO_SCRIPT $SOURCE_REPOSITORY | ||
|
|
||
| exec_container_root "echo 'deb [trusted=yes] file://$SOURCE_REPOSITORY/ /' > /etc/apt/sources.list.d/localrepo.list" | ||
| exec_container_root "printf 'Package: *\nPin: release o=local\nPin-Priority: 2000' > /etc/apt/preferences.d/localrepo.pref" | ||
| } | ||
|
|
||
| clean () { | ||
|
|
@@ -773,9 +771,7 @@ check_for_container_network() { | |
| NETWORK_UP=0 | ||
| for i in `seq 1 10` | ||
| do | ||
| if lxc query "/1.0/containers/${LXD_CONTAINER}/state" | jq -e '.network.eth0.addresses | any( .family == "inet" )' > /dev/null 2>&1 ; then | ||
|
There was a problem hiding this comment. also this one, please let it be in multiple lines :-) There was a problem hiding this comment. I agree, this one is longer, but still not the longest. There are 6 lines longer than this and not splited. So why this and not the other? Also the multi-line version is inconsistently indented too, and there is output redirection alone on the third line, which can mislead for which command it is for. There was a problem hiding this comment. I'm not very consistent on this, admittedly; but if we are making cosmetic style changes, let's go towards shorter lines please. :-) Extra points if you propose splitting the other long lines too :-) There was a problem hiding this comment. If I may ask again? How do you differentiate between long and short lines, where is the line, where the short line becomes to long? For me speaking, no line is too long. I use editor, which wraps lines. When I split, it is to improve readability of complicated 'if' statements. Also more lines will be shorter if the indentation is not 4 spaces (on this my opinion is to use 'tab' and anyone can set editor to his liking on how much spaces will be used for tabs). There was a problem hiding this comment. A line width of 79 chars max is the most safe, and people can rely on that to keep multiple editor windows opened side by side without having automatic line wrap. Yes, most editors have automatic line wrapping, but it's still easier to read code where lines are split manually. There was a problem hiding this comment. So you want from me to keep the lines most 79 characters long and you introduced more longer lines few commits ago (eg. this one)? This lacks consistency from your side. Edit: Btw. 79 chars for line is too short. Even Linus agrees with this. |
||
| NETWORK_UP=1 | ||
| break | ||
| fi | ||
|
|
@@ -894,7 +890,7 @@ deploy_to_device () { | |
| fi; | ||
|
|
||
| # setup sudo on device | ||
| exec_device "printf '#\041/bin/sh\necho $DEVICE_PASSWORD' > /tmp/askpass.sh" | ||
| exec_device chmod +x /tmp/askpass.sh | ||
|
|
||
| # check password is correct | ||
|
|
@@ -1068,10 +1064,11 @@ if stat --file-system $HOME | grep ecrypt ; then | |
| fi | ||
|
|
||
| if [ -e "/snap/bin/lxc" ]; then | ||
| echo "Force privileged container" | ||
| FORCE_PRIVILEGED=1 | ||
| fi | ||
|
|
||
| if ! which lxd > /dev/null 2>&1 ; then | ||
| echo -e "${POSITIVE_COLOR}$PROGRAM_NAME uses LXD to download dependencies and build.${NC}" | ||
| echo -e -n "${POSITIVE_COLOR}Would you like to install LXD? (y/n) ${NC}" | ||
| read REPLY | ||
|
|
@@ -1094,8 +1091,8 @@ if ! which lxd > /dev/null ; then | |
| fi | ||
| setup_lxd | ||
| ensure_lxd_subuid | ||
| echo "Adding user to lxd group" | ||
| sudo usermod -a -G lxd $(whoami) | ||
| echo -e "${ERROR_COLOR}LXD is now setup but will only work after you restart your computer.${NC}" | ||
| exit 0 | ||
| else | ||
|
|
@@ -1365,7 +1362,7 @@ else | |
| *) | ||
| display_help | ||
| echo "" | ||
| echo -e "${ERROR_COLOR}Error: unknown command: $COMMAND${NC}" | ||
| exit 1 | ||
| ;; | ||
| esac | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 would reather keep this as it was.
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.
May I ask why? I just tried to be consistent. There is nowhere in code splited line after pipe. And there are longer lines that are not splited just some lines above. So why split just this one? Also the splited original is inconsistently indented.
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 don't have a strong opinion on where the line should be split, but I have a preference for avoiding long lines.
If you want to make it more consistent and fix the indentation, you are very welcome to :-)
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.
In my opinion the one-liner is more consistent and fits better with surrounding code. I presented my arguments, heard yours and come to conclusion, that I won't revert or change this. If you want it splited, do it yourself or don't merge this commit.
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.
OK, I've calmed down a little. I'm sorry, yesterday I was a little cranky.
Please, don't get me wrong, I don't want to be disrespectful and I want to resolve our small dispute about long line breaks. I still think, that the lines don't need breaks and your argument about 79 characters per line is invalid, because you don't follow the rule yourself. But I'm willing to compromise.
How about this. This (L:280)and the line in next conversation (L:293) will not be broken into 2 lines and the line in last conversation (L:664) will be split into 2 lines. Is this OK for you?
If not, then I propose to let the some other member/contributor to make the decision. We can make a pool and will follow the decision of the majority.
So @mardy , which one will you choose? Or do you have another proposal?
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.
Let me be clear: I generally don't like formatting changes, they just add noise to the git history and make reverting commits harder.
If you proposed a commit which introduced a long line I most likely would not complain, because I don't think it's worth bothering contributors about it (given that we have plenty of them already). As you pointed out, I myself have added long lines, without being aware. I complained this time because you changed the formatting of an existing line (and this goes back to the first point: avoid formatting changes).
To sum it up, my philosophy is the following: I don't like formatting changes. They are fine, however, if they go in the direction of a style I'm more comfortable with.
So, while I appreciate that you proposed a compromise (joining these lines, and breaking some longer one), it is in fact not what I like, because it generates noise and those joined lines might be broken up again in the future...
I can also offer a compromise: do not touch these two lines, and feel free to add long lines (well, with a certain common sense :-) ) in your new code. :-)
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 clearly don't know what a compromise is. What you propose is not a compromise, so I can't accept it.
This commit doesn't exist, because I woke up & decided to make formatting changes. I was working on making the script work on non-debian based distros, literary manjaro (but you already know what I'm writing about). And when I work on something and I see some
trashmess on the workspace, then I dispose of it, because it is distracting. A clean, uniform working environment is more efficient & less distractful for everyone. So in the process of reading & understanding and editing the code, I've got rid of those distractions. Then you pointed out that these small fixes should go to separate PRs, so here we are.About these 3 lines. Whilst working on the manjaro fixes, I've stumbled upon the errors due to
lxc infochanges like @peat-psuwit did and made similar fixes as him. Then his changes got accepted, so I rebased and when resolving the conflict I joined the lines together. I folloved the logic, that the statements were one-liners before, so they should be a one-liners after. I accept, that the if statement is kind of long, but there is a longer if statement in the document and none of the 87 if statements are not multi-lined so why exactly this one? (see for yourself withgrep -P '^\s*if\s' crossbuilder | awk '{ print length, $0 }' | sort -n -s | cut -d" " -f2-in the repo dir)This is a community project, not yours. Requests should not be merged by what you are comfortable with or what you like, but what is logical and consistent. If you don't do formatting fixes because you don't like it, the code will go ugly and distractful in time and other people will be distracted by them and they will start to do formatting fixes and distract you even more.
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 you should try to be more respectful of other people's work. I'm not sure if by "trash" you mean the splitted lines that Ratchanan added, and which I happily accepted. I find broken indentation distracting at times, so I'm grateful that you want to fix that, but I find long lines more distracting than short lines, even if not properly indented.
As you wrote, this is a community project, and from my side I try to be welcoming of all contributions, accepting most of them as-is and not nitpicking on the format (up to a certain point, at least). The reason why we are hitting the wall with this PR is because it is changing the format itself: for such PRs, of course, I want us to try following my preferred style (which I assume is also Ratchanan's, since he deliberately avoided long lines in his commit), otherwise someone else (or even I myself) might come up some day with other cosmetic changes which partially revert the ones you are proposing here. That would just add noise in the git history, if we redo the same line over and over according to the preference of whoever happens to be "disturbed" by the current coding style.
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'm sorry, trash was a unfortunate word. I meant it figuratively. I meant no disrespect toward others work. For this I humbly apologize. The better word would be a "mess" in meaning of "no order".
To avoid other cosmetic changes, you have to make clear rules. If there are no rules, uniformity should be pursued (which I do now). So, if you (and others) don't like long lines, write rules to code of conduct, or make all long line split (no only a few).
I say it again. I'm not here for quarrel. I pursue uniformity. You could always ask me to split all the long lines and I will do it (I don't mind doing repetitive work, as long it makes code more uniform). But then we must agree on line length. Your proposal of 79 chars is too conservative. I'm proposing the maximum line length for this project 140 chars (120 is acceptable too).
Another way to go is to say, that the lines we discuss about are splited because of beter readability of piped commands. Then I will split all commands with pipes and don't have a problem.
Make/choose clear sane rule(s) and I will follow them and gladly rebuild the source code according to them.
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'm fine with your proposal to decide on a line length, and my preference, as already stated, is 79. I also believe that we shouldn't be too strict about this -- exceptions can be made, if in some cases staying within 79 chars makes the code over-convoluted.
@peat-psuwit, what is your preference?