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

extract library #314

Merged
merged 46 commits into from
Feb 21, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
ae3313e
wip
MarcoIeni Jan 21, 2023
d3dc47b
wip
MarcoIeni Jan 21, 2023
edda25a
wip
MarcoIeni Jan 21, 2023
62286f2
expand match
MarcoIeni Jan 21, 2023
3777e0a
clippy
MarcoIeni Jan 21, 2023
039b163
Merge branch 'main' into lib
MarcoIeni Jan 21, 2023
1d72ef7
clippy
MarcoIeni Jan 21, 2023
c1ba81a
move match
MarcoIeni Jan 21, 2023
fb3f9d2
move rev
MarcoIeni Jan 21, 2023
4a495fd
easier
MarcoIeni Jan 21, 2023
2ef3486
Update src/lib.rs
MarcoIeni Jan 22, 2023
7712fae
Merge remote-tracking branch 'origin' into lib
MarcoIeni Jan 23, 2023
94bd75f
introduce Rustdoc struct
MarcoIeni Jan 25, 2023
9e9d3c6
adapt code to rustdoc refactor
MarcoIeni Jan 25, 2023
afe48e1
Merge branch 'main' into lib
MarcoIeni Jan 25, 2023
b4e9aa6
clippy
MarcoIeni Jan 25, 2023
b27b7ab
fix
MarcoIeni Jan 25, 2023
daa8426
inline function
MarcoIeni Jan 27, 2023
1190770
avoid mixing including and excluded packages
MarcoIeni Jan 27, 2023
1324805
Merge branch 'main' into lib
MarcoIeni Jan 27, 2023
9d35505
clippy
MarcoIeni Jan 27, 2023
c5a7ae8
Merge branch 'main' into lib
MarcoIeni Jan 27, 2023
b3d22d6
Merge branch 'main' into lib
obi1kenobi Feb 3, 2023
ad9d81c
add derive debug
MarcoIeni Feb 5, 2023
8363bd8
Update src/lib.rs
MarcoIeni Feb 5, 2023
368ee5b
add non_exhaustive
MarcoIeni Feb 5, 2023
f626d7e
non_exhaustive
MarcoIeni Feb 5, 2023
19ba58f
Update src/lib.rs
MarcoIeni Feb 5, 2023
ab2dd0a
Update src/lib.rs
MarcoIeni Feb 5, 2023
eac165e
non_exhaustive
MarcoIeni Feb 5, 2023
64509d7
fix
MarcoIeni Feb 5, 2023
9990b69
debug
MarcoIeni Feb 5, 2023
5a334ba
doc comment
MarcoIeni Feb 5, 2023
a496d42
rustdoc
MarcoIeni Feb 5, 2023
bae60ec
Merge branch 'main' into lib
tonowak Feb 5, 2023
b0f7102
Path handling
tonowak Feb 8, 2023
371ccab
remove useless check
MarcoIeni Feb 8, 2023
72ee617
Fix baseline_root when no baseline-path given and baseline-rev passed
tonowak Feb 8, 2023
2449acd
Fallback to XDG cache dir
tonowak Feb 8, 2023
8485cc5
Renamed registry/version items
tonowak Feb 9, 2023
4988c0a
Merge branch 'main' into lib
obi1kenobi Feb 12, 2023
a6bd76b
Merge branch 'main' into lib
MarcoIeni Feb 17, 2023
e5da9d8
fmt
MarcoIeni Feb 18, 2023
22c0081
use directories crate
MarcoIeni Feb 18, 2023
7516ab0
Added panic in 'unknown' branch
tonowak Feb 20, 2023
09bd405
Merge branch 'main' into lib
obi1kenobi Feb 21, 2023
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
2 changes: 1 addition & 1 deletion src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use termcolor::{ColorChoice, StandardStream};
use crate::templating::make_handlebars_registry;

