test reports success despite build missing

RESOLVED FIXED in 3.11.9

Status

P3
normal
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: julien.pierre, Assigned: biswatosh2001)

Tracking

3.11
3.11.9

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 9 obsolete attachments)

(Reporter)

Description

13 years ago
The following test passes in the nightly QA even if the build failed :

 Tstclnt failed in an empty dir 1 	Passed

This needs to be fixed.

Comment 1

13 years ago
In the current case, we have a test that is supposed to fail (use tstclnt on an empty DB directory). If tstclnt does not exist, there is also a failure that is interpreted as a success in the context of this test case.

    Echo "test opening the database readonly in an empty directory"
    mkdir $EMPTY_DIR
    tstclnt -h  ${HOST}  -d $EMPTY_DIR
    ret=$?
    if [ $ret -ne 1 ]; then
      html_failed "<TR><TD> Tstclnt succeded in an empty directory $ret"
    else
      html_passed "<TR><TD> Tstclnt failed in an empty dir $ret"
    fi


I think that we should abort the tests at the beginning when the build is incomplete rather than fixing each test case to make sure that it fails when the needed tool is missing.

Should checking for "certutil" be good enough ?
Or another single check would be preferred ?
Or do we need to check for every single library / tool ?
IIRC, the build stops on the first failure, so it should suffice to check
for the presence of the last file normally built.  I suggest that you Look 
at the tail end of a typical build log to see what command is the last one 
built, and check for that file in the tests.

Comment 3

13 years ago
That's probably safer. Thanks for the suggestion.

The last thing we build is "modutil". This is what we should test first.
(Reporter)

Comment 4

13 years ago
If we test for modutil now, more tools could be added to the list in the future and may not be built. We could have the NSS "all" target in mozilla/security/nss touch some file after it is done with lib and cmd, and test for that instead.

Updated

13 years ago
Priority: -- → P3

Updated

11 years ago
Assignee: nobody → biswatosh2001
(Assignee)

Comment 5

11 years ago
Created attachment 282366 [details] [diff] [review]
patch ver1

I am using the idea suggested in comment #4.

The changes done are in two files, in nss/coreconf/rules.mk and
nss/tests/dbtests/dbtests.sh.

Major points:

1)If gmake is done at the top level, (that is, in security/nss/), then after
a successful build, an empty file (buildsuccessful.log) is created
in coreconf/.

2)If "gmake clean" is executed from the top level, after a sucessful
cleaning, buildsuccessful.log is removed fom coreconf folder.

3)If buildsuccessful.log file is already there in coreconf, and
at the top level(nss/), gmake is executed, it first removes the file
(buildsuccessful.log) and then goes on building nss. So, if it it fails
in between, that log file won't be there.

4)dbtest.sh exits if buildsuccessful.log is not found.

Limitations:

5)"$gmake clean" done at any other level except from nss/, for example
from nss/cmd/ or from nss/lib/certhigh etc., won't remove the log file.
But, do we wan't it? 

6)Hence, creation of the log file and it's deletion are both from
the top level.

7)Do we now want even all.sh to exit if the log file is not there?
Currently, all.sh would go on executing until it calls dbtests.sh.
Attachment #282366 - Flags: review?(christophe.ravel.bugs)
(Assignee)

Comment 6

11 years ago
Created attachment 282505 [details] [diff] [review]
alternative patch

Christophe,

This patch is basically same as the earlier patch but with
the difference that in this new one, a "gmake clean" at any level,
will first remove the "buildsuccessful.log" file from it's
location and then go on cleaning. 
The earlier patch removed it
when you do "gmake clean" only from top and after all the clean is done.

If you like the second approach, that is, "remove the logfile the moment
clean starts at any level",  I will give this patch for review then. Other
wise, you can review the previous patch.

Comment 7

11 years ago
Comment on attachment 282366 [details] [diff] [review]
patch ver1

