Skip to content
This repository has been archived by the owner on May 31, 2018. It is now read-only.

Add some comments to several functions #733

Merged
merged 3 commits into from
Jul 30, 2017
Merged

Conversation

ismaelgv
Copy link
Contributor

As we discussed in #443, I've added some comments to the source code in places where it was hard to understand or confusing to me. Please check them to be sure I understood correctly before merging this to pacaur50.

In addition, I've got observations on some of that functions:

IgnoreChecks()
IgnoreDepsChecks()

These two were particularly hard to understand even when their functionality is not that complex. I think that there is much repeated code here that could be converted to small functions, for instance, checking the ignore groups.

ProviderChecks()
ConflictChecks()

As you said, these functions are a bit convoluted, specially ProviderChecks(). However, their functionality is not simple and the problem they solve is not trivial either. Also, I think they are at least well documented.
Anyways, i think that some part of them can be splitted into simple functions.

ReinstallChecks()

I don't clearly get what are the initial checks to skip the packages:

[[ ! $foreign ]] && [[ ! " ${aurpkgs[@]} " =~ " ${depsAname[$i]} " || " ${aurconflictingpkgs[@]} " =~ " ${depsAname[$i]} " ]] && continue
[[ -z "${depsQver[$i]}" || "${depsQver[$i]}" = '#' || $(vercmp "${depsAver[$i]}" "${depsQver[$i]}") -gt 0 ]] && continue
[[ ! $installpkg && ! " ${aurdepspkgs[@]} " =~ " ${depsAname[$i]} " ]] && continue

OutofdateChecks()
OrphanChecks()
GetBuiltPkg()
CleanCache()
Proceed()

These functions are nice and a good example to follow when other functions are split. They do one thing at the time on a few lines.

Prompt()

Binary size computation could be extracted from here and put into an utils simple function.

EditPkgs()

Here, there are many nested loops and they are a bit hard to follow. Some functions could be created to handle this. I've noticed that options taken from global configuration and use as conditional make the code a lot of harder to read (p.e. $noconfirm).

MakePkgs()

I'm positive that a substantial part of this function can be extracted to new functions. A clear example is the fragment of code where is checked the integrity. I am also wondering if the new orphan checks couldn't use OrphanCheck() in some way. As you suggested, some parts of the code should be rewritten using a functional approach and this is a clear target to start with.

