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
+154 −65
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
183eeff
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
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
3 changes: 3 additions & 0 deletions src/cargo/sources/path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ impl<'gctx> PathSource<'gctx> {
}

fn read_package(&self) -> CargoResult<Package> {
if crate::util::toml::is_embedded(&self.path) {
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 am not sure if this is the best place to check this. Maybe we should move it to where we create the source ID?

Copy link
Contributor

Choose a reason for hiding this comment

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

imo this feels too spooky as this function is not directly tied to dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then I guess we should do the check when we read the manifest file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or the dependency resolution phase. Maybe do it when resolving dependencies makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it isn't so bad? If we view the lack of dependency / workspace support as not far away, this just needs to hold us over until then. If its far away or unlikely, then more care might be needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we view the lack of dependency / workspace support as not far away, this just needs to hold us over until then.

Do you have any plans for these features? They sound like big features. So I would prefer to find a better place for it. At least we could move it to the earlier stage, for example when we build the source for this package?

Copy link
Contributor

Choose a reason for hiding this comment

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

Dependency support is dependent on either

  • [lib] support which is hazy
  • artifact dependencies which I'd really like to move forward for build script delegation

Workspace support could be really useful for cargo-xtasks

These were deferred out of the first RFC just to focus on the minimal, core feature set. I am not lined up to immediately follow with these features but I could see them being high demand when this gets stabilized.

anyhow::bail!("The embedded script cannot be used as a dependency");
}
let path = self.path.join("Cargo.toml");
let pkg = ops::read_package(&path, self.source_id, self.gctx)?;
Ok(pkg)
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