We can run several builds with different options (DBG/OPT, 32/64 bit) or on different platforms on the same workspace. Your patch uses a single file that does not take this into account. Look at how directories are created under mozilla/dist (.OBJ dirs).
Attachment #282366 - Flags: review?(christophe.ravel.bugs) → review-
(Assignee)

Comment 8

11 years ago
Created attachment 283726 [details] [diff] [review]
patch ver2

Thanks Christophe for pointing to that gap. 

1)This attachment creates "buildsuccessful.log" in
/mozilla/dist/{Platfrom specific directory}/, such as
in /mozilla/dist/Linux2.4_x86_glibc_PTH_OPT.OBJ/ etc.
It uses OBJDIR value in rules.mk.

2)Plus, if at any level, "gmake clean" is executed, it removes
that file from that dir and then proceeds to clean.

3)dbtests.sh now tries to see whether this log file exists
in the platform specific dir in /mozilla/dist/. But, on it's own,
it does not
have the value of OBJDIR. Only when all.sh is run, OBJDIR
is exported and then dbtests.sh can use that to locate the
log file. But, even then, if somebody wants to execute dbtests.sh
directly, he can do that, provided he sets OBJDIR to the correct value,
such as "export OBJDIR=Linux2.4_x86_glibc_PTH_OPT.OBJ".
And, if he does not want to do that, as said above, he can always
run all.sh by exporting TESTS=dbtests and OBJDIR will be exported
and dbtests.sh can use that.

4)I tested this patch with the same workspace but with OPT and DBG versions
attached to it and it seemed to work. 
Thus, with the same workspace but building the lib with different
platforms, will create several log files(with the name buildsuccessful.log) under /mozilla/dist/*/ but each platform specific directory under
/mozilla/dist/ will contain only one copy of this log file.
Attachment #282366 - Attachment is obsolete: true
Attachment #282505 - Attachment is obsolete: true
Attachment #283726 - Flags: review?(christophe.ravel.bugs)
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
Comment on attachment 283726 [details] [diff] [review]
patch ver2

There are many problems with this patch.
1) it creates a "log" file which contains no log.
2) the path in which it creates the log file is incorrect for builds in read-only file systems.
3) coreconf is used to build things other than NSS.  This patch embeds knowledge of NSS-specific directories into coreconf.  

There are better ways to determine if the build succeeded or failed.  
Changes to coreconf are not required.
Attachment #283726 - Flags: review?(christophe.ravel.bugs) → review-
(Assignee)

Comment 10

11 years ago
(In reply to comment #9)
Point 1) The confusion is because of the "log" extension.
Another name, such as "buildmade" can be given to the file
instead of the earlier proposed buildsuccessful.log.

Point2) The location for the file has to be different. 
Fine. How about in nss/tests ?

Point 3)This seems to be the most important point. So, this 
suggests that coreconf need not be touched at all.

Now, are we basically agreeing on the following approach (given
in points a, b and c below) ?

a)Only when the whole build is made, should the file "buildmade"
be created in some fixed location, say in mozilla/security/nss/tests ?

b)When a "gmake clean" is done at any level, such as at security/nss
or at security/nss/cmd or in security/nss/cmd/certutil etc., the first
thing to be done is : remove the file buildmade from it's location and
then go ahead with the regular cleaning process. 
The reason is, buildmade file has to be there WHEN and ONLY WHEN the build
is fully successful. So, we should not remove the file after the regular
cleaning process is done because what if we type a control-c in between and
then we have an incomplete build with the buildmade file still lying there ?

c)In the solution, we are not going to use any particular existing filename.
See Comment #3 and Comment #4. I feel it is better to take Julien's approach
made in comment #4. 


Point (a) can be solved by making a small change in nss/Makfile viz.
*******************
nss_build_all: build_coreconf build_nspr build_dbm all
+all::
+       touch $(CORE_DEPTH)/nss/tests/buildmade
*******************

For point (b), the ealier patch was able to solve it by touching
coreconf file rules.mk. But, if we decide to not touch coreconf at all,
then can we do that by not touching lots of files under security/nss ?
Any "clean:: 
           rm  $(CORE_DEPTH)/nss/tests/buildmade
    "  directive has to be entered in all the Makefiles or by doing it in one 
file and including that in others. Both mean touching huge number of files.

I am trying to think of a better solution but it is very important to
know whether the approach given in points a, b and c are OK or not?
You don't need to create any new special "build done" file.
The build does nothing but create files. 
Look and find the last file that is normally built by a successful 
build, and test for that file.  
That requires no changes to the build system at all, and requires
only changes to the test script.  

It is possible to build NSS from a source tree on a read-only file
system.  In that case, NSS does not touch (create, alter) any files
in the source tree (nothing under mozilla/security).  Any commands
in the makefiles that ALWAYS touch a file in the source tree will 
break NSS's ability to build in a read-only source tree.  That is 
why I said the location of the file was unacceptable.  Moving it to
another location in the source tree is equally bad.  Moving it to a
directory that is not associated with the build, such as /tmp is also
bad, because it doesn't allow multiple builds to run on the same 
system at the same time.  I could explain how read-only source builds
work, but in this case, it's unnecessary, because (as I explained
above) there is no need for the build system to touch even a single
new file that it did not build before.
(Assignee)

Comment 12

11 years ago
Created attachment 286002 [details] [diff] [review]
Patch ver 3.

This takes the approach suggested in Comment #2 and 
Comment #11. The earlier patch was based on Comment #4.

1)It notices that modutil is the last file created in 
mozilla/dist/*/bin/ while doing a make at the top.
If it exists, it assumes the build was successful and
dbtests.sh is executed. Otherwise it exits.