#[allow(dead_code)]
pub(crate) struct GlobalConfig {
pub struct GlobalConfig {
level: Option<log::Level>,
is_stderr_tty: bool,
stdout: StandardStream,
Expand Down
310 changes: 310 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,310 @@
mod baseline;
mod check_release;
mod config;
mod dump;
mod manifest;
mod query;
mod templating;
mod util;

pub use config::*;
pub use query::*;

use check_release::run_check_release;
use trustfall_rustdoc::load_rustdoc;

use std::{collections::HashSet, path::PathBuf};

/// Test a release for semver violations.
#[derive(Default)]
pub struct Check {
tonowak marked this conversation as resolved.
Show resolved Hide resolved
/// Which packages to analyze.
scope: Scope,
current: Current,
baseline: Baseline,
log_level: Option<log::Level>,
Copy link
Owner

Choose a reason for hiding this comment

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

Is this different than GlobalConfig.log_level? I'm confused by the duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided not to expose GlobalConfig to the user. I'm only exposing the log level.
That's because if the user wants to use cargo-semver-checks as a library, I imagine cargo-semver-checks won't write to stdout or stderr in the future. So it doesn't make sense to allow the user to customize stdout and stderr.

}

#[derive(Default)]
enum Baseline {
/// Version from registry to lookup for a baseline. E.g. "1.0.0".
Version(String),
/// Git revision to lookup for a baseline.
Revision(String),
/// Directory containing baseline crate source.
Root(PathBuf),
/// The rustdoc json file to use as a semver baseline.
RustDoc(PathBuf),
/// Latest version published to the cargo registry.
#[default]
LatestVersion,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to group conflicting cli arguments together in enums.


/// Current version of the project to analyze.
#[derive(Default)]
enum Current {
/// Path to the manifest of the current version of the project.
/// It can be a workspace or a single package.
Manifest(PathBuf),
MarcoIeni marked this conversation as resolved.
Show resolved Hide resolved
/// The rustdoc json of the current version of the project.
RustDoc(PathBuf),
Copy link
Contributor Author

@MarcoIeni MarcoIeni Jan 21, 2023

Choose a reason for hiding this comment

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

I saw that this rustdoc also requires the Baseline rustdoc.
At the moment this is not captured by the type system.

  • How would you deal with this?
  • Should we move it outside of this enum?

Copy link
Owner

Choose a reason for hiding this comment

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

All of these allow the baseline rustdoc to be specified explicitly, while only one option requires it. This makes the decision tricky. It feels like this enum isn't at the right level of abstraction.

The baseline and the current rustdoc can be one of three things:

