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

Add has_lib and bin_names fields to the versions API #8859

Merged
merged 5 commits into from
Jul 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
93 changes: 87 additions & 6 deletions crates/crates_io_tarball/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ pub use crate::builder::TarballBuilder;
use crate::limit_reader::LimitErrorReader;
use crate::manifest::validate_manifest;
pub use crate::vcs_info::CargoVcsInfo;
use cargo_manifest::AbstractFilesystem;
pub use cargo_manifest::{Manifest, StringOrBool};
use flate2::read::GzDecoder;
use std::collections::BTreeMap;
use std::collections::{BTreeMap, BTreeSet};
use std::io::Read;
use std::path::{Path, PathBuf};
use std::path::{Component, Path, PathBuf};
use std::str::FromStr;
use tracing::instrument;

Expand Down Expand Up @@ -66,6 +67,7 @@ pub fn process_tarball<R: Read>(
let pkg_root = Path::new(&pkg_name);

let mut vcs_info = None;
let mut paths = Vec::new();
let mut manifests = BTreeMap::new();

for entry in archive.entries()? {
Expand All @@ -77,9 +79,9 @@ pub fn process_tarball<R: Read>(
// as `bar-0.1.0/` source code, and this could overwrite other crates in
// the registry!
let entry_path = entry.path()?;
if !entry_path.starts_with(pkg_name) {
let Ok(in_pkg_path) = entry_path.strip_prefix(pkg_root) else {
return Err(TarballError::InvalidPath(entry_path.display().to_string()));
}
};

// Historical versions of the `tar` crate which Cargo uses internally
// don't properly prevent hard links and symlinks from overwriting
Expand All @@ -93,6 +95,8 @@ pub fn process_tarball<R: Read>(
));
}

paths.push(in_pkg_path.to_path_buf());

// Let's go hunting for the VCS info and crate manifest. The only valid place for these is
// in the package root in the tarball.
if entry_path.parent() == Some(pkg_root) {
Expand Down Expand Up @@ -127,7 +131,7 @@ pub fn process_tarball<R: Read>(
// on case-insensitive filesystems, to match the behaviour of cargo we should only actually
// accept `Cargo.toml` and (the now deprecated) `cargo.toml` as valid options for the
// manifest.
let Some((path, manifest)) = manifests.pop_first() else {
let Some((path, mut manifest)) = manifests.pop_first() else {
return Err(TarballError::MissingManifest);
};

Expand All @@ -136,15 +140,45 @@ pub fn process_tarball<R: Read>(
return Err(TarballError::IncorrectlyCasedManifest(file.into()));
}

manifest.complete_from_abstract_filesystem(&PathsFileSystem(paths))?;

Ok(TarballInfo { manifest, vcs_info })
}

struct PathsFileSystem(Vec<PathBuf>);

impl AbstractFilesystem for PathsFileSystem {
fn file_names_in<T: AsRef<Path>>(&self, rel_path: T) -> std::io::Result<BTreeSet<Box<str>>> {
let mut rel_path = rel_path.as_ref();

// Deal with relative paths that start with `./`
let mut components = rel_path.components();
while components.next() == Some(Component::CurDir) {
rel_path = components.as_path();
}

let paths = &self.0;
let file_names = paths
.iter()
.filter_map(move |p| p.strip_prefix(rel_path).ok())
.filter_map(|name| match name.components().next() {
// We can skip non-utf8 paths, since those are not checked by `cargo_manifest` anyway
Some(Component::Normal(p)) => p.to_str(),
_ => None,
})
.map(From::from)
.collect();

Ok(file_names)
}
}

#[cfg(test)]
mod tests {
use super::process_tarball;
use crate::TarballBuilder;
use cargo_manifest::{MaybeInherited, StringOrBool};
use insta::assert_snapshot;
use insta::{assert_debug_snapshot, assert_snapshot};

const MANIFEST: &[u8] = b"[package]\nname = \"foo\"\nversion = \"0.0.1\"\n";
const MAX_SIZE: u64 = 512 * 1024 * 1024;
Expand All @@ -157,6 +191,9 @@ mod tests {

let tarball_info = assert_ok!(process_tarball("foo-0.0.1", &*tarball, MAX_SIZE));
assert_none!(tarball_info.vcs_info);
assert_none!(tarball_info.manifest.lib);
assert_eq!(tarball_info.manifest.bin, vec![]);
assert_eq!(tarball_info.manifest.example, vec![]);

let err = assert_err!(process_tarball("bar-0.0.1", &*tarball, MAX_SIZE));
assert_snapshot!(err, @"invalid path found: foo-0.0.1/Cargo.toml");
Expand Down Expand Up @@ -309,4 +346,48 @@ mod tests {
let err = assert_err!(process(vec!["Cargo.toml", "cargo.toml", "CARGO.TOML"]));
assert_snapshot!(err, @r###"more than one Cargo.toml manifest in tarball: ["foo-0.0.1/CARGO.TOML", "foo-0.0.1/Cargo.toml", "foo-0.0.1/cargo.toml"]"###);
}

#[test]
fn test_lib() {
let tarball = TarballBuilder::new()
.add_file("foo-0.0.1/Cargo.toml", MANIFEST)
.add_file("foo-0.0.1/src/lib.rs", b"pub fn foo() {}")
.build();

let tarball_info = assert_ok!(process_tarball("foo-0.0.1", &*tarball, MAX_SIZE));
let lib = assert_some!(tarball_info.manifest.lib);
assert_debug_snapshot!(lib);
assert_eq!(tarball_info.manifest.bin, vec![]);
assert_eq!(tarball_info.manifest.example, vec![]);
}

#[test]
fn test_lib_with_bins_and_example() {
let tarball = TarballBuilder::new()
.add_file("foo-0.0.1/Cargo.toml", MANIFEST)
.add_file("foo-0.0.1/examples/how-to-use-foo.rs", b"fn main() {}")
.add_file("foo-0.0.1/src/lib.rs", b"pub fn foo() {}")
.add_file("foo-0.0.1/src/bin/foo.rs", b"fn main() {}")
.add_file("foo-0.0.1/src/bin/bar.rs", b"fn main() {}")
.build();

let tarball_info = assert_ok!(process_tarball("foo-0.0.1", &*tarball, MAX_SIZE));
let lib = assert_some!(tarball_info.manifest.lib);
assert_debug_snapshot!(lib);
assert_debug_snapshot!(tarball_info.manifest.bin);
assert_debug_snapshot!(tarball_info.manifest.example);
}

#[test]
fn test_app() {
let tarball = TarballBuilder::new()
.add_file("foo-0.0.1/Cargo.toml", MANIFEST)
.add_file("foo-0.0.1/src/main.rs", b"fn main() {}")
.build();

let tarball_info = assert_ok!(process_tarball("foo-0.0.1", &*tarball, MAX_SIZE));
assert_none!(tarball_info.manifest.lib);
assert_debug_snapshot!(tarball_info.manifest.bin);
assert_eq!(tarball_info.manifest.example, vec![]);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
---
source: crates/crates_io_tarball/src/lib.rs
expression: tarball_info.manifest.bin
---
[
Product {
path: Some(
"src/main.rs",
),
name: Some(
"foo",
),
test: true,
doctest: true,
bench: true,
doc: true,
plugin: false,
proc_macro: false,
harness: true,
edition: None,
required_features: [],
crate_type: Some(
[
"bin",
],
),
},
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
---
source: crates/crates_io_tarball/src/lib.rs
expression: lib
---
Product {
path: Some(
"src/lib.rs",
),
name: Some(
"foo",
),
test: true,
doctest: true,
bench: true,
doc: true,
plugin: false,
proc_macro: false,
harness: true,
edition: None,
required_features: [],
crate_type: Some(
[
"lib",
],
),
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
---
source: crates/crates_io_tarball/src/lib.rs
expression: tarball_info.manifest.bin
---
[
Product {
path: Some(
"src/bin/bar.rs",
),
name: Some(
"bar",
),
test: true,
doctest: true,
bench: true,
doc: true,
plugin: false,
proc_macro: false,
harness: true,
edition: None,
required_features: [],
crate_type: Some(
[
"bin",
],
),
},
Product {
path: Some(
"src/bin/foo.rs",
),
name: Some(
"foo",
),
test: true,
doctest: true,
bench: true,
doc: true,
plugin: false,
proc_macro: false,
harness: true,
edition: None,
required_features: [],
crate_type: Some(
[
"bin",
],
),
},
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
---
source: crates/crates_io_tarball/src/lib.rs
expression: tarball_info.manifest.example
---
[
Product {
path: Some(
"examples/how-to-use-foo.rs",
),
name: Some(
"how-to-use-foo",
),
test: true,
doctest: true,
bench: true,
doc: true,
plugin: false,
proc_macro: false,
harness: true,
edition: None,
required_features: [],
crate_type: Some(
[
"bin",
],
),
},
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
---
source: crates/crates_io_tarball/src/lib.rs
expression: lib
---
Product {
path: Some(
"src/lib.rs",
),
name: Some(
"foo",
),
test: true,
doctest: true,
bench: true,
doc: true,
plugin: false,
proc_macro: false,
harness: true,
edition: None,
required_features: [],
crate_type: Some(
[
"lib",
],
),
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
alter table versions
drop column has_lib,
drop column bin_names;
6 changes: 6 additions & 0 deletions migrations/2024-06-14-081653_add-lib-and-bins-columns/up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
alter table versions
add has_lib boolean,
add bin_names text[];

comment on column versions.has_lib is 'TRUE if the version has a library (e.g. `src/lib.rs`), FALSE if no library was detected, or NULL if the version has not been analyzed yet.';
comment on column versions.bin_names is 'list of the names of all detected binaries in the version. the list may be empty which indicates that no binaries were detected in the version. the column may be NULL is the version has not been analyzed yet.';
Turbo87 marked this conversation as resolved.
Show resolved Hide resolved
10 changes: 10 additions & 0 deletions src/controllers/krate/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,14 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra
}
}

// https://doc.rust-lang.org/cargo/reference/cargo-targets.html#the-name-field says that
// the `name` field is required for `bin` targets, so we can ignore `None` values via
// `filter_map()` here.
let bin_names = tarball_info.manifest.bin
.into_iter()
.filter_map(|bin| bin.name.clone())
.collect();

// Read tarball from request
let hex_cksum: String = Sha256::digest(&tarball_bytes).encode_hex();

Expand All @@ -352,6 +360,8 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra
.checksum(hex_cksum)
.links(package.links)
.rust_version(rust_version)
.has_lib(tarball_info.manifest.lib.is_some())
.bin_names(bin_names)
.build()
.map_err(|error| internal(error.to_string()))?
.save(conn, &verified_email_address)?;
Expand Down
6 changes: 6 additions & 0 deletions src/models/version.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ pub struct Version {
pub checksum: String,
pub links: Option<String>,
pub rust_version: Option<String>,
pub has_lib: Option<bool>,
pub bin_names: Option<Vec<Option<String>>>,
}

impl Version {
Expand Down Expand Up @@ -82,6 +84,10 @@ pub struct NewVersion {
links: Option<String>,
#[builder(default)]
rust_version: Option<String>,
#[builder(default, setter(strip_option))]
pub has_lib: Option<bool>,
#[builder(default, setter(strip_option))]
pub bin_names: Option<Vec<String>>,
}

impl NewVersionBuilder {
Expand Down
4 changes: 4 additions & 0 deletions src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,10 @@ diesel::table! {
///
/// (Automatically generated by Diesel.)
rust_version -> Nullable<Varchar>,
/// TRUE if the version has a library (e.g. `src/lib.rs`), FALSE if no library was detected, or NULL if the version has not been analyzed yet.
has_lib -> Nullable<Bool>,
/// list of the names of all detected binaries in the version. the list may be empty which indicates that no binaries were detected in the version. the column may be NULL is the version has not been analyzed yet.
bin_names -> Nullable<Array<Nullable<Text>>>,
}
}

Expand Down
Loading