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.

Sign up for GitHub

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

Fix issues with deb_index and Nvidia's apt repository #67

Closed
wants to merge 5 commits into from
+33 −17
Closed

Fix issues with deb_index and Nvidia's apt repository #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1cce1b1
add support for third party repos
benmccown May 29, 2024
f26d909
debugging
benmccown May 29, 2024
d7a7c16
add domain to target_triple
benmccown May 29, 2024
fcd13a7
Revert "debugging"
benmccown May 29, 2024
ff05345
Merge branch 'main' into main
benmccown Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions apt/private/index.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,17 +85,19 @@ def _deb_package_index_impl(rctx):
),
)

deps = depset([
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had packages that shared duplicate transitive dependencies. The duplicate deps were a blocking issue, so I used a depset to dedupe packages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have an example the package(s) that broke? Were they packages in the NVIDIA CUDA repos?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjmaestro I had this in my sources:

  - channel: nvidia cuda
    url: https://developer.download.nvidia.com/compute/cuda/repos/ubuntu2004/x86_64

And this in my packages:

  # CUDA Dependencies
  - cuda-11-8
  - cudnn9-cuda-11-8

I think those would replicate the issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benmccown I'm checking the Packages index and there's no cuda-11-8 package, only cudnn9-cuda-11-8 :-/ I'll try to test with the latter and with other packages to see if I can repro it as well.

Copy link
Contributor

@jjmaestro jjmaestro Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benmccown OK, I looked more carefully and I was using the wrong Packages index / arch but that's also the problem for the repro, I can't get both of those packages for both amd64 and arm64 (they are only available for amd64 IIRC).

Have a look at jjmaestro/rules_distroless/tree/testing-cuda-packages, I have one commit there so far: jjmaestro@f216178, this is a test running on top of #97.

I've gotten it to work with just the cuda-11-8 package in arm64, and since that's the largest package by far, I think it's probably working but I'll change the example I'm working on to only amd64 and see if something breaks when adding both packages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found yet-another-issue when testing with amd64... the failure I'm getting is because there's a Debian package that's already compressed as a tar.gz, see #98. I'll see if I can work around that issue and test things further but so far, I haven't seen duplicate transitive dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, @benmccown please check jjmaestro/rules_distroless/tree/testing-cuda-packages.

I didn't get any errors / issues with duplicate transitive dependencies. I have it all working after porting #99 and also making sure I flatten the layers to avoid max depth exceeded (#36) and also, using pkg_tar to flatten, to avoid duplicates of file paths not supported Docker errors (e.g. bazelbuild/rules_docker#246).

Can you check that branch and confirm if that works for you?

Thanks!

'"//%s/%s"' % (dep["name"], package["arch"])
for dep in package["dependencies"]
])
repo_name = "%s%s_%s" % ("@" if rctx.attr.bzlmod else "", rctx.attr.name, package_key)

rctx.file(
"%s/%s/BUILD.bazel" % (package["name"], package["arch"]),
rctx.attr.package_template.format(
target_name = package["arch"],

src = '"@%s//:data"' % repo_name,
deps = ",\n ".join([
'"//%s/%s"' % (dep["name"], package["arch"])
for dep in package["dependencies"]
]),
deps = ",\n ".join(deps.to_list()),
urls = [package["url"]],
name = package["name"],
arch = package["arch"],
Expand Down
5 changes: 4 additions & 1 deletion apt/private/lockfile.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,14 @@ def _add_package(lock, package, arch):
k = _package_key(package, arch)
if k in lock.fast_package_lookup:
return
filename = package["Filename"]
if filename.startswith("./"):
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When Packages lives in the same directory/URL that the packages themselves live, the manifest has ./ as the leading file path for all the packages listed, however we don't want a ./ in the URL.

Copy link
Contributor

@jjmaestro jjmaestro Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this is not what the spec says, check https://wiki.debian.org/DebianRepository/Format#Filename:

Filename

The mandatory Filename field shall list the path of the package archive relative to the base directory of the repository. The path should be in canonical form, that is, without any components denoting the current or parent directory ("." or ".."). It also should not make use of any protocol-specific components, such as URL-encoded parameters.

I've checked the R-Project flat repos and this doesn't happen / is not needed. However, this is something that happens in the NVIDIA CUDA flat repos . Thus, I think that this is a quirk of some specific flat repos that don't conform with the Debian repo spec.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super interesting. I was looking for documentation on the manifest format for a debian repository and was having a hard time finding anything authoritative. Thanks for sharing the link.

filename = filename[2:]
lock.packages.append({
"key": k,
"name": package["Package"],
"version": package["Version"],
"url": "%s/%s" % (package["Root"], package["Filename"]),
"url": "%s/%s" % (package["Root"], filename),
"sha256": package["SHA256"],
"arch": arch,
"dependencies": [],
Expand Down
35 changes: 23 additions & 12 deletions apt/private/package_index.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,36 @@
load(":util.bzl", "util")

def _fetch_package_index(rctx, url, dist, comp, arch, integrity):
target_triple = "{dist}/{comp}/{arch}".format(dist = dist, comp = comp, arch = arch)
# Split the URL by the '://' delimiter
protocol, rest = url.split("://")

# Split the rest of the URL by the '/' delimiter and take the first part
domain = rest.split("/")[0]

target_triple = "{domain}/{dist}/{comp}/{arch}".format(domain = domain, dist = dist, comp = comp, arch = arch)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We needed to add domain to the target_triple because dist/comp/arch was duplicated across repositories since Ubuntu and Nvidia's repositories were both focal/main/x86_64.

target_triple is probably no longer the right term to use. I can change it as you see fit.


file_types = {"xz": ["xz", "--decompress"], "gz": ["gzip", "-d"]}
r = {"success": False, "integrity": None}

decompression_successful = False
for file_type, tool in file_types.items():
output = "{}/Packages.{}".format(target_triple, file_type)
r = rctx.download(
url = "{}/dists/{}/{}/binary-{}/Packages.{}".format(url, dist, comp, arch, file_type),
output = output,
integrity = integrity,
allow_fail = True,
)
if r.success:
re = rctx.execute(tool + [output])
if re.return_code == 0:
decompression_successful = True
break
urls = [
"{}/dists/{}/{}/binary-{}/Packages.{}".format(url, dist, comp, arch, file_type),
"{}/Packages.{}".format(url, file_type),
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line adds support for Nvidia's Packages location. We had to add gzip support as well since they didn't have an xz compressed archive.

]
for package_index_url in urls:
r = rctx.download(
url = package_index_url,
output = output,
integrity = integrity,
allow_fail = True,
)
if r.success:
re = rctx.execute(tool + [output])
if re.return_code == 0:
decompression_successful = True
return ("{}/Packages".format(target_triple), r.integrity)

if not r.success:
fail("unable to download package index")
Expand Down