@@ -367,7 +372,7 @@ MakePkgs() {
Note "i" $"Removing installed AUR dependencies..."
sudo $pacmanbin -Rsn ${aurdepspkgs[@]} --noconfirm
fi
# readd removed conflicting packages
# read removed conflicting packages
Copy link
Owner

Choose a reason for hiding this comment

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

This is "readd" as in "add again". When building packages, and building only (-Sw), dependencies need to be installed to ensure proper successful build of targets. They are removed after operation. This particular line readd the dependencies that conflicted and had to be removed, so the final state of the system is the same as before the operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could extend that comment to make that a bit more clear.

@@ -300,7 +305,7 @@ MakePkgs() {
# build
Note "i" $"Building ${colorW}${pkgsdeps[$i]}${reset} package(s)..."

# install then remove binary deps
# build then remove binary deps
Copy link
Owner

@rmarquis rmarquis Jul 24, 2017

Choose a reason for hiding this comment

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

Install was correct, since it refers to repo dependencies. Here again this is done to handle -Sw correctly. Note I'm using "repo dependencies" and "binary dependencies" interchangeably, it might be a good idea to stick to one of them only (repo would have my preference since it is less confusing).

The seemingly complexity of that code is due to split package support. See b0d4860 and related 6b2c1ed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that is a bit confusing. In that comment you are referring to install the repo deps and then remove them after a successful compilation of the AUR package? makepkg -r option?

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, the comment is about the whole if then code block below. makeopts=(${makeopts[@]/-r/}) is done to unset the -r flag to ensure there won't be any issue with the next package to build.

That line might be moved inside the if then block, I'll have to check why I set it outside.

[[ ! $foreign ]] && [[ ! " ${aurpkgs[@]} " =~ " ${depsAname[$i]} " || " ${aurconflictingpkgs[@]} " =~ " ${depsAname[$i]} " ]] && continue
[[ -z "${depsQver[$i]}" || "${depsQver[$i]}" = '#' || $(vercmp "${depsAver[$i]}" "${depsQver[$i]}") -gt 0 ]] && continue
[[ ! $installpkg && ! " ${aurdepspkgs[@]} " =~ " ${depsAname[$i]} " ]] && continue
# devel packages are never considered to be reinstalled, use always lastest revision without check
Copy link
Owner

Choose a reason for hiding this comment

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

This is correct, but this is primarily due to the design decision of not checking the version of VCS packages before the main prompt (since this operation is so slow). In other word, with --devel --needed we don't know at this point if the VCS package will be rebuilt or not - and not knowing isn't a big deal either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I leave there the comment then?

Copy link
Owner

Choose a reason for hiding this comment

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

Not necessary, unless my comment made it clearer to you the reason this is done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I could extend that comment to include your design decision to explain why it is done that way.

@@ -393,9 +403,11 @@ ReinstallChecks() {
# global aurpkgs aurdepspkgs deps aurconflictingpkgs depsAname depsQver depsAver depsAood depsAmain
depsAtmp=(${depsAname[@]})
for i in "${!depsAtmp[@]}"; do
# check and skip packages with conflicts
Copy link
Owner

Choose a reason for hiding this comment

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

Yes, this is done to avoid false positive.

@@ -27,13 +27,15 @@ IgnoreChecks() {

checkaurpkgsAver=($(GetJson "var" "$json" "Version"))
checkaurpkgsQver=($(expac -Q '%v' "${checkaurpkgs[@]}"))
# set always the latest revision for devel packages
Copy link
Owner

Choose a reason for hiding this comment

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

Yes. This is done because the version commit we can retrieve through the RPC is most likely wrong (outdated).

@rmarquis
Copy link
Owner

As we discussed in #443, I've added some comments to the source code in places where it was hard to understand or confusing to me. Please check them to be sure I understood correctly before merging this to pacaur50.

First of all, thanks to you for taking the time to look at the code. Very much appreciated.

These two were particularly hard to understand even when their functionality is not that complex. I think that there is much repeated code here that could be converted to small functions, for instance, checking the ignore groups.

Yes, they are most the same function for different set of packages (AUR targets and their AUR deps). It might even be possible to make them entirely functional and have only one function for both use. I remember I had quite troubles to make them working right, it's very possible I overcomplexified them unecessarily. Complete review and rewrite is in order I guess.

As you said, these functions are a bit convoluted, specially ProviderChecks(). However, their functionality is not simple and the problem they solve is not trivial either. Also, I think they are at least well documented.
Anyways, i think that some part of them can be splitted into simple functions.

Yes. Using pacsift from pacutils will also simplify some of that code.

I don't clearly get what are the initial checks to skip the packages:

This is all done to avoid false positives in all the edges cases, for example 1/ with --needed or conflicting packages, 2/ packages provided by others or with weird versioning scheme, and 3/ with -Sw.

Some of that code is 3 or 4 years old, and I'm not excluding that some of that code is obsolete today. However, the main issue I see is that ReinstallCheck() is tightly coupled with the previous code instead of being independent.

These functions are nice and a good example to follow when other functions are split. They do one thing at the time on a few lines.

Good. Incidentally, these are part of code that can be considered "independent" and not tightly coupled.

Binary size computation could be extracted from here and put into an utils simple function.

Absolutely.

Here, there are many nested loops and they are a bit hard to follow. Some functions could be created to handle this. I've noticed that options taken from global configuration and use as conditional make the code a lot of harder to read (p.e. $noconfirm).

I'd even say the level of nested conditional code is absolutely insane. There is undoubtedly something broken in the mind of the guy that coded that. To his defense, I'd say this is mostly the result of incremental changes without making the required refactoring :)

I'm positive that a substantial part of this function can be extracted to new functions. A clear example is the fragment of code where is checked the integrity. I am also wondering if the new orphan checks couldn't use OrphanCheck() in some way.

You might confuse OrphanCheck() and locally orphan packages. The former warns about packages marked as orphan in the AUR, while the latter is about packages that not explictly installed but not needed by any other package. Sadly, the same "orphan" term is used for both in the Arch community, so this is a bit confusing.

The good thing about MakePkgs() is that it is pretty much independent from the rest of the code. It takes the ordered package names to build as input, and does the rest by itself.
Of course, some part can be decouped in subfunctions to reduce duplication of code.

As you suggested, some parts of the code should be rewritten using a functional approach and this is a clear target to start with.

Yes. The past few days I've had a closer look at makepkg and aurutils code, and pacaur could profit of a similar functional approach in many way. It might not be possible to obtain something as clean (after all bash sucks when working with arrays), but there is a large margin for improvement here.

Again, thanks a lot for your review.

@ismaelgv
Copy link
Contributor Author

Yes, they are most the same function for different set of packages (AUR targets and their AUR deps). It might even be possible to make them entirely functional and have only one function for both use. I remember I had quite troubles to make them working right, it's very possible I overcomplexified them unecessarily. Complete review and rewrite is in order I guess.

We could open an issue to track it down and make some code prototypes. We could even test it on this branch.

Yes. Using pacsift from pacutils will also simplify some of that code.

It sounds like a good idea.

This is all done to avoid false positives in all the edges cases, for example 1/ with --needed or conflicting packages, 2/ packages provided by others or with weird versioning scheme, and 3/ with -Sw.

I will add some comments to this part to clarify it until is rewritten/reviewed.

Binary size computation could be extracted from here and put into an utils simple function.

I think this can be easily extracted without much trouble.

I'd even say the level of nested conditional code is absolutely insane. There is undoubtedly something broken in the mind of the guy that coded that. To his defense, I'd say this is mostly the result of incremental changes without making the required refactoring :)

You made me laugh there. 😆

You might confuse OrphanCheck() and locally orphan packages. The former warns about packages marked as orphan in the AUR, while the latter is about packages that not explictly installed but not needed by any other package. Sadly, the same "orphan" term is used for both in the Arch community, so this is a bit confusing.

I did not think about that. Well, I suppose it is fine then.

The good thing about MakePkgs() is that it is pretty much independent from the rest of the code. It takes the ordered package names to build as input, and does the rest by itself.
Of course, some part can be decouped in subfunctions to reduce duplication of code.

Nice, that will ease further refactoring. Anyways, I will start with other simpler parts of the code since that is a pretty critical function that I do not want to break.

Yes. The past few days I've had a closer look at makepkg and aurutils code, and pacaur could profit of a similar functional approach in many way. It might not be possible to obtain something as clean (after all bash sucks when working with arrays), but there is a large margin for improvement here.

I will also take another look to those programs, they are a really good code reference.

Again, thanks a lot for your review.

Thanks to you for answering that fast! You really encourage me to contribute and I am happy to help here. 😄

I will try to make some changes on the code comments that you reviewed before as soon as possible. I am going to be offline/busy the next month but I think that I can complete this PR and maybe open one or two more with some simple stuff this week.

@ismaelgv
Copy link
Contributor Author

I've made some changes following your comments. In addition, I have renamed all relevant binary dependency to repo dependency in the comments to keep the consistency.

@ismaelgv
Copy link
Contributor Author

If these comments are OK I can squash and merge this PR to pacaur5.

@@ -27,7 +27,7 @@ IgnoreChecks() {

checkaurpkgsAver=($(GetJson "var" "$json" "Version"))
checkaurpkgsQver=($(expac -Q '%v' "${checkaurpkgs[@]}"))
# set always the latest revision for devel packages
# set always the latest revision for devel packages since RPC can be outdated
Copy link
Owner

Choose a reason for hiding this comment

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

"since the RPC data is static only" might convey the meaning better.

repodeps=($(tr ' ' '\n' <<< ${repodeps[@]} | sort -u))

# add initial repodeps
[[ -z "${repodepspkgs[@]}" ]] && repodepspkgs=(${repodeps[@]})

# get non installed repo deps
allrepodepspkgs=($(expac -S -1 '%E' ${repodeps[@]})) # no version check needed as all deps are binary
allrepodepspkgs=($(expac -S -1 '%E' ${repodeps[@]})) # no version check needed as all deps are repo
Copy link
Owner

Choose a reason for hiding this comment

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

"repo deps"

providerspkgs=($(tr ' ' '\n' <<< ${providerspkgs[@]} | sort -u))

# add initial repodeps
[[ -z "${providerspkgspkgs[@]}" ]] && providerspkgspkgs=(${providerspkgs[@]})

# get non installed repo deps
allproviderrepodepspkgs=($(expac -S -1 '%E' ${providerspkgs[@]})) # no version check needed as all deps are binary
allproviderrepodepspkgs=($(expac -S -1 '%E' ${providerspkgs[@]})) # no version check needed as all deps are repo
Copy link
Owner

Choose a reason for hiding this comment

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

"repo deps"

@rmarquis
Copy link
Owner

Sorry, I forgot about it. Minor changes only, otherwise ok!

@ismaelgv ismaelgv merged commit ae8f0a3 into rmarquis:pacaur50 Jul 30, 2017
@ismaelgv ismaelgv deleted the pacaur50 branch July 30, 2017 11:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants