launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28960
[Merge] ~cjwatson/launchpad:fix-sdist-package-name into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:fix-sdist-package-name into launchpad:master.
Commit message:
Publish Python sdists to the correct paths
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1983401 in Launchpad itself: "launchpad publishes python source packages by launchpad source package name rather than python package name"
https://bugs.launchpad.net/launchpad/+bug/1983401
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/427901
When creating a `SourcePackageRelease` from a `CIBuild`, we have to take the release's source package name from the namespace of the build's Git repository, since that means everything is grouped together properly under the same source package name, and it also allows us to handle source package types whose true package name can't be used as a source package name (notably Go modules); teams building these also generally try to use the same source package name as used in Ubuntu, to make it easier to associate fixes in different distributions.
However, this meant that we published Python sdists to the wrong paths if their true package name didn't correspond to the repository namespace. To fix this, pass through the true package name via `SourcePackageRelease.user_defined_fields` and use that in the publishing machinery.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-sdist-package-name into launchpad:master.
diff --git a/lib/lp/archivepublisher/artifactory.py b/lib/lp/archivepublisher/artifactory.py
index e39e6f5..18190a4 100644
--- a/lib/lp/archivepublisher/artifactory.py
+++ b/lib/lp/archivepublisher/artifactory.py
@@ -44,19 +44,27 @@ def _path_for(
pub_file: IPackageReleaseFile,
) -> Path:
repository_format = archive.repository_format
+ if ISourcePackageReleaseFile.providedBy(pub_file):
+ release = pub_file.sourcepackagerelease
+ elif IBinaryPackageFile.providedBy(pub_file):
+ release = pub_file.binarypackagerelease
+ else:
+ # There are no other kinds of `IPackageReleaseFile` at the moment.
+ raise AssertionError("Unsupported file: %r" % pub_file)
if repository_format == ArchiveRepositoryFormat.DEBIAN:
path = rootpath / poolify(source_name)
elif repository_format == ArchiveRepositoryFormat.PYTHON:
- path = rootpath / source_name / source_version
+ package_name = release.getUserDefinedField("package-name")
+ if package_name is None:
+ package_name = source_name
+ path = rootpath / package_name / source_version
elif repository_format == ArchiveRepositoryFormat.CONDA:
- subdir = pub_file.binarypackagerelease.getUserDefinedField("subdir")
+ subdir = release.getUserDefinedField("subdir")
if subdir is None:
raise ValueError("Cannot publish a Conda package with no subdir")
path = rootpath / subdir
elif repository_format == ArchiveRepositoryFormat.GO_PROXY:
- module_path = pub_file.sourcepackagerelease.getUserDefinedField(
- "module-path"
- )
+ module_path = release.getUserDefinedField("module-path")
if module_path is None:
raise ValueError("Cannot publish a Go module with no module-path")
# Base path required by https://go.dev/ref/mod#module-proxy.
diff --git a/lib/lp/archivepublisher/tests/test_artifactory.py b/lib/lp/archivepublisher/tests/test_artifactory.py
index 54ce290..b86b619 100644
--- a/lib/lp/archivepublisher/tests/test_artifactory.py
+++ b/lib/lp/archivepublisher/tests/test_artifactory.py
@@ -630,6 +630,83 @@ class TestArtifactoryPoolFromLibrarian(TestCaseWithFactory):
ci_build = self.factory.makeCIBuild(distro_arch_series=das)
spr = self.factory.makeSourcePackageRelease(
archive=pool.archive,
+ sourcepackagename="python-foo",
+ version="1.0",
+ format=SourcePackageType.CI_BUILD,
+ ci_build=ci_build,
+ user_defined_fields=[("package-name", "foo")],
+ )
+ spph = self.factory.makeSourcePackagePublishingHistory(
+ archive=pool.archive,
+ sourcepackagerelease=spr,
+ distroseries=dses[0],
+ pocket=PackagePublishingPocket.RELEASE,
+ component="main",
+ sourcepackagename="python-foo",
+ version="1.0",
+ channel="edge",
+ format=SourcePackageType.CI_BUILD,
+ )
+ spr = spph.sourcepackagerelease
+ sprf = self.factory.makeSourcePackageReleaseFile(
+ sourcepackagerelease=spr,
+ library_file=self.factory.makeLibraryFileAlias(
+ filename="foo-1.0.tar.gz"
+ ),
+ filetype=SourcePackageFileType.SDIST,
+ )
+ spphs = [spph]
+ spphs.append(
+ spph.copyTo(dses[1], PackagePublishingPocket.RELEASE, pool.archive)
+ )
+ transaction.commit()
+ pool.addFile(None, spr.name, spr.version, sprf)
+ path = pool.rootpath / "foo" / "1.0" / "foo-1.0.tar.gz"
+ self.assertTrue(path.exists())
+ self.assertFalse(path.is_symlink())
+ self.assertEqual(
+ {
+ "launchpad.release-id": ["source:%d" % spr.id],
+ "launchpad.source-name": ["python-foo"],
+ "launchpad.source-version": ["1.0"],
+ "soss.source_url": [
+ ci_build.git_repository.getCodebrowseUrl()
+ ],
+ "soss.commit_id": [ci_build.commit_sha1],
+ },
+ path.properties,
+ )
+ pool.updateProperties(spr.name, spr.version, [sprf], spphs)
+ self.assertEqual(
+ {
+ "launchpad.release-id": ["source:%d" % spr.id],
+ "launchpad.source-name": ["python-foo"],
+ "launchpad.source-version": ["1.0"],
+ "launchpad.channel": list(
+ sorted("%s:edge" % ds.name for ds in dses)
+ ),
+ "soss.source_url": [
+ ci_build.git_repository.getCodebrowseUrl()
+ ],
+ "soss.commit_id": [ci_build.commit_sha1],
+ },
+ path.properties,
+ )
+
+ def test_updateProperties_python_sdist_no_package_name(self):
+ # Older sdist publications don't have package-name in
+ # user_defined_fields.
+ pool = self.makePool(ArchiveRepositoryFormat.PYTHON)
+ dses = [
+ self.factory.makeDistroSeries(
+ distribution=pool.archive.distribution
+ )
+ for _ in range(2)
+ ]
+ das = self.factory.makeDistroArchSeries(distroseries=dses[0])
+ ci_build = self.factory.makeCIBuild(distro_arch_series=das)
+ spr = self.factory.makeSourcePackageRelease(
+ archive=pool.archive,
sourcepackagename="foo",
version="1.0",
format=SourcePackageType.CI_BUILD,
diff --git a/lib/lp/archivepublisher/tests/test_pool.py b/lib/lp/archivepublisher/tests/test_pool.py
index f6be768..fdd9b10 100644
--- a/lib/lp/archivepublisher/tests/test_pool.py
+++ b/lib/lp/archivepublisher/tests/test_pool.py
@@ -61,7 +61,7 @@ class FakePackageRelease:
self.ci_build = ci_build
def getUserDefinedField(self, name):
- for k, v in self.user_defined_fields:
+ for k, v in self.user_defined_fields or []:
if k.lower() == name.lower():
return v
diff --git a/lib/lp/soyuz/model/archivejob.py b/lib/lp/soyuz/model/archivejob.py
index e713984..fca71f7 100644
--- a/lib/lp/soyuz/model/archivejob.py
+++ b/lib/lp/soyuz/model/archivejob.py
@@ -434,6 +434,12 @@ class CIBuildUploadJob(ArchiveJobDerived):
format=SourcePackageFileType.SDIST,
name=sdist.name,
version=sdist.version,
+ # Arrange for this to be explicitly stored in the database.
+ # The source package name will be taken from the namespace
+ # of the originating repository, but we need the true
+ # upstream package name as well in order to publish files to
+ # the correct location.
+ user_defined_fields=[("package-name", sdist.name)],
)
return all_metadata
@@ -648,9 +654,17 @@ class CIBuildUploadJob(ArchiveJobDerived):
build_target = self.ci_build.git_repository.target
spr = releases.get(build_target.sourcepackagename)
if spr is None:
- # Arbitrarily pick the first scanned artifact to provide
- # additional metadata. (This may need refinement in future.)
- first_metadata = scanned[0].metadata
+ # Arbitrarily pick the first scanned source artifact to provide
+ # additional metadata, falling back to the first non-source
+ # artifact if there are no source artifacts. (This may need
+ # refinement in future.)
+ for scanned_artifact in scanned:
+ metadata = scanned_artifact.metadata
+ if isinstance(metadata, SourceArtifactMetadata):
+ first_metadata = metadata
+ break
+ else:
+ first_metadata = scanned[0].metadata
spr = self.ci_build.createSourcePackageRelease(
distroseries=distroseries,
sourcepackagename=build_target.sourcepackagename,
diff --git a/lib/lp/soyuz/model/binarypackagerelease.py b/lib/lp/soyuz/model/binarypackagerelease.py
index 1393515..ac1f4f3 100644
--- a/lib/lp/soyuz/model/binarypackagerelease.py
+++ b/lib/lp/soyuz/model/binarypackagerelease.py
@@ -6,11 +6,11 @@ __all__ = [
"BinaryPackageReleaseDownloadCount",
]
+import json
import re
from operator import attrgetter
from typing import Any
-import simplejson
from storm.locals import Date, Int, Reference, Store, Storm
from zope.component import getUtility
from zope.interface import implementer
@@ -161,7 +161,7 @@ class BinaryPackageRelease(SQLBase):
def __init__(self, *args, **kwargs):
if "user_defined_fields" in kwargs:
- kwargs["_user_defined_fields"] = simplejson.dumps(
+ kwargs["_user_defined_fields"] = json.dumps(
kwargs["user_defined_fields"]
)
del kwargs["user_defined_fields"]
@@ -189,7 +189,10 @@ class BinaryPackageRelease(SQLBase):
"""See `IBinaryPackageRelease`."""
if self._user_defined_fields is None:
return []
- return simplejson.loads(self._user_defined_fields)
+ user_defined_fields = json.loads(self._user_defined_fields)
+ if user_defined_fields is None:
+ return []
+ return user_defined_fields
def getUserDefinedField(self, name):
for k, v in self.user_defined_fields:
diff --git a/lib/lp/soyuz/tests/test_archivejob.py b/lib/lp/soyuz/tests/test_archivejob.py
index 1992e3c..28c7e80 100644
--- a/lib/lp/soyuz/tests/test_archivejob.py
+++ b/lib/lp/soyuz/tests/test_archivejob.py
@@ -364,6 +364,7 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
format=SourcePackageFileType.SDIST,
name="wheel-arch",
version="0.0.1",
+ user_defined_fields=[("package-name", "wheel-arch")],
),
}
),
@@ -728,6 +729,9 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
filetype=SourcePackageFileType.SDIST,
)
),
+ user_defined_fields=Equals(
+ [["package-name", "wheel-arch"]]
+ ),
),
format=Equals(SourcePackageType.CI_BUILD),
distroseries=Equals(distroseries),
diff --git a/lib/lp/soyuz/tests/test_binarypackagerelease.py b/lib/lp/soyuz/tests/test_binarypackagerelease.py
index f01c244..f1c8b71 100644
--- a/lib/lp/soyuz/tests/test_binarypackagerelease.py
+++ b/lib/lp/soyuz/tests/test_binarypackagerelease.py
@@ -57,6 +57,23 @@ class TestBinaryPackageRelease(TestCaseWithFactory):
release.user_defined_fields,
)
+ def test_getUserDefinedField_no_fields(self):
+ release = self.factory.makeBinaryPackageRelease()
+ self.assertIsNone(release.getUserDefinedField("Missing"))
+
+ def test_getUserDefinedField_present(self):
+ release = self.factory.makeBinaryPackageRelease(
+ user_defined_fields=[("Key", "value")]
+ )
+ self.assertEqual("value", release.getUserDefinedField("Key"))
+ self.assertEqual("value", release.getUserDefinedField("key"))
+
+ def test_getUserDefinedField_absent(self):
+ release = self.factory.makeBinaryPackageRelease(
+ user_defined_fields=[("Key", "value")]
+ )
+ self.assertIsNone(release.getUserDefinedField("Other-Key"))
+
def test_homepage_default(self):
# By default, no homepage is set.
bpr = self.factory.makeBinaryPackageRelease()
