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

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Fri Sep 12 13:43:03 UTC 2014


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.

However, now I think it wouldn't be worth much making a module just for it.
It would be an overkill.

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

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.

> 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?

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

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

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

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.

> At the moment LOG_DIR is always at or below WORK_DIR, but there is no reason
> for such limitation. The main problem is that you put log files and build
> files into the same directory. With current version it will be easier
> to separate them and have log files and build files in different directories,
> but I did not want to break backward compatibility with your scripts.
> On the other hand, patch would be simpler if I did it.

I'm willing to continue this discussion, but let's focus on the patch at hand
for now.

>> Yes, right now LOG_DIR is only used in log output, but essentially it is
>> pointing to a directory where current CI output goes and once we'll need to
>> point to that directory while being in another one and output something else
>> than log files there, we'll need that variable renamed.
> I didn't get your point. "CI output" is not the same thing as log files.
> In current version, "CI output" is supper set of ci log files.
> It is not problem of attached patch, which slightly improve it.
> It is a bad design decision of 1st version, which I didn't want to completely
> break.

Let's leave the judgement and discussion of that design decision for a
separate thread. The current patch tries to make the current design
independent of the directory it is invoked from. Let's not have it speculating
about how it could have or should have been done, but instead let it fit
well with the current design.

We can change the design in a separate patch, if necessary.

Thank you.

Nick



More information about the sssd-devel mailing list