2)If one wants to execute dbtests.sh directly, he needs
to set OBJDIR first and then run it. Otherwise, he can
always set TESTS to dbtests and run all.sh to execute
dbtests.sh. In this case, setting of OBJDIR is not 
required.
Attachment #283726 - Attachment is obsolete: true
Attachment #286002 - Flags: review?(nelson)
Comment on attachment 286002 [details] [diff] [review]
Patch ver 3.

This is getting closer to the right solution, I think.  
Two comments about this patch:

> dbtest_init()
> {
>+  if [  -z "${OBJDIR}" ]
>+  then
>+     echo "OBJDIR is not defined. Run all.sh or export OBJDIR to the platform"
>+     echo "specific directory under mozilla/dist/ and then run this script" 
>+     exit
>+  fi

1) All NSS test scripts should be able to be run by themselves.  
For example, although ssl.sh is run inside of all.sh, it may also be run
by itself, without running any other script first.  If a script needs 
another script to run first, it may run that script first.  For example,
ssl.sh determines if cert.sh has been run first, and if not, it runs 
cert.sh.  That property should be true of this script also.  


>+  # This checks for modutil just because gmake at the level of security/nss,
>+  # creates modutil file at the end. Thus existence of modutil ensures that
>+  # the build was successful. In case some other file is created after modutil,
>+  # that file has to replace modutil in this script.
>+
>+  if test ! -f ../../../../dist/${OBJDIR}/bin/modutil; 
>+  then 						
>+		echo "Build incomplete. Aborting test...";         
>+  		exit;	                                            
>+  fi

The above test command assumes that the dist directory is located at a fixed
relative path to the current source directory.  That assumption will be 
false in some runs.  Look at the coreconf makefiles for the variable BUILD_TREE
for clues about how to do this.  See 
http://mxr.mozilla.org/security/search?string=BUILD_TREE&case=on&tree=security
Attachment #286002 - Flags: review?(nelson) → review-
(Assignee)

Comment 14

11 years ago
Created attachment 286269 [details] [diff] [review]
Patch ver 4

It tries to address the concern made in Comment #13
on Patch ver 4.