  • already generated, just load it from an existing file
  • generate it based on code at some path, optionally first checking out a gitrev (maybe better split into two cases? what do you think?)
  • generate it based on a registry version (in the current CLI, only the baseline can do this, but @tonowak has a lib use case where it would be useful for the current rustdoc as well)

Perhaps a pub enum RustdocSource like that might be better? Then the CLI can have its own structs and behaviors around "current directory" etc. For testability reasons, it might be best if the library is ignorant of the current directory and always expects explicit paths to be fed to it.

Not set on it, just wanted to suggest an idea to look at.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we want to create a RustdocSource struct that can be used both by the Baseline and Current rustdoc?

already generated, just load it from an existing file

enum RustdocSource {
    RustDoc(PathBuf)
}

generate it based on code at some path, optionally first checking out a gitrev (maybe better split into two cases? what do you think?)

enum RustdocSource {
    RustDoc(PathBuf),
    Root(PathBuf),
    // (path to root, rev)
    Revision(PathBuf, String)
}

generate it based on a registry version

enum RustdocSource {
    RustDoc(PathBuf),
    Root(PathBuf),
    // (path to root, rev)
    Revision(PathBuf, String),
    Version(Option<String>),
}

This looks similar to the current Baseline enum. Is it valid for Current, too?
If I didn't get it, can you give me the structure of the enum you were thinking about?

Copy link
Owner

Choose a reason for hiding this comment

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

I think it looks reasonable. It currently isn't valid for Current in the CLI, but I think it should be in the lib. Not much reason not to: that would make the types more complex while providing less functionality.

Small nit: let's call the variant Rustdoc instead, since the underlying tool doesn't capitalize the D either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It currently isn't valid for Current in the CLI, but I think it should be in the lib.

FYI @MarcoIeni, the recent merged chain of PRs (which ended with #351) made it so that the current crate's rustdoc is generated in the same way as the baseline's rustdoc, so now it's possible to create the current rustdoc from registry and from git revision -- your RustdocSource now is also fully valid for the current crates.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And also, this chain of PRs renamed quite a lot of things in the source code, so there might be a big merge conflict, sorry :( I can try to merge main into this branch when I'll have some free time.

Copy link
Owner

Choose a reason for hiding this comment

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

If you could help with the merge, that would be great @tonowak. Probably the highest impact next thing you can do right now, TBH.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I'll do it in max few days.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done.

/// Use the manifest in the current directory.
#[default]
CurrentDir,
}

#[derive(Default)]
struct Scope {
selection: ScopeSelection,
excluded_packages: Vec<String>,
}

/// Which packages to analyze.
#[derive(Default, PartialEq, Eq)]
enum ScopeSelection {
/// Package to process (see `cargo help pkgid`)
Packages(Vec<String>),
Copy link
Owner

Choose a reason for hiding this comment

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

Can we define explicitly what happens if ScopeSelection::Packages contains a package that's also in excluded_packages? I'm not sure what should happen, or what cargo does assuming the same thing can happen in cargo. But our lib docs should state the expected behavior in any case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 1190770 I have made it impossible for this to happen

/// All packages in the workspace. Equivalent to `--workspace`.
Workspace,
/// Default members of the workspace.
#[default]
DefaultMembers,
}

impl Scope {
fn selected_packages<'m>(
obi1kenobi marked this conversation as resolved.
Show resolved Hide resolved
&self,
meta: &'m cargo_metadata::Metadata,
) -> Vec<&'m cargo_metadata::Package> {
let workspace_members: HashSet<_> = meta.workspace_members.iter().collect();
let base_ids: HashSet<_> = match &self.selection {
ScopeSelection::DefaultMembers => {
// Deviating from cargo because Metadata doesn't have default members
let resolve = meta.resolve.as_ref().expect("no-deps is unsupported");
match &resolve.root {
Some(root) => {
let mut base_ids = HashSet::new();
base_ids.insert(root);
base_ids
}
None => workspace_members,
}
}
ScopeSelection::Workspace => workspace_members,
ScopeSelection::Packages(patterns) => {
meta.packages
.iter()
// Deviating from cargo by not supporting patterns
// Deviating from cargo by only checking workspace members
.filter(|p| workspace_members.contains(&p.id) && patterns.contains(&p.name))
.map(|p| &p.id)
.collect()
}
};

meta.packages
.iter()
.filter(|p| base_ids.contains(&p.id) && !self.excluded_packages.contains(&p.name))
.collect()
}
}

impl Check {
pub fn new() -> Self {
Self::default()
}

pub fn with_manifest(&mut self, path: PathBuf) -> &mut Self {
self.current = Current::Manifest(path);
self
}
Copy link
Contributor Author

@MarcoIeni MarcoIeni Jan 21, 2023

Choose a reason for hiding this comment

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

I used a builder pattern that matches the cli args.
style of builder pattern: https://rust-lang.github.io/api-guidelines/type-safety.html#non-consuming-builders-preferred


pub fn with_workspace(&mut self) -> &mut Self {
self.scope.selection = ScopeSelection::Workspace;
self
}

pub fn with_packages(&mut self, packages: Vec<String>) -> &mut Self {
self.scope.selection = ScopeSelection::Packages(packages);
self
}

pub fn with_excluded_packages(&mut self, excluded_packages: Vec<String>) -> &mut Self {
self.scope.excluded_packages = excluded_packages;
self
}

pub fn with_current_rustdoc(&mut self, rustdoc: PathBuf) -> &mut Self {
self.current = Current::RustDoc(rustdoc);
self
}

pub fn with_baseline_version(&mut self, version: String) -> &mut Self {
self.baseline = Baseline::Version(version);
self
}

pub fn with_baseline_revision(&mut self, revision: String) -> &mut Self {
self.baseline = Baseline::Revision(revision);
self
}

pub fn with_baseline_root(&mut self, root: PathBuf) -> &mut Self {
self.baseline = Baseline::Root(root);
self
}

pub fn with_baseline_rustdoc(&mut self, rustdoc: PathBuf) -> &mut Self {
self.baseline = Baseline::RustDoc(rustdoc);
self
}

pub fn with_log_level(&mut self, log_level: log::Level) -> &mut Self {
self.log_level = Some(log_level);
self
}

fn manifest_path(&self) -> anyhow::Result<PathBuf> {
let path = match &self.current {
Current::Manifest(path) => path.clone(),
Current::RustDoc(_) => {
anyhow::bail!("error: RustDoc is not supported with these arguments.")
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this.

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed, it doesn't seem great. Perhaps the rustdoc-source restructuring might help?

Current::CurrentDir => PathBuf::from("Cargo.toml"),
};
Ok(path)
}

fn manifest_metadata(&self) -> anyhow::Result<cargo_metadata::Metadata> {
let mut command = cargo_metadata::MetadataCommand::new();
let metadata = command.manifest_path(self.manifest_path()?).exec()?;
Ok(metadata)
}

fn manifest_metadata_no_deps(&self) -> anyhow::Result<cargo_metadata::Metadata> {
let mut command = cargo_metadata::MetadataCommand::new();
let metadata = command
.manifest_path(self.manifest_path()?)
.no_deps()
.exec()?;
Ok(metadata)
}

pub fn check_release(&self) -> anyhow::Result<Report> {
let mut config = GlobalConfig::new().set_level(self.log_level);

let loader: Box<dyn baseline::BaselineLoader> = match &self.baseline {
Baseline::Version(version) => {
let mut registry = self.registry_baseline(&mut config)?;
let version = semver::Version::parse(&version)?;
registry.set_version(version);
Box::new(registry)
}
Baseline::Revision(rev) => {
let metadata = self.manifest_metadata_no_deps()?;
let source = metadata.workspace_root.as_std_path();
let slug = util::slugify(&rev);
let target = metadata
.target_directory
.as_std_path()
.join(util::SCOPE)
.join(format!("git-{slug}"));
Box::new(baseline::GitBaseline::with_rev(
source,
&target,
&rev,
&mut config,
)?)
}
Baseline::Root(root) => Box::new(baseline::PathBaseline::new(&root)?),
Baseline::RustDoc(path) => Box::new(baseline::RustdocBaseline::new(path.to_owned())),
Baseline::LatestVersion => {
let metadata = self.manifest_metadata_no_deps()?;
let target = metadata.target_directory.as_std_path().join(util::SCOPE);
let registry = baseline::RegistryBaseline::new(&target, &mut config)?;
Box::new(registry)
}
};
let rustdoc_cmd = dump::RustDocCommand::new()
.deps(false)
.silence(!config.is_verbose());

let rustdoc_paths = match &self.current {
Current::RustDoc(rustdoc_path) => {
let name = "<unknown>";
Copy link
Collaborator

Choose a reason for hiding this comment

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

While developing #207 I've found a bug: when both crates are generated from registry, this code tries to find a package named <unknown> in registry. From how the interface looked, I was convinced that it is enough to pass .with_packages(vec![name]) to Check, but now when I look at the lib.rs, I don't think it's what it's supposed to accept. So, in the scenario "from registry & from registry", there is no way to pass the name of the crate to check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In #207 I've made a temporary fix 7ecf587 so that I can progress further. In the case where this fix is good enough, I can move it here, otherwise we should discuss better alternatives.

Copy link
Owner

Choose a reason for hiding this comment

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

In 7ecf587 it seems like there's still an unknown branch. That looks like it could work if current is given by gitrev, but won't work with a registry version nor with a premade rustdoc file.

Can we add an explicit panic!() in the cases we know for sure won't work, instead of setting a bogus value and then breaking in a confusing way?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done in 7516ab0.

let version = None;
vec![(
name.to_owned(),
loader.load_rustdoc(&mut config, &rustdoc_cmd, name, version)?,
rustdoc_path.to_owned(),
)]
}
Current::CurrentDir | Current::Manifest(_) => {
let metadata = self.manifest_metadata()?;
let selected = self.scope.selected_packages(&metadata);
let mut rustdoc_paths = Vec::with_capacity(selected.len());
for selected in selected {
let manifest_path = selected.manifest_path.as_std_path();
let crate_name = &selected.name;
let version = &selected.version;

let is_implied = self.scope.selection == ScopeSelection::Workspace;
if is_implied && selected.publish == Some(vec![]) {
config.verbose(|config| {
config.shell_status(
"Skipping",
format_args!("{crate_name} v{version} (current)"),
)
})?;
continue;
}

config.shell_status(
"Parsing",
format_args!("{crate_name} v{version} (current)"),
)?;
let rustdoc_path = rustdoc_cmd.dump(manifest_path, None, true)?;
let baseline_path = loader.load_rustdoc(
&mut config,
&rustdoc_cmd,
crate_name,
Some(version),
)?;
rustdoc_paths.push((crate_name.clone(), baseline_path, rustdoc_path));
}
rustdoc_paths
}
};
let mut success = true;
for (crate_name, baseline_path, current_path) in rustdoc_paths {
let baseline_crate = load_rustdoc(&baseline_path)?;
let current_crate = load_rustdoc(&current_path)?;
tonowak marked this conversation as resolved.
Show resolved Hide resolved

if !run_check_release(&mut config, &crate_name, current_crate, baseline_crate)? {
success = false;
}
}

Ok(Report { success })
}

fn registry_baseline(
&self,
config: &mut GlobalConfig,
) -> Result<baseline::RegistryBaseline, anyhow::Error> {
let metadata = self.manifest_metadata_no_deps()?;
let target = metadata.target_directory.as_std_path().join(util::SCOPE);
let registry = baseline::RegistryBaseline::new(&target, config)?;
Ok(registry)
}
}

pub struct Report {
MarcoIeni marked this conversation as resolved.
Show resolved Hide resolved
success: bool,
}
MarcoIeni marked this conversation as resolved.
Show resolved Hide resolved

impl Report {
pub fn success(&self) -> bool {
self.success
}
}
Loading