Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

This patch contains the first modifications for multi repository buildsets. #287

Merged
merged 11 commits into from Jan 8, 2012

Conversation

Projects
None yet
4 participants
@hborkhuis
Copy link
Contributor

hborkhuis commented Dec 7, 2011

This patch does not add any new features to buildbot, but only implement the new sourcestampset structure and is a first delivery that can be tested and reviewed. A sourcestampset will only contain one sourcestamp.

The changes can be grouped in four categories:
database: new table 'sourcestampsets' buildsets and sourcestamps refere to this table
business logic: build can return sourcestamps for given 'repo id' or buildrequest.source
builder can handle sourcestamp sets containing exactly 1 sourcestamp
buildrequest has now properties 'source' (backwards comp) and 'sources'
status/web: rebuild and force can handle sourcestampsets with exacly 1 sourcestamp
test: all test modified to support sourcestampsets (add set when insertdata)

For the next patch: changes will be categorized per repository. For each repository a separate sourcestamp will be created containing the appropriate changes. All sourcestamps will be part of a single sourcestampset. The buildrequest will revere to the sourcestampset.

This patch contains the first modifications for supporting multi repo…
…sitory buildsets.

database: new table 'sourcestampsets' buildsets and sourcestamps refere to this table
business logic: build can return sourcestamps for given 'repo id' or buildrequest.source
                builder can handle sourcestamp sets containing exactly 1 sourcestamp
                buildrequest has now properties 'source' (backwards comp) and 'sources'
status/web: rebuild and force can handle sourcestampsets with exacly 1 sourcestamp
test: all test modified to support sourcestampsets (add set when insertdata)
@benallard

This comment has been minimized.

Copy link
Contributor

benallard commented Dec 7, 2011

To make review easier, I can stress the parts where the stuff actually happen:

  • https://github.com/buildbot/buildbot/pull/287/files#L4R69 (db.sourcestamps, getSourceStamps function is added to gather all sourcestamps from a set)
  • https://github.com/buildbot/buildbot/pull/287/files#L9R257 (scheduler.base, all functions addBuildsetForXXX now create a set before creating the sourcestamp itself. further work is needed in this area:
    • addBuildsetforChangesMultirepo (not used) needs to replace the adddBuildsetForChanges
    • addBuildsetForSourceStamp needs to be renamed in addBuildsetForSourcestampSet
  • multiple places care that a set is created before e sourcestampt be created (status.web.builder, sourcestamp, schedulers.trysched)

All the rest is basically a replacement of sourcestamp into sourcestampset.

The sourcestamps should actually not be used anymore except inside a sourcestampset.

@tomprince

This comment has been minimized.

Copy link
Member

tomprince commented Dec 11, 2011

I was going to try upgrading one of my buildbots, but this doesn't seem to merge cleanly with master. There are conflicts with the new ForceScheduler.

@djmitche

This comment has been minimized.

Copy link
Member

djmitche commented Dec 18, 2011

OK, this was a long time in the making, but this was a big patch and it took me a few days to give it the thorough reading it deserves.

A few general points, and I'll follow up on the diff for more specific points.

  • Any incompatible changes that might affect people who have written custom schedulers should be noticed in master/NEWS. The change from BuildRequest.source to BuildRequest.sources should be noted here, too.
  • The tests don't pass -- I get errors both on the upgrade tests and for the force scheduler
  • DB API changes should be reflected in the developer documentation
  • I would like to get away from expensive process objects like SourceStamp, so -- since buildrequest.source is being changed to buildrequest.sources, let's make that a list of ssdicts, instead.

I agree with your assessment of further work required for master/buildbot/schedulers/base.py. I think that there should still be an addBuildsetForSourceStamp (if only for compatibility -- it should add a sourcestampsets row) and an addBuildsetForSourceStamps, rather than trying to make the same method do both. It would probably make sense to have one method call the other.