(1)It makes change only in dbtests.sh and does not search
for modutil using any fixed relative path. It searches modutil
in $(DIST)/$(OBJDIR)/bin location.

(2) dbtests.sh can be run independently as it can call init.sh
before checking for the existence of modutil.

(3)As DIST and OBJDIR are already exported by init.sh, these
variables are available to dbtests.sh, after init.sh is run
from within dbtests.sh.

(4)If BUILD_TREE value is not empty, DIST is constructed based
on the value of BUILD_TREE. See below for details.


location.mk in coreconf defines 
DIST = $(SOURCE_PREFIX)/$(PLATFORM)

and SOURCE_PREFIX is defined this way
in source.mk in coreconf:

ifndef SOURCE_PREFIX
    ifndef BUILD_TREE
        SOURCE_PREFIX = $(CORE_DEPTH)/../dist
    else
        SOURCE_PREFIX = $(BUILD_TREE)/dist
    endif
endif
Attachment #286002 - Attachment is obsolete: true
Attachment #286269 - Flags: review?(nelson)
(Assignee)

Comment 15

11 years ago
Errata: The statement in comment #14 should have been  
"
It tries to address the concern made in Comment #13
on Patch ver 3.
"
Comment on attachment 286269 [details] [diff] [review]
Patch ver 4

>Index: security/nss/tests/dbtests/dbtests.sh

