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

feat: add cargo pkgid support for cargo-script #14961

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

feat: add cargo pkgid support for cargo-script #14961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5465e6a
fix: remove unsupported embedded workspace check from `cargo pkgid` c…
Rustin170506 Dec 17, 2024
feda97d
test: add the no lock file case
Rustin170506 Dec 19, 2024
0c2bb4e
test: add the dep case
Rustin170506 Dec 19, 2024
d4c1eee
refactor: use manifest path as the key
Rustin170506 Jan 20, 2025
95f6d0b
feat: use full URL for embedded script paths
Rustin170506 Jan 9, 2025
1514c7e
test: add test for script as a dependency
Rustin170506 Jan 9, 2025
1d960c7
fix: better error message for using the script as a dep
Rustin170506 Jan 13, 2025
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
7 changes: 0 additions & 7 deletions src/bin/cargo/commands/pkgid.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,6 @@ pub fn cli() -> Command {

pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult {
let ws = args.workspace(gctx)?;
if ws.root_maybe().is_embedded() {
return Err(anyhow::format_err!(
"{} is unsupported by `cargo pkgid`",
ws.root_manifest().display()
)
.into());
}
if args.is_present_with_zero_values("package") {
print_available_packages(&ws)?
}
Expand Down
3 changes: 3 additions & 0 deletions src/cargo/core/source_id.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,9 @@ impl SourceId {
.url
.to_file_path()
.expect("path sources cannot be remote");
if crate::util::toml::is_embedded(&path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I'm not seeing a test for cargo install --path foo.rs which I think would fail with this

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I think we only support the directory path now. So it wouldn't reach here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add a test case for it.

anyhow::bail!("The embedded script cannot be used as a dependency");
Copy link
Contributor

Choose a reason for hiding this comment

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

"embedded script" isn't quite the right term to use for this.

We refer to the manifest as "embedded" because it is encapsulated within the script. We have unofficially been referring to the files as scripts but I'm not fully satisfied with that term. In the RFC, I also called these "single file packages".

}
Ok(Box::new(PathSource::new(&path, self, gctx)))
}
SourceKind::Registry | SourceKind::SparseRegistry => Ok(Box::new(
Expand Down
25 changes: 12 additions & 13 deletions src/cargo/core/workspace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,10 +269,11 @@ impl<'gctx> Workspace<'gctx> {
let mut ws = Workspace::new_default(package.manifest_path().to_path_buf(), gctx);
ws.is_ephemeral = true;
ws.require_optional_deps = require_optional_deps;
let key = ws.current_manifest.parent().unwrap();
let id = package.package_id();
let package = MaybePackage::Package(package);
ws.packages.packages.insert(key.to_path_buf(), package);
ws.packages
.packages
.insert(ws.current_manifest.clone(), package);
ws.target_dir = if let Some(dir) = target_dir {
Rustin170506 marked this conversation as resolved.
Show resolved Hide resolved
Some(dir)
} else {
Expand Down Expand Up @@ -538,11 +539,7 @@ impl<'gctx> Workspace<'gctx> {
/// Returns a mutable iterator over all packages in this workspace
pub fn members_mut(&mut self) -> impl Iterator<Item = &mut Package> {
let packages = &mut self.packages.packages;
let members: HashSet<_> = self
.members
.iter()
.map(|path| path.parent().unwrap().to_owned())
.collect();
let members: HashSet<_> = self.members.iter().map(|path| path).collect();

packages.iter_mut().filter_map(move |(path, package)| {
if members.contains(path) {
Expand Down Expand Up @@ -1159,7 +1156,6 @@ impl<'gctx> Workspace<'gctx> {

pub fn emit_warnings(&self) -> CargoResult<()> {
for (path, maybe_pkg) in &self.packages.packages {
let path = path.join("Cargo.toml");
if let MaybePackage::Package(pkg) = maybe_pkg {
if self.gctx.cli_unstable().cargo_lints {
self.emit_lints(pkg, &path)?
Expand Down Expand Up @@ -1788,19 +1784,22 @@ impl<'gctx> Packages<'gctx> {
}

fn maybe_get(&self, manifest_path: &Path) -> Option<&MaybePackage> {
self.packages.get(manifest_path.parent().unwrap())
self.packages.get(manifest_path)
}

fn maybe_get_mut(&mut self, manifest_path: &Path) -> Option<&mut MaybePackage> {
self.packages.get_mut(manifest_path.parent().unwrap())
self.packages.get_mut(manifest_path)
}

fn load(&mut self, manifest_path: &Path) -> CargoResult<&MaybePackage> {
let key = manifest_path.parent().unwrap();
match self.packages.entry(key.to_path_buf()) {
match self.packages.entry(manifest_path.to_path_buf()) {
Entry::Occupied(e) => Ok(e.into_mut()),
Entry::Vacant(v) => {
let source_id = SourceId::for_path(key)?;
let source_id = if crate::util::toml::is_embedded(manifest_path) {
SourceId::for_path(manifest_path)?
} else {
SourceId::for_path(manifest_path.parent().unwrap())?
};
let manifest = read_manifest(manifest_path, source_id, self.gctx)?;
Ok(v.insert(match manifest {
EitherManifest::Real(manifest) => {
Expand Down
4 changes: 2 additions & 2 deletions tests/testsuite/alt_registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,8 @@ fn depend_on_alt_registry_depends_on_crates_io() {
[DOWNLOADED] bar v0.0.1 (registry `alternative`)
[CHECKING] baz v0.0.1
[CHECKING] bar v0.0.1 (registry `alternative`)
[CHECKING] foo v0.0.1 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[CHECKING] foo v0.0.1 ([ROOT]/foo)

"#]]
.unordered(),
Expand Down Expand Up @@ -441,8 +441,8 @@ fn alt_registry_and_crates_io_deps() {
[DOWNLOADED] alt_reg_dep v0.1.0 (registry `alternative`)
[CHECKING] crates_io_dep v0.0.1
[CHECKING] alt_reg_dep v0.1.0 (registry `alternative`)
[CHECKING] foo v0.0.1 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[CHECKING] foo v0.0.1 ([ROOT]/foo)

"#]]
.unordered(),
Expand Down
10 changes: 5 additions & 5 deletions tests/testsuite/artifact_dep.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,9 +602,9 @@ fn build_script_with_bin_artifacts() {
.with_stderr_data(
str![[r#"
[LOCKING] 1 package to latest compatible version
[COMPILING] foo v0.0.0 ([ROOT]/foo)
[COMPILING] bar v0.5.0 ([ROOT]/foo/bar)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[COMPILING] foo v0.0.0 ([ROOT]/foo)

"#]]
.unordered(),
Expand Down Expand Up @@ -1433,7 +1433,6 @@ fn profile_override_basic() {
str![[r#"
[LOCKING] 1 package to latest compatible version
[COMPILING] bar v0.5.0 ([ROOT]/foo/bar)
[COMPILING] foo v0.0.1 ([ROOT]/foo)
[RUNNING] `rustc --crate-name build_script_build [..] -C opt-level=1 [..]`
[RUNNING] `rustc --crate-name bar --edition=2015 bar/src/main.rs [..] -C opt-level=3 [..]`
[RUNNING] `rustc --crate-name bar --edition=2015 bar/src/main.rs [..] -C opt-level=1 [..]`
Expand All @@ -1442,6 +1441,7 @@ fn profile_override_basic() {
[RUNNING] `rustc --crate-name foo [..] -C opt-level=3 [..]`
[RUNNING] `[ROOT]/foo/target/debug/build/foo-[HASH]/build-script-build`
[FINISHED] `dev` profile [optimized + debuginfo] target(s) in [ELAPSED]s
[COMPILING] foo v0.0.1 ([ROOT]/foo)

"#]]
.unordered(),
Expand Down Expand Up @@ -1870,8 +1870,8 @@ fn allow_dep_renames_with_multiple_versions() {
[DOWNLOADED] bar v1.0.0 (registry `dummy-registry`)
[COMPILING] bar v1.0.0
[COMPILING] bar v0.5.0 ([ROOT]/foo/bar)
[COMPILING] foo v0.0.0 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[COMPILING] foo v0.0.0 ([ROOT]/foo)

"#]]
.unordered(),
Expand Down Expand Up @@ -2835,13 +2835,13 @@ fn with_assumed_host_target_and_optional_build_dep() {
.with_stderr_data(
str![[r#"
[LOCKING] 1 package to latest compatible version
[COMPILING] foo v0.0.1 ([ROOT]/foo)
[COMPILING] d1 v0.0.1 ([ROOT]/foo/d1)
[RUNNING] `rustc --crate-name build_script_build --edition=2021 [..]--crate-type bin[..]
[RUNNING] `rustc --crate-name d1 --edition=2021 [..]--crate-type bin[..]
[RUNNING] `[ROOT]/foo/target/debug/build/foo-[HASH]/build-script-build`
[RUNNING] `rustc --crate-name foo --edition=2021 [..]--cfg[..]d1[..]
[FINISHED] `dev` profile [..]
[COMPILING] foo v0.0.1 ([ROOT]/foo)

"#]]
.unordered(),
Expand Down Expand Up @@ -3199,8 +3199,8 @@ fn decouple_same_target_transitive_dep_from_artifact_dep_and_proc_macro() {
[COMPILING] b v0.1.0 ([ROOT]/foo/b)
[COMPILING] c v0.1.0 ([ROOT]/foo/c)
[COMPILING] bar v0.1.0 ([ROOT]/foo/bar)
[COMPILING] foo v0.1.0 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]
[COMPILING] foo v0.1.0 ([ROOT]/foo)

"#]]
.unordered(),
Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/bad_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1814,13 +1814,13 @@ fn workspace_default_features2() {
p.cargo("check")
.with_stderr_data(
str![[r#"
[WARNING] [ROOT]/foo/workspace_only/Cargo.toml: `default_features` is deprecated in favor of `default-features` and will not work in the 2024 edition
(in the `dep_workspace_only` dependency)
[CHECKING] dep_package_only v0.1.0 ([ROOT]/foo/dep_package_only)
[CHECKING] dep_workspace_only v0.1.0 ([ROOT]/foo/dep_workspace_only)
[CHECKING] package_only v0.1.0 ([ROOT]/foo/package_only)
[CHECKING] workspace_only v0.1.0 ([ROOT]/foo/workspace_only)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[WARNING] [ROOT]/foo/workspace_only/Cargo.toml: `default_features` is deprecated in favor of `default-features` and will not work in the 2024 edition

"#]]
.unordered(),
Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/fix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2479,7 +2479,7 @@ fn main() {
.with_stderr_data(str![[r#"
[MIGRATING] foo.rs from 2021 edition to 2024
[FIXED] foo.rs (1 fix)
[CHECKING] foo v0.0.0 ([ROOT]/foo)
[CHECKING] foo v0.0.0 ([ROOT]/foo/foo.rs)
[MIGRATING] [ROOT]/home/.cargo/target/[HASH]/foo.rs from 2021 edition to 2024
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/open_namespaces.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ fn main() {}
"edition": "2021",
"features": {},
"homepage": null,
"id": "path+[ROOTURL]/foo#foo::[email protected]",
"id": "path+[ROOTURL]/foo/foo::bar.rs#foo::[email protected]",
"keywords": [],
"license": null,
"license_file": null,
Expand Down
Loading
Loading