As for addBuildsetForChangesMultiRepo, that references self.repositories which isn't referenced elsewhere in the file. Perhaps that would better be handled as a parameter to the method, so that it's clear that subclasses must manage that value?

The changes to addBuildsetForLatest and addBuildsetForChanges look good.

tmp_buildsets = sa.Table('tmp_buildsets', metadata,
sa.Column('id', sa.Integer, primary_key=True),

# a simple external identifier to track down this buildset later, e.g.,

This comment has been minimized.

Copy link
@djmitche

djmitche Dec 18, 2011

Member

Generally upgrade scripts can omit the comments that are contained in model.py.

This comment has been minimized.

Copy link
@hborkhuis

hborkhuis Dec 19, 2011

Author Contributor

You 're right. I will remove all 'model.py' comments.


# Add index for performance reasons to find all sourcestamps in a set quickly
idx = sa.Index('sourcestamps_sourcestampsetid', sourcestamps_table.c.sourcestampsetid, unique=False)
idx.create()

This comment has been minimized.

Copy link
@djmitche

djmitche Dec 18, 2011

Member

This index isn't reflected in model.py

This comment has been minimized.

Copy link
@hborkhuis

hborkhuis Dec 19, 2011

Author Contributor

The index should have been in model.py already. Index will be added.

if repoid is None:
return self.source
assert (repoid in self.sources)
return self.sources[repoid]

This comment has been minimized.

Copy link
@djmitche

djmitche Dec 18, 2011

Member

How is self.sources set? I don't see it referenced elsewhere in this file.

This comment has been minimized.

Copy link
@hborkhuis

hborkhuis Dec 19, 2011

Author Contributor

The base class (Source) for source steps in oldsource.py contains a method GetRepoId that determines the identification of a repository for a specific step. As this is the base class for all different VC's I made the term repository somewhat more abstract as I am not sure if repository is good a term that is always used in every VC.
The atribute self.sources is not set in this patch. In the next patch when buildrequest may deliver 1+ sourcestamps, this attribute will be set to a dictionary of sourcestamps, with the repository (identifier) as key.
I have decided to include the new interface (GetRepoId + getSourceStamp(repoid) as this is a part of the new design, but to keep the new implementation of more than one sourcestamps outside the current patch.

This comment has been minimized.

Copy link
@djmitche

djmitche Dec 25, 2011

Member

We've been fairly consistent about identifying repositories by a single string -- this actually makes sense for all of the VC's Buildbot currently supports. It's usually in URL format, but that's not required. So I think you could just stick with repository here, rather than adding another layer of abstraction.

@djmitche

This comment has been minimized.

Copy link
Member

djmitche commented Dec 18, 2011

What is a repoid?

from buildbot.db import base

class SourceStampSetsConnectorComponent(base.DBConnectorComponent):
# Documentation is in developer/database.rst

This comment has been minimized.

Copy link
@djmitche

djmitche Dec 18, 2011

Member

hint hint ;)

This comment has been minimized.

Copy link
@hborkhuis

hborkhuis Dec 19, 2011

Author Contributor

Got the hint, I 'll change the doc


def addSourcestampSet(self):
def thd(conn):
# insert the buildset itself

This comment has been minimized.

Copy link
@djmitche

djmitche Dec 18, 2011

Member

This comment is incorrect

This comment has been minimized.

Copy link
@hborkhuis

hborkhuis Dec 19, 2011

Author Contributor

Sorry, should be sourcestampset. I 'll change this.

buildrequest.sources.append(wfd.getResult())

# support old interface where only one source could exist
# TODO: remove and change all client objects that use this property

This comment has been minimized.

Copy link
@djmitche

djmitche Dec 18, 2011

Member

I have an aversion to merging code with "TODO" in it, unless the person writing the code is committed TO DO it soon. Do you have a plan for how to fix this wrinkle?

This comment has been minimized.

Copy link
@hborkhuis

hborkhuis Dec 19, 2011

Author Contributor

This will change this in the second patch, otherwise a buildrequest never can have multiple sourcestamps.

@@ -88,7 +88,8 @@ def _checkCompletedBuildsets(self, bsid, result):
# build a dependent build if the status is appropriate
if sub_results in (SUCCESS, WARNINGS):
wfd = defer.waitForDeferred(
self.addBuildsetForSourceStamp(ssid=sub_ssid,
# TODO: ssid is not needed anymore as setid defines the sourcestamp(s)

This comment has been minimized.

Copy link
@djmitche

djmitche Dec 18, 2011

Member

I'm not sure what this comment means?

This comment has been minimized.

Copy link
@hborkhuis

hborkhuis Dec 19, 2011

Author Contributor

Forgot to remove the comment, will be changed in this patch

del buildset['sourcestampid']

#TODO: if buildbot supports more sourcestamps in a set
# then ss becomes dictOfssDict and the break must be removed

This comment has been minimized.

Copy link
@djmitche

djmitche Dec 18, 2011

Member

Shouldn't this happen now?

@hborkhuis

This comment has been minimized.

Copy link
Contributor Author

hborkhuis commented Dec 19, 2011

Dustin,

Thanks for reviewing the pull request, I will process/answer all the
comments. I have some questions about the tests.

You mentioned that some tests fail, so I pulled the latest version of the
master. Test failed at the status_mail and forced scheduler, not the
upgrade. I fixed these tests already.
But, can you give some details about what failed in the upgrade tests.

Harry

-----Original Message-----
From: Dustin J. Mitchell
[mailto:reply+i-2475178-99e1b69084e87efb70d5035c737fbee155280071-1246424@rep
ly.github.com]
Sent: Sunday, December 18, 2011 9:05 PM
To: hborkhuis
Subject: Re: [buildbot] This patch contains the first modifications for
multi repository buildsets. (#287)

OK, this was a long time in the making, but this was a big patch and it took
me a few days to give it the thorough reading it deserves.

A few general points, and I'll follow up on the diff for more specific
points.

  • Any incompatible changes that might affect people who have written custom
    schedulers should be noticed in master/NEWS. The change from
    BuildRequest.source to BuildRequest.sources should be noted here, too.
  • The tests don't pass -- I get errors both on the upgrade tests and for
    the force scheduler
  • DB API changes should be reflected in the developer documentation
  • I would like to get away from expensive process objects like
    SourceStamp, so -- since buildrequest.source is being changed to
    buildrequest.sources, let's make that a list of ssdicts, instead.

I agree with your assessment of further work required for
master/buildbot/schedulers/base.py. I think that there should still be an
addBuildsetForSourceStamp (if only for compatibility -- it should add a
sourcestampsets row) and an addBuildsetForSourceStamps, rather than trying
to make the same method do both. It would probably make sense to have one
method call the other.

As for addBuildsetForChangesMultiRepo, that references self.repositories
which isn't referenced elsewhere in the file. Perhaps that would better be
handled as a parameter to the method, so that it's clear that subclasses
must manage that value?

The changes to addBuildsetForLatest and addBuildsetForChanges look good.


Reply to this email directly or view it on GitHub:
#287 (comment)

@djmitche

This comment has been minimized.

Copy link
Member

djmitche commented Dec 25, 2011

Sure -- here's what I'm seeing:

===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/Users/dustin/code/buildbot/t/buildbot/master/buildbot/test/integration/test_upgrade.py", line 188, in check
    self.fail(str(diff))
twisted.trial.unittest.FailTest: got unexpected index sourcestamps_sourcestampsetid on table sourcestamps: {'unique': 0, 'name': u'sourcestamps_sourcestampsetid', 'column_names': [u'sourcestampsetid']}
missing index buildsets_complete on table buildsets
missing index buildsets_submitted_at on table buildsets

buildbot.test.integration.test_upgrade.UpgradeTestV082.test_upgrade
buildbot.test.integration.test_upgrade.UpgradeTestV083.test_upgrade
buildbot.test.integration.test_upgrade.UpgradeTestV084.test_upgrade
===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/twisted/python/threadpool.py", line 207, in _worker
    result = context.call(ctx, function, *args, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/twisted/python/context.py", line 118, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/twisted/python/context.py", line 81, in callWithContext
    return func(*args,**kw)
  File "/Users/dustin/code/buildbot/t/buildbot/master/buildbot/db/pool.py", line 144, in thd
    rv = callable(self.engine, *args, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/master/buildbot/db/model.py", line 508, in thd
    upgrade(engine)
  File "/Users/dustin/code/buildbot/t/buildbot/master/buildbot/db/model.py", line 469, in upgrade
    schema.runchange(version, change, 1)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/versioning/schema.py", line 91, in runchange
    change.run(self.engine, step)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/versioning/script/py.py", line 145, in run
    script_func(engine)
  File "/Users/dustin/code/buildbot/t/buildbot/master/buildbot/db/migrate/versions/018_add_sourcestampset.py", line 126, in upgrade
    ss_sourcestampsetid.alter(nullable=False)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/schema.py", line 493, in alter
    return alter_column(self, *p, **k)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/schema.py", line 136, in alter_column
    engine._run_visitor(visitorcallable, delta)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 2222, in _run_visitor
    conn._run_visitor(visitorcallable, element, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1898, in _run_visitor
    **kwargs).traverse_single(element)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/ansisql.py", line 57, in traverse_single
    ret = super(AlterTableVisitor, self).traverse_single(elem)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/visitors.py", line 86, in traverse_single
    return meth(obj, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/databases/sqlite.py", line 55, in visit_column
    self.recreate_table(table,column,delta)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/databases/sqlite.py", line 42, in recreate_table
    table.create()
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/schema.py", line 523, in create
    checkfirst=checkfirst)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 2222, in _run_visitor
    conn._run_visitor(visitorcallable, element, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1898, in _run_visitor
    **kwargs).traverse_single(element)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/visitors.py", line 86, in traverse_single
    return meth(obj, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/ddl.py", line 82, in visit_table
    self.connection.execute(schema.CreateTable(table))
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1399, in execute
    params)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1484, in _execute_ddl
    compiled = ddl.compile(dialect=dialect)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/expression.py", line 1647, in compile
    return self._compiler(dialect, bind=bind, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/schema.py", line 2766, in _compiler
    return dialect.ddl_compiler(dialect, self, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 699, in __init__
    self.string = self.process(self.statement)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 718, in process
    return obj._compiler_dispatch(self, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/visitors.py", line 59, in _compiler_dispatch
    return getter(visitor)(self, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1278, in visit_create_table
    const = self.create_table_constraints(table)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1298, in create_table_constraints
    for constraint in constraints
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1296, in 
    return ", \n\t".join(p for p in
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1304, in 
    not getattr(constraint, 'use_alter', False)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 718, in process
    return obj._compiler_dispatch(self, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/visitors.py", line 59, in _compiler_dispatch
    return getter(visitor)(self, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1278, in visit_create_table
    const = self.create_table_constraints(table)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1298, in create_table_constraints
    for constraint in constraints
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1296, in 
    return ", \n\t".join(p for p in
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1304, in 
    not getattr(constraint, 'use_alter', False)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 718, in process
    return obj._compiler_dispatch(self, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/visitors.py", line 59, in _compiler_dispatch
    return getter(visitor)(self, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/dialects/sqlite/base.py", line 380, in visit_foreign_key_constraint
    return super(SQLiteDDLCompiler, self).visit_foreign_key_constraint(constraint)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1428, in visit_foreign_key_constraint
    preparer.format_constraint(constraint)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1721, in format_constraint
    return self.quote(constraint.name, constraint.quote)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1693, in quote
    if self._requires_quotes(ident):
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1674, in _requires_quotes
    lc_value = value.lower()
exceptions.AttributeError: 'int' object has no attribute 'lower'

buildbot.test.integration.test_upgrade.TestWeirdChanges.testUpgradeChangeLinks
buildbot.test.integration.test_upgrade.TestWeirdChanges.testUpgradeChangeNoRevision
buildbot.test.integration.test_upgrade.TestWeirdChanges.testUpgradeChangeProperties
buildbot.test.integration.test_upgrade.TestWeirdChanges.testUpgradeListsAsFilenames
buildbot.test.integration.test_upgrade.UpgradeTest075.test_upgrade
buildbot.test.integration.test_upgrade.UpgradeTestCitools.test_upgrade
buildbot.test.integration.test_upgrade.UpgradeTestEmptyReal.test_emptydb_modelmatches
buildbot.test.regressions.test_import_unicode_changes.TestUnicodeChanges.testAsciiChange
buildbot.test.regressions.test_import_unicode_changes.TestUnicodeChanges.testUTF16Change
buildbot.test.regressions.test_import_unicode_changes.TestUnicodeChanges.testUnicodeChange
===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/twisted/python/threadpool.py", line 207, in _worker
    result = context.call(ctx, function, *args, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/twisted/python/context.py", line 118, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/twisted/python/context.py", line 81, in callWithContext
    return func(*args,**kw)
    return func(*args,**kw)
  File "/Users/dustin/code/buildbot/t/buildbot/master/buildbot/db/pool.py", line 144, in thd
    rv = callable(self.engine, *args, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/master/buildbot/test/util/migration.py", line 76, in upgrade_thd
    schema.runchange(version, change, 1)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/versioning/schema.py", line 91, in runchange
    change.run(self.engine, step)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/versioning/script/py.py", line 145, in run
    script_func(engine)
  File "/Users/dustin/code/buildbot/t/buildbot/master/buildbot/db/migrate/versions/018_add_sourcestampset.py", line 126, in upgrade
    ss_sourcestampsetid.alter(nullable=False)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/schema.py", line 493, in alter
    return alter_column(self, *p, **k)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/schema.py", line 136, in alter_column
    engine._run_visitor(visitorcallable, delta)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 2222, in _run_visitor
    conn._run_visitor(visitorcallable, element, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1898, in _run_visitor
    **kwargs).traverse_single(element)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/ansisql.py", line 57, in traverse_single
    ret = super(AlterTableVisitor, self).traverse_single(elem)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/visitors.py", line 86, in traverse_single
    return meth(obj, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/databases/sqlite.py", line 55, in visit_column
    self.recreate_table(table,column,delta)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/databases/sqlite.py", line 42, in recreate_table
    table.create()
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/schema.py", line 523, in create
    checkfirst=checkfirst)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 2222, in _run_visitor
    conn._run_visitor(visitorcallable, element, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1898, in _run_visitor
    **kwargs).traverse_single(element)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/visitors.py", line 86, in traverse_single
    return meth(obj, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/databases/sqlite.py", line 55, in visit_column
    self.recreate_table(table,column,delta)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/databases/sqlite.py", line 42, in recreate_table
    table.create()
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/schema.py", line 523, in create
    checkfirst=checkfirst)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 2222, in _run_visitor
    conn._run_visitor(visitorcallable, element, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1898, in _run_visitor
    **kwargs).traverse_single(element)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/visitors.py", line 86, in traverse_single
    return meth(obj, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/ddl.py", line 82, in visit_table
    self.connection.execute(schema.CreateTable(table))
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1399, in execute
    params)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1484, in _execute_ddl
    compiled = ddl.compile(dialect=dialect)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/expression.py", line 1647, in compile
    return self._compiler(dialect, bind=bind, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/schema.py", line 2766, in _compiler
    return dialect.ddl_compiler(dialect, self, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 699, in __init__
    self.string = self.process(self.statement)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 718, in process
    return obj._compiler_dispatch(self, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/visitors.py", line 59, in _compiler_dispatch
    return getter(visitor)(self, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1278, in visit_create_table
    const = self.create_table_constraints(table)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1298, in create_table_constraints
    for constraint in constraints
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1296, in 
    return ", \n\t".join(p for p in
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1304, in 
    not getattr(constraint, 'use_alter', False)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 718, in process
    return obj._compiler_dispatch(self, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/visitors.py", line 59, in _compiler_dispatch
    return getter(visitor)(self, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/dialects/sqlite/base.py", line 380, in visit_foreign_key_constraint
    return super(SQLiteDDLCompiler, self).visit_foreign_key_constraint(constraint)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1428, in visit_foreign_key_constraint
    preparer.format_constraint(constraint)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1721, in format_constraint
    return self.quote(constraint.name, constraint.quote)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1693, in quote
    if self._requires_quotes(ident):
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1674, in _requires_quotes
    lc_value = value.lower()
exceptions.AttributeError: 'int' object has no attribute 'lower'

buildbot.test.unit.test_db_migrate_versions_18_add_sourcestampset.Migration.test_1_buildsets
buildbot.test.unit.test_db_migrate_versions_18_add_sourcestampset.Migration.test_2_sourcestamp
buildbot.test.unit.test_db_migrate_versions_18_add_sourcestampset.Migration.test_3_sourcestampset
buildbot.test.unit.test_db_migrate_versions_18_add_sourcestampset.Migration.test_4_integrated_migration
===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/twisted/python/threadpool.py", line 207, in _worker
    result = context.call(ctx, function, *args, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/twisted/python/context.py", line 118, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/twisted/python/context.py", line 81, in callWithContext
    return func(*args,**kw)
  File "/Users/dustin/code/buildbot/t/buildbot/master/buildbot/db/pool.py", line 132, in thd
    rv = callable(conn, *args, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/master/buildbot/test/util/db.py", line 66, in __thd_clean_database
    meta.reflect()
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/schema.py", line 2386, in reflect
    Table(name, self, **reflect_opts)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/schema.py", line 265, in __new__
    table._init(name, metadata, *args, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/schema.py", line 335, in _init
    self, include_columns
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1894, in run_callable
    return callable_(self, *args, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/default.py", line 254, in reflecttable
    return insp.reflecttable(table, include_columns)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/reflection.py", line 450, in reflecttable
    **reflection_options
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/schema.py", line 265, in __new__
    table._init(name, metadata, *args, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/schema.py", line 324, in _init
    self, include_columns
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1894, in run_callable
    return callable_(self, *args, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/default.py", line 254, in reflecttable
    return insp.reflecttable(table, include_columns)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/reflection.py", line 417, in reflecttable
    raise exc.NoSuchTableError(table.name)
sqlalchemy.exc.NoSuchTableError: sourcestamps

buildbot.test.unit.test_db_migrate_versions_18_add_sourcestampset.Migration.test_1_buildsets
buildbot.test.unit.test_db_migrate_versions_18_add_sourcestampset.Migration.test_2_sourcestamp
buildbot.test.unit.test_db_migrate_versions_18_add_sourcestampset.Migration.test_3_sourcestampset
buildbot.test.unit.test_db_migrate_versions_18_add_sourcestampset.Migration.test_4_integrated_migration
===============================================================================
[ERROR]
Traceback (most recent call last):
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/twisted/internet/defer.py", line 133, in maybeDeferred
    result = f(*args, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/master/buildbot/db/model.py", line 508, in thd
    upgrade(engine)
  File "/Users/dustin/code/buildbot/t/buildbot/master/buildbot/db/model.py", line 469, in upgrade
    schema.runchange(version, change, 1)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/versioning/schema.py", line 91, in runchange
    change.run(self.engine, step)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/versioning/script/py.py", line 145, in run
    script_func(engine)
  File "/Users/dustin/code/buildbot/t/buildbot/master/buildbot/db/migrate/versions/018_add_sourcestampset.py", line 126, in upgrade
    ss_sourcestampsetid.alter(nullable=False)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/schema.py", line 493, in alter
    return alter_column(self, *p, **k)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/schema.py", line 136, in alter_column
    engine._run_visitor(visitorcallable, delta)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 2222, in _run_visitor
    conn._run_visitor(visitorcallable, element, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1898, in _run_visitor
    **kwargs).traverse_single(element)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/ansisql.py", line 57, in traverse_single
    ret = super(AlterTableVisitor, self).traverse_single(elem)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/visitors.py", line 86, in traverse_single
    return meth(obj, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/databases/sqlite.py", line 55, in visit_column
    self.recreate_table(table,column,delta)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/migrate/changeset/databases/sqlite.py", line 42, in recreate_table
    table.create()
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/schema.py", line 523, in create
    checkfirst=checkfirst)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 2222, in _run_visitor
    conn._run_visitor(visitorcallable, element, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1898, in _run_visitor
    **kwargs).traverse_single(element)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/visitors.py", line 86, in traverse_single
    return meth(obj, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/ddl.py", line 82, in visit_table
    self.connection.execute(schema.CreateTable(table))
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1399, in execute
    params)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1484, in _execute_ddl
    compiled = ddl.compile(dialect=dialect)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/expression.py", line 1647, in compile
    return self._compiler(dialect, bind=bind, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/schema.py", line 2766, in _compiler
    return dialect.ddl_compiler(dialect, self, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 699, in __init__
    self.string = self.process(self.statement)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 718, in process
    return obj._compiler_dispatch(self, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/visitors.py", line 59, in _compiler_dispatch
    return getter(visitor)(self, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1278, in visit_create_table
    const = self.create_table_constraints(table)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1298, in create_table_constraints
    for constraint in constraints
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1296, in 
    return ", \n\t".join(p for p in
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1304, in 
    not getattr(constraint, 'use_alter', False)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 718, in process
    return obj._compiler_dispatch(self, **kwargs)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/visitors.py", line 59, in _compiler_dispatch
    return getter(visitor)(self, **kw)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/dialects/sqlite/base.py", line 380, in visit_foreign_key_constraint
    return super(SQLiteDDLCompiler, self).visit_foreign_key_constraint(constraint)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1428, in visit_foreign_key_constraint
    preparer.format_constraint(constraint)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1721, in format_constraint
    return self.quote(constraint.name, constraint.quote)
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1693, in quote
    if self._requires_quotes(ident):
  File "/Users/dustin/code/buildbot/t/buildbot/sand27osx/lib/python2.7/site-packages/sqlalchemy/sql/compiler.py", line 1674, in _requires_quotes
    lc_value = value.lower()
exceptions.AttributeError: 'int' object has no attribute 'lower'

buildbot.test.unit.test_db_model.DBConnector_Basic.test_is_current_full

Here's what I'm running:

(sand27osx)dustin@lorentz ~/code/buildbot/t/buildbot [master*] $ pip freeze
Jinja2==2.6
Pygments==1.4
SQLAlchemy==0.7.2
Sphinx==1.0.7
Tempita==0.5.1
Twisted==11.0.0
buildbot==0.8.5-100-ge1e6cc8
buildbot-slave==0.8.5-100-ge1e6cc8
coverage==3.5
decorator==3.3.1
distribute==0.6.14
docutils==0.8
mock==0.7.2
pyglet==1.0
sqlalchemy-migrate==0.7.1
wsgiref==0.1.2
zope.interface==3.7.0
(sand27osx)dustin@lorentz ~/code/buildbot/t/buildbot [master*] $ python --version
Python 2.7.1

I see about the same thing with Python-2.6.7 on my Linux system.

@hborkhuis

This comment has been minimized.

Copy link
Contributor Author

hborkhuis commented Jan 3, 2012

The first Fail is due to the missing index in the db.model, this should be
solved in pull request 287.
Another two indexes on buildsets where not recreated in the
018_add_sourcestampset.py so I added these again.

The 'int object has no attribute lower' is probably a bug in SQL Alchemy. I
found that the module sqlalchemy/sql/compiler.py at line 1721 passes a name
object that should be string. In Sqlite this is not a name but a number
(integer). DB's like Oracle have names for their constraints; it looks like
Sqlite does not. Using a str constructor to pass the integer as a string
solved the problem, but that does not work for this pull request.
sqlalchemy/sql/compiler.py:1721

  •   return self.quote(constraint.name, constraint.quote)
    
  •   return self.quote(str(constraint.name), constraint.quote)
    
    Without this change I am not able to alter the nullable constraint of a
    column or to add a constraint to a column.
    This problem is reported to SQL Alchemy as ticket #2364

Any idea how to omit this issue in my upgrade script?

@djmitche

This comment has been minimized.

Copy link
Member

djmitche commented Jan 7, 2012

Yeah, SQLAlchemy-migrate is very buggy 😢 - I've run into problems like this before.

I suspect that the solution may be to special-case sqlalchemy, and hard-code the SQL there. Unfortunately, SQLite's column-alteration code is under-developed, so it's not possible to add a non-null column to an existing table, leaving us with selecting into a temporary table. Which is ugly.

The best bet may be to monkey-patch sqlalchemy-migrate to handle this case correctly.

I can't merge the patch until this is fixed, but I can work on fixing it while you move on to the next step in the project -- sound good? I'll file an sqlalchemy-migrate bug, too.

@djmitche

This comment has been minimized.

Copy link
Member

djmitche commented Jan 7, 2012

You can see why this took me a while to get back to -- it definitely required a few hours' study!

This is a problem in SQLAlchemy. I'll update their bug 2364 accordingly, including with a reproduction recipe.

Within Buildbot, we can monkey-patch the fix -- which is to not produce constraint objects with non-string names during metadata autoloading. I'll push your patch with my monkey-patch added to the main repo shortly.

@djmitche

This comment has been minimized.

Copy link
Member

djmitche commented Jan 7, 2012

Ugh, and there are yet more errors on some versions of sqlalchemy. I'll keep hacking.

@djmitche djmitche merged commit b27cc74 into buildbot:master Jan 8, 2012

@djmitche

This comment has been minimized.

Copy link
Member

djmitche commented Jan 8, 2012

Well, that worked with some version combinations, but not all..

@hborkhuis

This comment has been minimized.

Copy link
Contributor Author

hborkhuis commented Jan 10, 2012

Dustin,

Thanks for all the work!

You have been quit busy with my request. Thanks for monkeypatching SQL
alchemy stuff. You fixed some issues that I didn't find in SQLite. You
probably perform tests against all other databases, do you?

Harry Borkhuis

-----Original Message-----
From: Dustin J. Mitchell
[mailto:reply+i-2475178-99e1b69084e87efb70d5035c737fbee155280071-1246424@rep
ly.github.com]
Sent: Sunday, January 08, 2012 1:31 AM
To: hborkhuis
Subject: Re: [buildbot] This patch contains the first modifications for
multi repository buildsets. (#287)

Well, that worked with some version combinations, but not all..


Reply to this email directly or view it on GitHub:
#287 (comment)

@djmitche

This comment has been minimized.

Copy link
Member

djmitche commented Jan 13, 2012

On Tue, Jan 10, 2012 at 2:11 AM, hborkhuis
reply@reply.github.com
wrote:

You have been quit busy with my request. Thanks for monkeypatching SQL
alchemy stuff. You fixed some issues that I didn't find in SQLite. You
probably perform tests against all other databases, do you?

I do, locally, and the metabuildbot tests against them as well -- and
with a few versions of sqlalchemy and sqlalchemy-migrate. Which led
to a lot of bugs!

Anyway, I'm glad to help. These migration scripts are difficult to get right.

Dustin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.