Biswatosh, this patch patches dbtests.sh. 
Shouldn't it be patching all.sh instead?
What about the other test scripts in nss/tests/* ?
(Assignee)

Comment 17

11 years ago
Nelson,

The bug reported came from dbtests.sh and I wanted 
to focus on that although the same problem could be 
repeated elsewhere as well. In that case, are we
saying  "We won't run any test if the build is
incomplete"? Although any particular test would not
depend upon the whole build to be successful but never
theless we wont even run the script if build is not
complete.

Then, is it not better to put my patch in init.sh ?
Putting in all.sh would make dbtests.sh report success
if run individually even when tstclnt is not found. 
Only, when all.sh is run with
TESTS=dbtests, can make dbtests.sh(and all.sh) fail. 
Putting it in init.sh would affect all test scripts
(including all.sh) and they wont simply run if build 
fails at any stage.

I saw some test scripts in nss.tests/* but did not find
repeatition of this kind of problem reported in the bug.
I mean, case similar to:
********
somecmdtool 
    ret=$?
    if [ $ret -ne 1 ]; then
      echo "success"
    else
      echo "failure".
    fi
*********
But, someone might add a script like this in future.
Comment on attachment 286269 [details] [diff] [review]
Patch ver 4

The report complains that the nightly build+QA test run doesn't detect 
a build failure.  

I think this fix wants to be in all.sh, and nowhere else.
The purpose of all.sh is to test all of NSS's tests.
The whole build should succeed first.
Other tests test individual parts of NSS.  It may not be necessary
for all of the NSS build to finish for those individual parts.
But all.sh needs to check that the build succeeded.
Attachment #286269 - Flags: review?(nelson) → review-
Target Milestone: --- → 3.11.8
(Assignee)

Comment 19

11 years ago
Created attachment 286813 [details] [diff] [review]
Patch ver 5

Moved the additional lines suggested in dbtests.sh in Patch ver4
to all.sh.
Ensuring that the error is output on console as well on 
results.html. 

I am deliberately not using html_failed()
and other related functions because on this error, no single 
testcase will be run and the cells and tables are meaningful
if at least some tests have been run already. But that is not
the case here.
Attachment #286269 - Attachment is obsolete: true
Attachment #286813 - Flags: review?(nelson)
(Assignee)

Comment 20

11 years ago
Comment on attachment 286813 [details] [diff] [review]
Patch ver 5

Want to do a small change in a line and resubmit the patch.
Attachment #286813 - Attachment is obsolete: true
Attachment #286813 - Flags: review?(nelson)
(Assignee)

Comment 21

11 years ago
Created attachment 286850 [details] [diff] [review]
Patch 5

It outputs the error message to the console, to results.html
and to output.log.
The patch which I marked obsolete just now was not outputing
to output.log.
Attachment #286850 - Flags: review?(nelson)
Comment on attachment 286850 [details] [diff] [review]
Patch 5

init.sh can return without defining ${RESULTS}
Attachment #286850 - Flags: review?(nelson) → review-
(Assignee)

Comment 23

11 years ago
Nelson,

Sorry, I could not undertand Comment #22. Please correct me where
I am wrong.

Do you want the patch to just echo the error message to the console 
or echo it to console and put it to LOGFILE as well?
But, if init.sh can return without defining ${RESULTS} , so it can 
return without defining ${LOGFILE} as well?

But how could init.sh return without defining ${RESULTS} or ${LOGFILE}?

The only case when RESULTS is not defined by a running init.sh ,
is when init is already sourced and if init is sourced, RESULTS 
must have been defined by the earlier ran init.sh(which had 
caused init to be sourced).

When all.sh is run and init.sh exits, all.sh will also exit.
We don't consider this case here. We are talking of a return by 
init.sh.
But, if init.sh returns, will not LOGFILE and RESULTS be already
defined and can be used in all.sh ? LOGFILE is exported in init.sh
and RESULTS is not but should that matter ? init.sh, all.sh,
and all the subsequent scripts are run from the same shell and thus
RESULTS should be available to all of these shell scripts, is not it?
   

Comment 24

11 years ago
Don't write directly into ${RESULTS}: use html_msg (or html_failed maybe) instead. We may do some processing in html_* that we want to keep centralized.

You need to exit with an error code so the program calling all.sh knows the tests failed.

Comment 25

11 years ago
Don't use:
  if test <condition>
but use
  if [ <condition> ]

to be consistent with the coding style of the rest of the code.

Don't add ; at the end of each line.

Comment 26

11 years ago
(In reply to comment #24)
> Don't write directly into ${RESULTS}: use html_msg (or html_failed maybe)
> instead. We may do some processing in html_* that we want to keep centralized.
> 
> You need to exit with an error code so the program calling all.sh knows the
> tests failed.
> 

There is no html table created at time the when this is tested, so using only html_msg would just mess up results page.

Maybe construction like this would be OK:

if [ ! -f ${DIST}/${OBJDIR}/bin/modutil ]; then
	html_head "Testing initialization"
	Exit "Checking for build"
fi
(Assignee)

Comment 27

11 years ago
(In reply to comment #26)
Also need to put this to LOGFILE. Thus, can be modified to:

if [ ! -f ${DIST}/${OBJDIR}/bin/modutil ]; then
        echo "Build Incomplete. Aborting test." >> ${LOGFILE}
        html_head "Testing initialization"
        Exit "Checking for build"
fi

Does this seem OK ?

Comment 28

11 years ago
OK with me.
(Assignee)

Comment 29

11 years ago
Created attachment 287672 [details] [diff] [review]
Patch 6

Uses html_head function available in init.sh instead of directly
using RESULTS. LOGFILE however is being used in all.sh already
and thus used in the patch.
Attachment #286850 - Attachment is obsolete: true
Attachment #287672 - Flags: review?(nelson)
Comment on attachment 287672 [details] [diff] [review]
Patch 6

I think this will be OK. 
Let's ask Julien
Attachment #287672 - Flags: superreview?(julien.pierre.boogz)
Attachment #287672 - Flags: review?(nelson)
Attachment #287672 - Flags: review+
(Reporter)

Comment 31

11 years ago
Comment on attachment 287672 [details] [diff] [review]
Patch 6

I believe this will break Windows, since the file is named modutil.exe, not modutil.
Attachment #287672 - Flags: superreview?(julien.pierre.boogz) → superreview-
(Assignee)

Comment 32

11 years ago
(In reply to comment #31)
Julien,

To solve the problem you mentioned, one way is just to add
'*' to the end of modutil. That will take care of modutil
or modutil.exe. I mean this:
******************
if [ ! -f ${DIST}/${OBJDIR}/bin/modutil* ]; then
echo "Build Incomplete. Aborting test." >> ${LOGFILE}
html_head "Testing Initialization"
Exit "Checking for build"
fi
********************
The other way is to use #ifdef. For Windows and OS2, 
it will search for modutil.exe and for others, just modutil.

Which one you suggest ? My opinion is the first alternative as
it is extremely unlikely to have another cmd utility with the name
starting with "modutil".  I am attaching another patch with that
idea.
  
(Assignee)

Comment 33

11 years ago
Created attachment 288835 [details] [diff] [review]
Patch7

See Comment #32
Attachment #287672 - Attachment is obsolete: true
Attachment #288835 - Flags: review?(julien.pierre.boogz)
(Reporter)

Comment 34

11 years ago
Comment on attachment 288835 [details] [diff] [review]
Patch7

Biswatosh, there are other files such as modutil.pdb which end up in the bin directory. You need to check for modutil.exe exactly.
Attachment #288835 - Flags: review?(julien.pierre.boogz) → review-
(Reporter)

Updated

11 years ago
OS: SunOS → All
Hardware: PC → All
(Assignee)

Comment 35

11 years ago
(In reply to comment #34)
Julien,

Does this seem OK?
****************
if [ ! -f ${DIST}/${OBJDIR}/bin/modutil -a  \
     ! -f ${DIST}/${OBJDIR}/bin/modutil.exe ]; then
	echo "Build Incomplete. Aborting test." >> ${LOGFILE}
	html_head "Testing Initialization"
	Exit "Checking for build"
fi
****************

So, if build fails either in Windows or in Linux/Solaris, both
of modutil and modutil.exe will not exist and all.sh will exit.
If build succeeds, one of modutil.exe and modutil will exist and
thus all.sh won't exit.


(Reporter)

Comment 36

11 years ago
Yes, that's fine.
(Assignee)

Comment 37

11 years ago
Created attachment 288963 [details] [diff] [review]
Implements Comment #35
Attachment #288835 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #288963 - Flags: review?(julien.pierre.boogz)
(Reporter)

Updated

11 years ago
Attachment #288963 - Flags: review?(julien.pierre.boogz) → review+
(Assignee)

Updated

11 years ago
Target Milestone: 3.11.8 → 3.11.9
(Assignee)

Comment 38

11 years ago
Created attachment 288968 [details] [diff] [review]
Patch for the Branch

The patch 288963 (in Comment #37) was for the trunk and this patch 
is for the NSS_3_11_BRANCH.

The all.sh is implemented in a little different fashion compared
to the trunk's one and so I am giving it for review again and also
for superreview. Although, the patch is basically same as that for
trunk.
Attachment #288968 - Flags: superreview?(nelson)
Attachment #288968 - Flags: review?(julien.pierre.boogz)
(Assignee)

Comment 39

11 years ago
Checked in to the trunk...

Checking in all.sh;
/cvsroot/mozilla/security/nss/tests/all.sh,v  <--  all.sh
new revision: 1.41; previous revision: 1.40
done
(Reporter)

Updated

11 years ago
Attachment #288968 - Flags: review?(julien.pierre.boogz) → review+
Attachment #288968 - Flags: superreview?(nelson) → superreview+
(Assignee)

Comment 40

11 years ago
Checked in to the Branch...

Checking in all.sh;
/cvsroot/mozilla/security/nss/tests/all.sh,v  <--  all.sh
new revision: 1.19.2.5; previous revision: 1.19.2.4
done
Status: ASSIGNED → NEW
(Assignee)

Comment 41

11 years ago
Closing the bug as patches for Trunk and Branch have been reviewed
and checked into the repository.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.