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

feat: support --package to specify the package to profile #61

Merged
merged 5 commits into from
Apr 25, 2022

Conversation

tizee
Copy link
Contributor

@tizee tizee commented Apr 9, 2022

Solve #56 and #39 without breaking the architecture.

When implementing this feature, I found the method fn validate_target is redundant for validating the target. For Cargo builtin sub-commands, this is done by setting the field spec in CompileOptions. However, remove the method validate_target isn't enough.

The way Cargo used to construct a ComplieOptions is like following :

        let opts = CompileOptions {
            build_config,
            cli_features: self.cli_features()?,
            spec,
            filter: CompileFilter::from_raw_arguments(
                self.is_valid_and_present("lib"),
                self._values_of("bin"),
                self.is_valid_and_present("bins"),
                self._is_valid_arg("test")
                    .then(|| self._values_of("test"))
                    .unwrap_or_default(),
                self.is_valid_and_present("tests"),
                self._values_of("example"),
                self.is_valid_and_present("examples"),
                self._is_valid_arg("bench")
                    .then(|| self._values_of("bench"))
                    .unwrap_or_default(),
                self.is_valid_and_present("benches"),
                self.is_valid_and_present("all-targets"),
            ),
            target_rustdoc_args: None,
            target_rustc_args: None,
            target_rustc_crate_types: None,
            local_rustdoc_args: None,
            rustdoc_document_private_items: false,
            honor_rust_version: !self.is_valid_and_present("ignore-rust-version"),
        };

In implementation of CompileOptions::new , spec: ops::Packages::Packages(Vec::new()), makes the field spec an empty vector, which cause Cargo fail to find bin, example, bench for a virtual manifest. Unless we set the field spec explicitly to Packages::Default to tell Cargo to search the whole workspace. For the sake of time, I didn't spend too much time digging in the internal logic.

    pub fn new(config: &Config, mode: CompileMode) -> CargoResult<CompileOptins> {
        let jobs = None;
        let keep_going = false;
        Ok(CompileOptions {
            build_config: BuildConfig::new(config, jobs, keep_going, &[], mode)?,
            cli_features: CliFeatures::new_all(false),
            spec: ops::Packages::Packages(Vec::new()),
            filter: CompileFilter::Default {
                required_features_filterable: false,
            },
            target_rustdoc_args: None,
            target_rustc_args: None,
            target_rustc_crate_types: None,
            local_rustdoc_args: None,
            rustdoc_document_private_items: false,
            honor_rust_version: true,
        })
    }

I implement the feature by introducing a Enum type Package that corresponds to Cargo's Packages::Default and Packages::Packages(Vec<String>) respectively.

I do not insist on this and will gladly to use other approach i.e. if there is a huge refactor on architecture. Right now I'm using this patch for projects having more than one package.

Copy link
Owner

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Thanks!

I'm not sure exactly how cargo Packages works, but playing with this patch it seems like a pretty significant improvement.

In particular, running this in a workspace where a given target name exists in only one package, it is able to find that target without me needing to specify --package at all.

Is the --package argument only necessary if there are multiple packages that contain a given target name? I believe this is the case, in which case we should clarify that in the documentation. :)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/opt.rs Outdated Show resolved Hide resolved
src/opt.rs Outdated Show resolved Hide resolved
tizee and others added 4 commits April 13, 2022 23:38
Copy link
Owner

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Great, thank you for this contribution, it is much appreciated!

I'm going to merge this as-is, and then I'll do a separate PR myself that clears up some documentation and gets ready for a release. :)

@cmyr cmyr merged commit db39b61 into cmyr:master Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants