Add Validation for BuildFactory Configuration

Add Validation for BuildFactory Configuration

Raises ValueError if the configuration for a BuildFactory differs from the
expectation.

(SkipBuildbotRuns)

Committed: https://code.google.com/p/skia/source/detail?r=8874

[borenet, 2013-04-25 15:50:28]

This arises from concerns about changing configurations in
https://codereview.chromium.org/14475006/.  We want to make sure that we know
when the builder configuration has changed, and that the change is intentional.

My plan is to update https://codereview.chromium.org/14475006/ to include a
script which maps old builder name to new builder name and verifies that only
the builder name has changed.

Ideally, we want to run a test which validates all builders every time we commit
code into skia/buildbot, so that we aren't surprised when the master fails to
restart.  I'm not really sure how to do that right now.

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/fa...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/fa...
master/skia_master_scripts/factory.py:118: self._env_vars = {}
Adding this to self so that it gets recorded.

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/fa...
master/skia_master_scripts/factory.py:203: # Should we error out here?  We
really want to have expectations...
I'm in favor of throwing an error.  That way, we are forced to add/modify
expectation files every time we add/modify a builder.

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/fa...
master/skia_master_scripts/factory.py:526: self.Validate()
Each builder only uses one of these Build functions, so this is safe.  Calling
Validate() here rather than at the end of the constructor causes all of the
build steps to be included in the dump.

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/ut...
File master/skia_master_scripts/utils.py (right):

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/ut...
master/skia_master_scripts/utils.py:48: 
Helpers for dumping out the dictionary.  We need this because the default
tostring functions don't handle objects well - they print out the memory
location which is incompatible with our use case.  This is also more nicely
formatted.

[rmistry, 2013-04-25 17:31:50]

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/fa...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/fa...
master/skia_master_scripts/factory.py:194: if not os.path.exists(expected_dir):
I think we can assume that the expected dir always exists.

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/fa...
master/skia_master_scripts/factory.py:203: # Should we error out here?  We
really want to have expectations...
On 2013/04/25 15:50:28, borenet wrote:
> I'm in favor of throwing an error.  That way, we are forced to add/modify
> expectation files every time we add/modify a builder.

Yes lets throw an error, else we will never be able to see the below warning
with all the logs that are outputted.

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/fa...
master/skia_master_scripts/factory.py:206: as_string =
utils.ToString(self.__dict__)
as_string -> vars_as_string  or attr_as_string ?

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/fa...
master/skia_master_scripts/factory.py:209: if expectation and as_string !=
expectation:
if we throw an error above then expecation should always exist and we can do:
if as_string != expectation:

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/ut...
File master/skia_master_scripts/utils.py (right):

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/ut...
master/skia_master_scripts/utils.py:49: def IndentStr(indent):
private function?
The only one that looks useful here for external callers is ToString

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/ut...
master/skia_master_scripts/utils.py:56: def ToString(o, indent=0):
If this is going to be the only public function lets add detailed pydoc about
what it is doing.

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/ut...
master/skia_master_scripts/utils.py:71: def ListToString(l, indent):
private function?

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/ut...
master/skia_master_scripts/utils.py:85: def DictToString(d, indent):
private function?

[epoger, 2013-04-25 18:12:29]

LGTM if it LGTRavi

some minor, optional suggestions below...

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
master/skia_master_scripts/factory.py:208: 'expectation!' % self._builder_name)
would there be value to dumping self_as_string and/or expectation in this
ValueError?

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
File master/skia_master_scripts/utils.py (right):

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
master/skia_master_scripts/utils.py:42: string = ''
I think you can implement this as:

return '    ' * (indent + 1)

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
master/skia_master_scripts/utils.py:51: z
z ???

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
master/skia_master_scripts/utils.py:52: o: the object to print.
suggest o -> obj

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
master/skia_master_scripts/utils.py:68: def _ListToString(l, indent):
suggest "list" or a variable name like that; "l" looks too much like "1"

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
master/skia_master_scripts/utils.py:491: configuration='Debug',
I think there's value in using constants to hold these strings, to avoid a
mistaken "Deubg" somewhere along the line

https://codereview.chromium.org/14081044/diff/16001/tools/tests/factory_confi...
File
tools/tests/factory_configuration/expected/Skia_MacMiniLion_Float_Compile_Release_32
(right):

https://codereview.chromium.org/14081044/diff/16001/tools/tests/factory_confi...
tools/tests/factory_configuration/expected/Skia_MacMiniLion_Float_Compile_Release_32:149:
'None',
I think you missed a spot.

Oh, wait, that's fine.

[borenet, 2013-04-25 18:28:09]

Patch set 5 addresses comments and adds a standalone test.  We should add a bot
which runs buildbot self-tests.

Adding the test required fixing a circular import in which utils imported
skia_factory.

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/fa...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/fa...
master/skia_master_scripts/factory.py:194: if not os.path.exists(expected_dir):
On 2013/04/25 17:31:50, rmistry wrote:
> I think we can assume that the expected dir always exists.

