launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05262
[Merge] lp:~julian-edwards/launchpad/arch-all-domination-bug-34086 into lp:launchpad
Julian Edwards has proposed merging lp:~julian-edwards/launchpad/arch-all-domination-bug-34086 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #34086 in Launchpad itself: "removal of arch-all packages while there are arch-specific packages dependent on it results in uninstallable binaries"
https://bugs.launchpad.net/launchpad/+bug/34086
For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/arch-all-domination-bug-34086/+merge/79675
Fix problems discovered in the previous branch that was backed out where arch-indep and arch-specific binaries flip.
--
https://code.launchpad.net/~julian-edwards/launchpad/arch-all-domination-bug-34086/+merge/79675
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/arch-all-domination-bug-34086 into lp:launchpad.
=== modified file 'lib/lp/archivepublisher/domination.py'
--- lib/lp/archivepublisher/domination.py 2011-10-18 02:09:38 +0000
+++ lib/lp/archivepublisher/domination.py 2011-10-18 11:39:52 +0000
@@ -153,6 +153,7 @@
class.
"""
def __init__(self, is_source=True):
+ self.is_source = is_source
if is_source:
self.traits = SourcePublicationTraits
else:
@@ -204,6 +205,27 @@
self.logger = logger
self.archive = archive
+ def _checkArchIndep(self, publication):
+ """Return True if the binary publication can be superseded.
+
+ If the publication is an arch-indep binary, we can only supersede
+ it if all the binaries from the same source are also superseded,
+ else those binaries may become uninstallable.
+ See bug 34086.
+ """
+ binary = publication.binarypackagerelease
+ if not binary.architecturespecific:
+ # getOtherPublicationsForSameSource returns PENDING in
+ # addition to PUBLISHED binaries, and we rely on this since
+ # they must also block domination.
+ others = publication.getOtherPublicationsForSameSource()
+ if others.any():
+ # Don't dominate this arch:all binary as there are
+ # other arch-specific binaries from the same build
+ # that are still active.
+ return False
+ return True
+
def dominatePackage(self, publications, live_versions, generalization):
"""Dominate publications for a single package.
@@ -260,7 +282,9 @@
pub.supersede(current_dominant, logger=self.logger)
self.logger.debug2(
"Superseding older publication for version %s.", version)
- elif version in live_versions:
+ elif (version in live_versions or
+ (not generalization.is_source and
+ not self._checkArchIndep(pub))):
# This publication stays active; if any publications
# that follow right after this are to be superseded,
# this is the release that they are superseded by.
@@ -298,6 +322,7 @@
# one, this dominatePackage call will never result in a
# deletion.
latest_version = generalization.getPackageVersion(publications[0])
+ self.logger.debug2("Dominating %s" % name)
self.dominatePackage(
publications, [latest_version], generalization)
@@ -359,27 +384,6 @@
self.logger.debug("Beginning superseded processing...")
- # XXX: dsilvers 2005-09-22 bug=55030:
- # Need to make binaries go in groups but for now this'll do.
- # An example of the concrete problem here is:
- # - Upload foo-1.0, which builds foo and foo-common (arch all).
- # - Upload foo-1.1, ditto.
- # - foo-common-1.1 is built (along with the i386 binary for foo)
- # - foo-common-1.0 is superseded
- # Foo is now uninstallable on any architectures which don't yet
- # have a build of foo-1.1, as the foo-common for foo-1.0 is gone.
-
- # Essentially we ideally don't want to lose superseded binaries
- # unless the entire group is ready to be made pending removal.
- # In this instance a group is defined as all the binaries from a
- # given build. This assumes we've copied the arch_all binaries
- # from whichever build provided them into each arch-specific build
- # which we publish. If instead we simply publish the arch-all
- # binaries from another build then instead we should scan up from
- # the binary to its source, and then back from the source to each
- # binary published in *this* distroarchseries for that source.
- # if the binaries as a group (in that definition) are all superseded
- # then we can consider them eligible for removal.
for pub_record in binary_records:
binpkg_release = pub_record.binarypackagerelease
self.logger.debug(
@@ -450,19 +454,19 @@
generalization = GeneralizedPublication(is_source=False)
for distroarchseries in distroseries.architectures:
- self.logger.debug(
+ self.logger.info(
"Performing domination across %s/%s (%s)",
distroseries.name, pocket.title,
distroarchseries.architecturetag)
- bpph_location_clauses = And(
+ bpph_location_clauses = [
BinaryPackagePublishingHistory.status ==
PackagePublishingStatus.PUBLISHED,
BinaryPackagePublishingHistory.distroarchseries ==
distroarchseries,
BinaryPackagePublishingHistory.archive == self.archive,
BinaryPackagePublishingHistory.pocket == pocket,
- )
+ ]
candidate_binary_names = Select(
BinaryPackageName.id,
And(
@@ -474,7 +478,7 @@
),
group_by=BinaryPackageName.id,
having=Count(BinaryPackagePublishingHistory.id) > 1)
- binaries = IStore(BinaryPackagePublishingHistory).find(
+ main_clauses = [
BinaryPackagePublishingHistory,
BinaryPackageRelease.id ==
BinaryPackagePublishingHistory.binarypackagereleaseID,
@@ -482,10 +486,28 @@
candidate_binary_names),
BinaryPackageRelease.binpackageformat !=
BinaryPackageFormat.DDEB,
- bpph_location_clauses)
- self.logger.debug("Dominating binaries...")
- self._dominatePublications(
- self._sortPackages(binaries, generalization), generalization)
+ ]
+ main_clauses.extend(bpph_location_clauses)
+
+ clauses = []
+ clauses.extend(main_clauses)
+ self.logger.info("Finding binaries...")
+ bins = IStore(BinaryPackagePublishingHistory).find(*clauses)
+ self.logger.info("Dominating binaries...")
+ sorted_packages = self._sortPackages(bins, generalization)
+ self._dominatePublications(sorted_packages, generalization)
+
+ # We need to make a second pass to cover the cases where:
+ # * arch-specific binaries were not all dominated before arch-all
+ # ones that depend on them
+ # * An arch-all turned into an arch-specific, or vice-versa
+ # * A package is completely schizophrenic and changes all of
+ # its binaries between arch-all and arch-any (apparently
+ # occurs sometimes!)
+ self.logger.info("Dominating binaries (2nd pass)...")
+ bins = IStore(BinaryPackagePublishingHistory).find(*clauses)
+ sorted_packages = self._sortPackages(bins, generalization)
+ self._dominatePublications(sorted_packages, generalization)
def _composeActiveSourcePubsCondition(self, distroseries, pocket):
"""Compose ORM condition for restricting relevant source pubs."""
=== modified file 'lib/lp/archivepublisher/tests/test_dominator.py'
--- lib/lp/archivepublisher/tests/test_dominator.py 2011-10-18 02:20:28 +0000
+++ lib/lp/archivepublisher/tests/test_dominator.py 2011-10-18 11:39:52 +0000
@@ -23,7 +23,9 @@
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.registry.interfaces.series import SeriesStatus
from lp.services.log.logger import DevNullLogger
-from lp.soyuz.enums import PackagePublishingStatus
+from lp.soyuz.enums import (
+ PackagePublishingStatus,
+ )
from lp.soyuz.interfaces.publishing import ISourcePackagePublishingHistory
from lp.soyuz.tests.test_publishing import TestNativePublishingBase
from lp.testing import (
@@ -169,6 +171,136 @@
dominator._dominatePublications,
pubs, GeneralizedPublication(True))
+ def test_archall_domination(self):
+ # Arch-all binaries should not be dominated when a new source
+ # version builds an updated arch-all binary, because slower builds
+ # of other architectures will leave the previous version
+ # uninstallable if they depend on the arch-all binary.
+ # See https://bugs.launchpad.net/launchpad/+bug/34086
+
+ # Set up a source, "foo" which builds "foo-bin" and foo-common
+ # (which is arch-all).
+ foo_10_src = self.getPubSource(
+ sourcename="foo", version="1.0", architecturehintlist="i386",
+ status=PackagePublishingStatus.PUBLISHED)
+ [foo_10_i386_bin] = self.getPubBinaries(
+ binaryname="foo-bin", status=PackagePublishingStatus.PUBLISHED,
+ architecturespecific=True, version="1.0", pub_source=foo_10_src)
+ [build] = foo_10_src.getBuilds()
+ bpr = self.factory.makeBinaryPackageRelease(
+ binarypackagename="foo-common", version="1.0", build=build,
+ architecturespecific=False)
+ foo_10_all_bins = self.publishBinaryInArchive(
+ bpr, self.ubuntutest.main_archive, pocket=foo_10_src.pocket,
+ status=PackagePublishingStatus.PUBLISHED)
+
+ # Now, make version 1.1 of foo and add a foo-common but not foo-bin
+ # (imagine that it's not finished building yet).
+ foo_11_src = self.getPubSource(
+ sourcename="foo", version="1.1", architecturehintlist="all",
+ status=PackagePublishingStatus.PUBLISHED)
+ foo_11_all_bins = self.getPubBinaries(
+ binaryname="foo-common", status=PackagePublishingStatus.PUBLISHED,
+ architecturespecific=False, version="1.1", pub_source=foo_11_src)
+
+ dominator = Dominator(self.logger, self.ubuntutest.main_archive)
+ dominator.judgeAndDominate(
+ foo_10_src.distroseries, foo_10_src.pocket)
+
+ # The source will be superseded.
+ self.checkPublication(foo_10_src, PackagePublishingStatus.SUPERSEDED)
+ # The arch-specific has no dominant, so it's still published
+ self.checkPublication(
+ foo_10_i386_bin, PackagePublishingStatus.PUBLISHED)
+ # The arch-indep has a dominant but must not be superseded yet
+ # since the arch-specific is still published.
+ self.checkPublications(
+ foo_10_all_bins, PackagePublishingStatus.PUBLISHED)
+
+ # Now creating a newer foo-bin should see those last two
+ # publications superseded.
+ [build2] = foo_11_src.getBuilds()
+ foo_11_bin = self.factory.makeBinaryPackageRelease(
+ binarypackagename="foo-bin", version="1.1", build=build2,
+ architecturespecific=True)
+ self.publishBinaryInArchive(
+ foo_11_bin, self.ubuntutest.main_archive,
+ pocket=foo_10_src.pocket,
+ status=PackagePublishingStatus.PUBLISHED)
+ dominator.judgeAndDominate(
+ foo_10_src.distroseries, foo_10_src.pocket)
+ self.checkPublication(
+ foo_10_i386_bin, PackagePublishingStatus.SUPERSEDED)
+ self.checkPublications(
+ foo_10_all_bins, PackagePublishingStatus.SUPERSEDED)
+
+ def test_any_superseded_by_all(self):
+ # Set up a source, foo, which builds an architecture-dependent
+ # binary, foo-bin.
+ foo_10_src = self.getPubSource(
+ sourcename="foo", version="1.0", architecturehintlist="i386",
+ status=PackagePublishingStatus.PUBLISHED)
+ [foo_10_i386_bin] = self.getPubBinaries(
+ binaryname="foo-bin", status=PackagePublishingStatus.PUBLISHED,
+ architecturespecific=True, version="1.0", pub_source=foo_10_src)
+
+ # Now, make version 1.1 of foo, where foo-bin is now
+ # architecture-independent.
+ foo_11_src = self.getPubSource(
+ sourcename="foo", version="1.1", architecturehintlist="all",
+ status=PackagePublishingStatus.PUBLISHED)
+ [foo_10_all_bin, foo_10_all_bin_2] = self.getPubBinaries(
+ binaryname="foo-bin", status=PackagePublishingStatus.PUBLISHED,
+ architecturespecific=False, version="1.1", pub_source=foo_11_src)
+
+ dominator = Dominator(self.logger, self.ubuntutest.main_archive)
+ dominator.judgeAndDominate(
+ foo_10_src.distroseries, foo_10_src.pocket)
+
+ # The source will be superseded.
+ self.checkPublication(foo_10_src, PackagePublishingStatus.SUPERSEDED)
+ # The arch-specific is superseded by the new arch-indep.
+ self.checkPublication(
+ foo_10_i386_bin, PackagePublishingStatus.SUPERSEDED)
+
+ def test_schitzoid_package(self):
+ # Test domination of a source that produces an arch-indep and an
+ # arch-all, that then switches both on the next version to the
+ # other arch type.
+ foo_10_src = self.getPubSource(
+ sourcename="foo", version="1.0", architecturehintlist="i386",
+ status=PackagePublishingStatus.PUBLISHED)
+ [foo_10_i386_bin] = self.getPubBinaries(
+ binaryname="foo-bin", status=PackagePublishingStatus.PUBLISHED,
+ architecturespecific=True, version="1.0", pub_source=foo_10_src)
+ [build] = foo_10_src.getBuilds()
+ bpr = self.factory.makeBinaryPackageRelease(
+ binarypackagename="foo-common", version="1.0", build=build,
+ architecturespecific=False)
+ foo_10_all_bins = self.publishBinaryInArchive(
+ bpr, self.ubuntutest.main_archive, pocket=foo_10_src.pocket,
+ status=PackagePublishingStatus.PUBLISHED)
+
+ foo_11_src = self.getPubSource(
+ sourcename="foo", version="1.1", architecturehintlist="i386",
+ status=PackagePublishingStatus.PUBLISHED)
+ [foo_11_i386_bin] = self.getPubBinaries(
+ binaryname="foo-common", status=PackagePublishingStatus.PUBLISHED,
+ architecturespecific=True, version="1.1", pub_source=foo_11_src)
+ [build] = foo_11_src.getBuilds()
+ bpr = self.factory.makeBinaryPackageRelease(
+ binarypackagename="foo-bin", version="1.1", build=build,
+ architecturespecific=False)
+ foo_11_all_bins = self.publishBinaryInArchive(
+ bpr, self.ubuntutest.main_archive, pocket=foo_11_src.pocket,
+ status=PackagePublishingStatus.PUBLISHED)
+
+ dominator = Dominator(self.logger, self.ubuntutest.main_archive)
+ dominator.judgeAndDominate(foo_10_src.distroseries, foo_10_src.pocket)
+
+ self.checkPublications(foo_10_all_bins + [foo_10_i386_bin],
+ PackagePublishingStatus.SUPERSEDED)
+
class TestDomination(TestNativePublishingBase):
"""Test overall domination procedure."""
@@ -223,35 +355,6 @@
self.ubuntutest['breezy-autotest'].status = (
SeriesStatus.OBSOLETE)
- def test_any_superseded_by_all(self):
- # Set up a source, foo, which builds an architecture-dependent
- # binary, foo-bin.
- foo_10_src = self.getPubSource(
- sourcename="foo", version="1.0", architecturehintlist="i386",
- status=PackagePublishingStatus.PUBLISHED)
- [foo_10_i386_bin] = self.getPubBinaries(
- binaryname="foo-bin", status=PackagePublishingStatus.PUBLISHED,
- architecturespecific=True, version="1.0", pub_source=foo_10_src)
-
- # Now, make version 1.1 of foo, where foo-bin is now
- # architecture-independent.
- foo_11_src = self.getPubSource(
- sourcename="foo", version="1.1", architecturehintlist="all",
- status=PackagePublishingStatus.PUBLISHED)
- [foo_10_all_bin, foo_10_all_bin_2] = self.getPubBinaries(
- binaryname="foo-bin", status=PackagePublishingStatus.PUBLISHED,
- architecturespecific=False, version="1.1", pub_source=foo_11_src)
-
- dominator = Dominator(self.logger, self.ubuntutest.main_archive)
- dominator.judgeAndDominate(
- foo_10_src.distroseries, foo_10_src.pocket)
-
- # The source will be superseded.
- self.checkPublication(foo_10_src, PackagePublishingStatus.SUPERSEDED)
- # The arch-specific is superseded by the new arch-indep.
- self.checkPublication(
- foo_10_i386_bin, PackagePublishingStatus.SUPERSEDED)
-
def make_spphs_for_versions(factory, versions):
"""Create publication records for each of `versions`.
=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py 2011-10-18 02:09:38 +0000
+++ lib/lp/soyuz/model/publishing.py 2011-10-18 11:39:52 +0000
@@ -33,6 +33,7 @@
Desc,
LeftJoin,
Or,
+ Select,
Sum,
)
from storm.store import Store
@@ -1118,6 +1119,60 @@
section=self.section,
priority=self.priority)
+ def getOtherPublicationsForSameSource(self, include_archindep=False):
+ """Return all the other published or pending binaries for this
+ source.
+
+ For example if source package foo builds:
+ foo - i386
+ foo - amd64
+ foo-common - arch-all (published in i386 and amd64)
+ then if this publication is the arch-all amd64, return foo(i386),
+ foo(amd64). If include_archindep is True then also return
+ foo-common (i386)
+
+ :param include_archindep: If True, return architecture independent
+ publications too. Defaults to False.
+
+ :return: an iterable of `BinaryPackagePublishingHistory`
+ """
+ # Avoid circular wotsits.
+ from lp.soyuz.model.binarypackagebuild import BinaryPackageBuild
+ from lp.soyuz.model.distroarchseries import DistroArchSeries
+ from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
+ source_select = Select(
+ SourcePackageRelease.id,
+ And(
+ BinaryPackageBuild.source_package_release_id ==
+ SourcePackageRelease.id,
+ BinaryPackageRelease.build == BinaryPackageBuild.id,
+ self.binarypackagereleaseID == BinaryPackageRelease.id,
+ ))
+ pubs = [
+ BinaryPackageBuild.source_package_release_id ==
+ SourcePackageRelease.id,
+ SourcePackageRelease.id.is_in(source_select),
+ BinaryPackageRelease.build == BinaryPackageBuild.id,
+ BinaryPackagePublishingHistory.binarypackagereleaseID ==
+ BinaryPackageRelease.id,
+ BinaryPackagePublishingHistory.archiveID == self.archive.id,
+ BinaryPackagePublishingHistory.distroarchseriesID ==
+ DistroArchSeries.id,
+ DistroArchSeries.distroseriesID == self.distroseries.id,
+ BinaryPackagePublishingHistory.pocket == self.pocket,
+ BinaryPackagePublishingHistory.status.is_in(
+ active_publishing_status),
+ BinaryPackagePublishingHistory.id != self.id
+ ]
+
+ if not include_archindep:
+ pubs.append(BinaryPackageRelease.architecturespecific == True)
+
+ return IMasterStore(BinaryPackagePublishingHistory).find(
+ BinaryPackagePublishingHistory,
+ *pubs
+ )
+
def supersede(self, dominant=None, logger=None):
"""See `IBinaryPackagePublishingHistory`."""
# At this point only PUBLISHED (ancient versions) or PENDING (
=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py 2011-10-18 02:09:38 +0000
+++ lib/lp/soyuz/tests/test_publishing.py 2011-10-18 11:39:52 +0000
@@ -1471,6 +1471,112 @@
self.assertEquals(spph.ancestor.displayname, ancestor.displayname)
+class TestGetOtherPublicationsForSameSource(TestNativePublishingBase):
+ """Test parts of the BinaryPackagePublishingHistory model.
+
+ See also lib/lp/soyuz/doc/publishing.txt
+ """
+
+ layer = LaunchpadZopelessLayer
+
+ def _makeMixedSingleBuildPackage(self, version="1.0"):
+ # Set up a source with a build that generated four binaries,
+ # two of them an arch-all.
+ foo_src_pub = self.getPubSource(
+ sourcename="foo", version=version, architecturehintlist="i386",
+ status=PackagePublishingStatus.PUBLISHED)
+ [foo_bin_pub] = self.getPubBinaries(
+ binaryname="foo-bin", status=PackagePublishingStatus.PUBLISHED,
+ architecturespecific=True, version=version,
+ pub_source=foo_src_pub)
+ # Now need to grab the build for the source so we can add
+ # more binaries to it.
+ [build] = foo_src_pub.getBuilds()
+ foo_one_common = self.factory.makeBinaryPackageRelease(
+ binarypackagename="foo-one-common", version=version, build=build,
+ architecturespecific=False)
+ foo_one_common_pubs = self.publishBinaryInArchive(
+ foo_one_common, self.ubuntutest.main_archive,
+ pocket=foo_src_pub.pocket,
+ status=PackagePublishingStatus.PUBLISHED)
+ foo_two_common = self.factory.makeBinaryPackageRelease(
+ binarypackagename="foo-two-common", version=version, build=build,
+ architecturespecific=False)
+ foo_two_common_pubs = self.publishBinaryInArchive(
+ foo_two_common, self.ubuntutest.main_archive,
+ pocket=foo_src_pub.pocket,
+ status=PackagePublishingStatus.PUBLISHED)
+ foo_three = self.factory.makeBinaryPackageRelease(
+ binarypackagename="foo-three", version=version, build=build,
+ architecturespecific=True)
+ [foo_three_pub] = self.publishBinaryInArchive(
+ foo_three, self.ubuntutest.main_archive,
+ pocket=foo_src_pub.pocket,
+ status=PackagePublishingStatus.PUBLISHED)
+ # So now we have source foo, which has arch specific binaries
+ # foo-bin and foo-three, and arch:all binaries foo-one-common and
+ # foo-two-common. The latter two will have multiple publications,
+ # one for each DAS in the series.
+ return (
+ foo_src_pub, foo_bin_pub, foo_one_common_pubs,
+ foo_two_common_pubs, foo_three_pub)
+
+ def test_getOtherPublicationsForSameSource(self):
+ # By default getOtherPublicationsForSameSource should return all
+ # of the other binaries built by the same source as the passed
+ # binary publication, except the arch-indep ones.
+ (foo_src_pub, foo_bin_pub, foo_one_common_pubs, foo_two_common_pubs,
+ foo_three_pub) = self._makeMixedSingleBuildPackage()
+
+ foo_one_common_pub = foo_one_common_pubs[0]
+ others = foo_one_common_pub.getOtherPublicationsForSameSource()
+ others = list(others)
+
+ self.assertContentEqual([foo_three_pub, foo_bin_pub], others)
+
+ def test_getOtherPublicationsForSameSource_include_archindep(self):
+ # Check that the arch-indep binaries are returned if requested.
+ (foo_src_pub, foo_bin_pub, foo_one_common_pubs, foo_two_common_pubs,
+ foo_three_pub) = self._makeMixedSingleBuildPackage()
+
+ foo_one_common_pub = foo_one_common_pubs[0]
+ others = foo_one_common_pub.getOtherPublicationsForSameSource(
+ include_archindep=True)
+ others = list(others)
+
+ # We expect all publications created above to be returned,
+ # except the one we use to call the method on.
+ expected = [foo_three_pub, foo_bin_pub]
+ expected.extend(foo_one_common_pubs[1:])
+ expected.extend(foo_two_common_pubs)
+ self.assertContentEqual(expected, others)
+
+ def test_getOtherPublicationsForSameSource_inactive(self):
+ # Check that inactive publications are not returned.
+ (foo_src_pub, foo_bin_pub, foo_one_common_pubs, foo_two_common_pubs,
+ foo_three_pub) = self._makeMixedSingleBuildPackage()
+ foo_bin_pub.status = PackagePublishingStatus.SUPERSEDED
+ foo_three_pub.status = PackagePublishingStatus.SUPERSEDED
+ foo_one_common_pub = foo_one_common_pubs[0]
+ others = foo_one_common_pub.getOtherPublicationsForSameSource()
+ others = list(others)
+
+ self.assertEqual(0, len(others))
+
+ def test_getOtherPublicationsForSameSource_multiple_versions(self):
+ # Check that publications for only the same version as the
+ # context binary publication are returned.
+ (foo_src_pub, foo_bin_pub, foo_one_common_pubs, foo_two_common_pubs,
+ foo_three_pub) = self._makeMixedSingleBuildPackage(version="1.0")
+ self._makeMixedSingleBuildPackage(version="1.1")
+
+ foo_one_common_pub = foo_one_common_pubs[0]
+ others = foo_one_common_pub.getOtherPublicationsForSameSource()
+ others = list(others)
+
+ self.assertContentEqual([foo_three_pub, foo_bin_pub], others)
+
+
class TestGetBuiltBinaries(TestNativePublishingBase):
"""Test SourcePackagePublishingHistory.getBuiltBinaries() works."""
Follow ups
