Code review comment for lp:~deeptik/linaro-license-protection/publish-to-snapshot

On Tue, 13 Mar 2012 09:55:27 -0000
"Deepti B. Kalakeri" <email address hidden> wrote:

> >
> > 9
> > parser.add_argument("-a", "--archive_info", dest="archive_info",
> > 10
> > help="Specify the job which resulted the
> > archive to be stored.\
> > 11
> > Ex: ${JOB_NAME}/${BUILD_NUMBER} should be
> > specified for \
> > 12
> > andriod and for \
> > 13
> > kernel-hwpack
> > ${KERNEL_NAME}/${KERNEL_JOB_NAME}")
> >
> > This seems like confusing option. Reading the help, I don't have an
> > idea why it is called "archive_info". Nor reading help I have clear
> > picture what and how it should be used/works - that's too much
> > specifics, out of context. This script is supposed to be called
> > from Jenkins, and Jenkins cab basically pass to it it job name (in
> > its native format) and build number. That's what script should
> > accept, and help for options should describe just that, any
> > specifics like the fact that job name have internal structure based
> > on --job-type should be left to manual IMHO.
>
> There is not archive_info anymore. This was an old change.
>
> > Also, short option assignment is not ideal IMHO. For "job type",
> > main word is (t)ype. For "kernel tree" it's (k)ernel (or
> > (r)epository if should be generalized), "build number" is
> > apparently (n)umber, which leaves "job name" to (j)ob.
>
> Good suggestions. done.
>
> > 60 + if args.job_type == "android":
> > 61 + build_path = '/'.join([args.job_type, args.user_name,
> > args.job_name])
> >
> > Well, nope, that's not correct. There's no "user name" on Jenkins
> > level, only job name. That's what this script accepts. It then can
> > reformat native Jenkins name into "external" name which we want to
> > see.
>
> Yes I agree that there is no username at jenkins level. But for
> android, I believe the jenkins-post-www.sh script line 38 refer to
> $TARGET_PATH/~$username/$jobname.

Yes, and those are parsed from Jenkin job name in line 32 and on:
http://bazaar.launchpad.net/~linaro-infrastructure/linaro-license-protection/trunk/view/head:/scripts/jenkins-post-www.sh#L32

> So I just tried to retain the extra
> work of separating the jobname into username + jobname. Let me know
> if this is not required and what exactly did the username in the
> script meant to be. I will make the changes accordingly.

So, the script should accept ${JOB_NAME} from Jenkins (we simply
don't have anything more specific on its level), but then it should be
ready to parse/convert that into job-type-specific target path (i.e.
this conversion will be different for different job-type). So, I would
suggest to add job_to_target_subdir(job_name, build_no) method to
encapsulate this conversion. For android case, it will be equivalent of
lines 32-38 in jenkins-post-www.sh .

>
> > 71 + if not (os.path.isdir(uploads_path) or
> > os.path.isdir(build_dir_path)):
> > 72 + print "Missing build path", build_dir_path
> > 73 + return FAIL
> >
> > The error message is incorrect - it checks 2 dirs, but reports
> > problem as if applies to only one. Such cases are always great
> > source of confusion and suffering (the latest one, (c) by Google:
> > https://bugs.launchpad.net/bugs/949100)
>
> I agree. Modified this to correct the error message.
>
> > 79 + try:
> > 80 + # Make a note of the contents of src dir so that
> > 81 + # it can be used to validate the move to destination
> > 82 + uploads_dirList = os.listdir(build_dir_path)
> >
> > That sounds complicated and overinsuring. Can't we make requirement
> > that upload dir and destination dir were on the same FS, so we can
> > just use mv? Even if we can't, why wouldn't we trust
> > shutil.copy2()? It either does the copy, or throws an exception, so
> > we can't miss failed move.
> >
>
> I did consider using the mv action for this. shutil.move has the
> following issue: 1) The destination dir should be non-existent. so in
> case of kernel hwpacks we intend to store all the hwpacks of one
> category in a single dir. so this move would fail. Even if we wanted
> to just move the files under the directory instead of the directory
> itself, the amount of code would be the same for copy and move. 2)
> move tends to internally use the shutil.copy2() when the source and
> the destination are not on the same FS. So , shutil.copy2() would be
> handy instead in case we moved source and destination on diff FS.

Well, there's also os.rename() which is complete equivalent of shell mv
command - it will atomically move a directory into another within one
filesystem. But well, it seems that jenkins-post-www.sh istelf does cp
instead of mv (but probably because we wanted to be "on safe side"), so
if you think copy would be better, ok.

But I still think that extra checking destination contents after the
copy is superfluous - it either succeeds, or throw an exception.

>
> > Minor note - would be nice to use consistent var naming convention,
> > like uploads_dir_list.
> Done.
>
> > 111 +def main():
> > 112 + uploads_path = '/srv3/snapshots.linaro.org/uploads/'
> > 113 + target_path = '/srv3/snapshots.linaro.org/www/'
> > 114 + uploads_path = '/tmp/uploads/'
> > 115 + target_path = '/tmp/www/'
> >
> > IMHO, it would be better to treat these as the constants, not as the
> > variables, to be defined in the beginning of the script (like they
> > were before r50).
>
> Ok, noted.
>
> I will submit the changes in sometime.
>
> Thanks!!!

Thanks for all the changes. Another note - did you see
http://bazaar.launchpad.net/~linaro-infrastructure/linaro-license-protection/trunk/view/head:/scripts/jenkins-post-www.sh#L43 ?
That line (generation of MANIFEST file) is required for Android Build
Frontend, so please add --manifest switch to generate it (well, that
probably can go into separate change, just shouldn't be forgotten, so
please add it to WIs at least).

> Deepti.
>

Thanks,
Paul

Linaro.org | Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro
http://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog