[SSSD] [PATCH] CI: Improve usability of ci script

Lukas Slebodnik lslebodn at redhat.com
Fri Sep 12 15:18:23 UTC 2014


On (12/09/14 16:43), Nikolai Kondrashov wrote:
>Hi Lukas,
>
>Thank you for the fixes. Please see my comments below.
>
>On 09/12/2014 10:22 AM, Lukas Slebodnik wrote:
>>On (11/09/14 23:09), Nikolai Kondrashov wrote:
>>>On 09/11/2014 09:16 PM, Lukas Slebodnik wrote:
>>>Um, "shared_variables.sh" module name is too long, too generic and doesn't
>>>really make much sense having, akin to having "code.sh".
>>>
>>You proposed name of module "src.sh". This name does not say anything.
>>We are no more in 80's in DOS world. There is no more limitation for name 8+3.
>>I prefer to have long self descriptive name.
>
>I wouldn't mind much having it named "source.sh", although following the
>convention having all module symbols prefixed with module name, that would
>make all symbols longer.
>
1) "source.sh" does not say anything. Source of what?
   "shared_variables.sh" exactly says what is in this module.
2) What kind of convention do you talk about? You can have some conventions
in your mind, but I don't know them. I could just guess them.

>However, now I think it wouldn't be worth much making a module just for it.
>It would be an overkill.
>
It is late, you proposed it and now you change your mind. The idea with shared
module is good, but your proposal for name isn't good. they are really
confusing or aren't self describing.

>>>There is no need to put WORK_DIR in a module, as only contrib/ci/run uses it,
>>>but if you really want to have it in a module, together with SRC_DIR, please
>>>put both of them into "misc.sh", which is just the place for mixed things
>>>which don't have a module of their own (yet).
>>>
>>It was your request in mail [1] to move this variable into shared module.
>>There isn't reason to put them into module "misc.sh". The name of module
>>"misc.sh" evokes name "garbage" or "I don't know find proper name for module".
>
>Yes, it is exactly that. Such modules are inevitable. It is not worth trying
>to categorize a handful of symbols, if each of them would need a separate
>module with a longish name.
>
"Code is read more than it is written". Long name are more descriptive.
and there isn't any problem with creating more modules with long name.
We don't have limits for amount of modules.

>The purpose of such (misc.sh or util.sh) modules is to keep symbols which
>can't be easily attributed to an existing module and are not worth creating a
>dedicated module for. Once such a "garbage" module accumulates symbols worth
>having in a separate module, such module should be created and these symbols
>should be moved there.
>
misc is abbreviation of miscellaneous

The free dictionary says:
miscellaneous
adj
1. composed of or containing a variety of things; mixed; varied
2. having varied capabilities, sides, etc

In other words: ANYTHING.

"util.sh" evokes helpful functions. This name is small and good, but it is
just an exception. In most cases, sort names aren't good. You as an author can
know what is in module "misc.sh", but anyone else have to look into this module
and read the content and find out what is there.

Imagine:
If anyone want to find some utility function it should look into "util.sh".
If anyone want to add new dependency it will look into "deps.sh" (btw better
name would be dependencies.sh). but "misc.sh" does not say anything.
Actually it does: "I don't know (I am lazy to) find proper name for module"

>>It's better to have self descriptive name otherwise you need to look into
>>the module itself  to find out what it actualy does.
>
>I agree in general, but in this case "shared_variables.sh" doesn't say much
>more than "misc.sh", yet is actually more confusing. Why it contains only two
>shared variables and the rest is in other modules (such as DEPS_LIST,
>DISTRO_ID)? Or, if they all were in this single module, wouldn't it be worse?
>
"shared_variables" says that this module contain shared variables and nothing
else. I can file a ticket to rename file "misc.sh" to more descriptive,
but there is no reason to add another variables to to module with confusing
name.

>>>>Subject: [PATCH 2/2] CI: Improve usability of ci script
>>>
>>>Could you please make the subject more specific? It's a bit too general now.
>>>Something like "CI: Allow running outside source root" would be better, IMHO.
>>>
>>The title exactly says what is a purpose of this patch.
>
>It describes it accurately, but not exactly enough. Of course, we cannot be
>very exact in a commit subject with its length limitation, but we can make
>them exact enough to help distinguish the commits, which is the purpose of
>subjects.
>
One more time:
The purpose of this patch is to improve "usability of ci script" and the title
of this commit message says it. You should not interchange title of commit
message with body of commit message.

>>The curret version of CI script is inflexible and very limiting from user
>>point of view. You have to execute script from one particular directory.
>>This is a very big limitation.  Attached patch remove this limitation and
>>add ability to execute script from any directory. For example, you can
>>execute scipt from ramdisk and save creating files in SSD disk. (I don't
>>have one), but it is still faster to build in ramdisk than in HDD.
>
>I agree that this change makes CI more flexible and less limiting.
>
>However, I'm sure there will be more commits doing such improvements. Shall we
>put the "Improve usability of ci script" subject on all of them? Would that make
>it easy to distinguish them when viewing the log? I doubt that.
>
No, because you should implement Michal's proposal and it will need broader
discussion about next version. It is very likely that it will be significantly
rewritten, because current version cannot be modify easily to fulfill
requirements.

In another words, current version somehow works, but it isn't very flexible and
it is difficult to extend it. (--tests=....)

So the title of the next patch should be "CI: refactor ci script"

>>>>+declare LOG_DIR="$PWD"
>>>>+declare -r WORK_DIR="$PWD"
>>>
>>>I would really prefer "WORK_DIR" and "LOG_DIR" being called "WORK_ROOT" and
>>>"WORK_DIR" respectively, or something similar instead. Because LOG_DIR is
>>>always at or below WORK_DIR and they're used to point to directories not only
>>>containing log files, but also builds and reports.
>>>
>>In mail [1], you proposed either WORK_DIR or WORK_ROOT.
>>My preference is to use WORK_DIR. It is more suitable.
>
>Um, I actually proposed both:
>
>    > +declare -r CWD_DIR="$(pwd)"
>    > +declare LOG_DIR="$(pwd)"
>
>    Can you name these "WORK_ROOT" and "WORK_DIR" instead?
>
>Perhaps I could have made it clearer.
>
>I meant this:
>
>WORK_ROOT (CWD_DIR formerly, WORK_DIR currently) should point to the root of
>CI output directories, i.e. the directory from where it was invoked.
>
Please do not mention former names. It is confusing, because the latest version
of patch does not have them. i accepted that comment and name was changed.

>WORK_DIR (LOG_DIR currently) should point to the current CI output directory,
>i.e.  should take value of $WORK_ROOT, $WORK_ROOT/ci-build-debug and
>$WORK_ROOT/ci-build-coverage.
>
WORK_DIR is alias to the directory which was script executed from. It is read
only variable and all log files and build files should be stored relative to
this directory. There isn't bi difference between WORK_ROOT, WORK_DIR
WORK_DIR != CURRENT_WORK_DIR

If you don't want to give an options to choose a name don't do it.
You proposed it; the name was chosen. I would appreciate if you accept
your own proposal.

LS



More information about the sssd-devel mailing list