Done.

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/fa...
master/skia_master_scripts/factory.py:206: as_string =
utils.ToString(self.__dict__)
On 2013/04/25 17:31:50, rmistry wrote:
> as_string -> vars_as_string  or attr_as_string ?

Changed to self_as_string, since it really is a representation of the entire
object as a string.

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/ut...
File master/skia_master_scripts/utils.py (right):

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/ut...
master/skia_master_scripts/utils.py:49: def IndentStr(indent):
On 2013/04/25 17:31:50, rmistry wrote:
> private function?
> The only one that looks useful here for external callers is ToString

Done.

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/ut...
master/skia_master_scripts/utils.py:56: def ToString(o, indent=0):
On 2013/04/25 17:31:50, rmistry wrote:
> If this is going to be the only public function lets add detailed pydoc about
> what it is doing.

Done.

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/ut...
master/skia_master_scripts/utils.py:71: def ListToString(l, indent):
On 2013/04/25 17:31:50, rmistry wrote:
> private function?

Done.

https://codereview.chromium.org/14081044/diff/1/master/skia_master_scripts/ut...
master/skia_master_scripts/utils.py:85: def DictToString(d, indent):
On 2013/04/25 17:31:50, rmistry wrote:
> private function?

Done.

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
master/skia_master_scripts/factory.py:208: 'expectation!' % self._builder_name)
On 2013/04/25 18:12:29, epoger wrote:
> would there be value to dumping self_as_string and/or expectation in this
> ValueError?

It would be a gigantic thing to print.  I don't think it would be very
informative.  Maybe if we could do an on-the-fly diff?

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
File master/skia_master_scripts/utils.py (right):

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
master/skia_master_scripts/utils.py:42: string = ''
On 2013/04/25 18:12:29, epoger wrote:
> I think you can implement this as:
> 
> return '    ' * (indent + 1)

Done.

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
master/skia_master_scripts/utils.py:51: z
On 2013/04/25 18:12:29, epoger wrote:
> z ???

Done.

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
master/skia_master_scripts/utils.py:52: o: the object to print.
On 2013/04/25 18:12:29, epoger wrote:
> suggest o -> obj

Done.

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
master/skia_master_scripts/utils.py:68: def _ListToString(l, indent):
On 2013/04/25 18:12:29, epoger wrote:
> suggest "list" or a variable name like that; "l" looks too much like "1"

Changed to list_var to avoid collision with built-in "list".

https://codereview.chromium.org/14081044/diff/16001/master/skia_master_script...
master/skia_master_scripts/utils.py:491: configuration='Debug',
On 2013/04/25 18:12:29, epoger wrote:
> I think there's value in using constants to hold these strings, to avoid a
> mistaken "Deubg" somewhere along the line

Agreed.  Unfortunately, I can't use the constants directly from SkiaFactory,
since that is a circular import.  However, SkiaFactory validates the
configuration, so we're relatively safe.  Keep in mind, this code will all
disappear soon.

[rmistry, 2013-04-25 20:53:17]

LGTM

https://codereview.chromium.org/14081044/diff/20077/master/skia_master_script...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/14081044/diff/20077/master/skia_master_script...
master/skia_master_scripts/factory.py:197: expectation = None
I am suprised you need need a None here since the except is raising an Exception

https://codereview.chromium.org/14081044/diff/20077/master/skia_master_script...
File master/skia_master_scripts/utils.py (right):

https://codereview.chromium.org/14081044/diff/20077/master/skia_master_script...
master/skia_master_scripts/utils.py:49: obj: the object to print.
Missing indent parameter, I wonder why the linter didnt complain, maybe because
its an optional parameter?

[borenet, 2013-04-25 21:12:39]

Patch set 6.  Will commit tomorrow morning and restart the master unless I hear
any objections.

https://codereview.chromium.org/14081044/diff/20077/master/skia_master_script...
File master/skia_master_scripts/factory.py (right):

https://codereview.chromium.org/14081044/diff/20077/master/skia_master_script...
master/skia_master_scripts/factory.py:197: expectation = None
On 2013/04/25 20:53:17, rmistry wrote:
> I am suprised you need need a None here since the except is raising an
Exception

That's left over from before the Exception. Removed.

https://codereview.chromium.org/14081044/diff/20077/master/skia_master_script...
File master/skia_master_scripts/utils.py (right):

https://codereview.chromium.org/14081044/diff/20077/master/skia_master_script...
master/skia_master_scripts/utils.py:49: obj: the object to print.
On 2013/04/25 20:53:17, rmistry wrote:
> Missing indent parameter, I wonder why the linter didnt complain, maybe
because
> its an optional parameter?

Done.

[borenet, 2013-04-26 11:45:29]

Committed patchset #6 manually as r8874 (presubmit successful).