Skip to content

Follow up for: Use the umbrella crate for the parachain template #5993#7464

Merged
serban300 merged 56 commits intomasterfrom
seemant-parachain-umbrella-crate-mig
Mar 17, 2025
Merged

Follow up for: Use the umbrella crate for the parachain template #5993#7464
serban300 merged 56 commits intomasterfrom
seemant-parachain-umbrella-crate-mig

Conversation

@seemantaggarwal
Copy link
Copy Markdown
Contributor

@seemantaggarwal seemantaggarwal commented Feb 5, 2025

Resolving sc-tracing not being resolved when imported through the polkadot sdk

@seemantaggarwal seemantaggarwal changed the title removing unused imports Follow up for: Use the umbrella crate for the parachain template #5993 Feb 5, 2025
@serban300 serban300 added the R0-no-crate-publish-required The change does not require any crates to be re-published. label Feb 6, 2025
serban300
serban300 previously approved these changes Feb 6, 2025
@serban300 serban300 dismissed their stale review February 6, 2025 11:31

Will take another look when the PR is ready for review

@iulianbarbu
Copy link
Copy Markdown
Contributor

iulianbarbu commented Feb 12, 2025

The node crate still fails to build because an error is raised here during the compilation, when the macro is expanded: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/client/tracing/proc-macro/src/lib.rs#L123. crate_name function searches through the node's Cargo.toml for the sc-tracing dependency, or for the renamed version of the same crate (https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#renaming-dependencies-in-cargotoml), but it can not find it, since we rely on polkadot-sdk with its node feature, which pulls the sc-tracing crate and re-exports it, and the compilation fails as a result.

I would attempt a fix in sc-tracing-proc-macro crate, maybe based on cargo-metadata (https://docs.rs/cargo_metadata/latest/cargo_metadata/), where we parse through the dependency graph. Not sure how easy it is, but I would check for our current crate dependencies, search for polkadot-sdk, and then iterate through its dependencies (and here check if sc-tracing exists ). I am not sure if sc-tracing will show up no matter what polkadot-sdk features we enable, because if this is the case, then we might report sc-tracing can be used in the crate that uses polkadot-sdk, but in practice this would fail, because no enabled features pulls it in. However, this is already better since for the cases where sc-tracing is pulled in because of an enabled feature that pulls it in, the compilation wouldn't fail (assuming use polkadot-sdk::* is used in the file where sc_tracing is used).

@seemantaggarwal
Copy link
Copy Markdown
Contributor Author

Maybe I can try it out instead, we can discuss of what and how, that way, we try it out, and I learn something, 2 birds one stone ;)

@iulianbarbu
Copy link
Copy Markdown
Contributor

iulianbarbu commented Feb 13, 2025

Just posting the code change and the file. It successfully builds now.

	let crate_name = match crate_name("sc-tracing") {
		Ok(FoundCrate::Itself) => Ident::new("sc_tracing", Span::call_site()),
		Ok(FoundCrate::Name(crate_name)) => Ident::new(&crate_name, Span::call_site()),
		Err(_) => match crate_name("polkadot-sdk") {
			Ok(FoundCrate::Itself) | Ok(FoundCrate::Name(_)) =>
				Ident::new("sc_tracing", Span::call_site()),
			Err(_) => return Error::new(Span::call_site(), "Neither one of `sc-tracing` or `polkadot-sdk` hasn't been found in your dependencies, or no `polkadot-sdk`'s enabled feature pulls in `sc-tracing`.").to_compile_error().into(),
		},
	};

Copy link
Copy Markdown
Contributor

@serban300 serban300 left a comment

Choose a reason for hiding this comment

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

Just left a nit. Otherwise looks good

/// Resolve the correct path for sc_tracing:
/// - If `polkadot-sdk` is in scope, returns a Path corresponding to `polkadot_sdk::sc_tracing`
/// - Otherwise, falls back to `sc_tracing`
pub fn resolve_sc_tracing() -> Result<Path> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we need to put this logic in a separate function and file if we don't need to reuse it.

@paritytech-workflow-stopper
Copy link
Copy Markdown

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13875873988
Failed job name: fmt

@seemantaggarwal
Copy link
Copy Markdown
Contributor Author

/cmd fmt

Copy link
Copy Markdown
Contributor

@serban300 serban300 left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Copy Markdown
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Copy Markdown
Contributor

@iulianbarbu iulianbarbu left a comment

Choose a reason for hiding this comment

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

LGTM!

Co-authored-by: Sebastian Kunert <skunert49@gmail.com>
@serban300 serban300 enabled auto-merge March 17, 2025 11:58
@serban300 serban300 added this pull request to the merge queue Mar 17, 2025
Merged via the queue into master with commit 5f44a77 Mar 17, 2025
235 of 243 checks passed
@serban300 serban300 deleted the seemant-parachain-umbrella-crate-mig branch March 17, 2025 13:10
ordian added a commit that referenced this pull request Mar 19, 2025
* master: (58 commits)
  Upgrade link-checker cache to v4 (#7874)
  Updating readmes (#7950)
  Cumulus: Remove some old scripts (#7946)
  pallet-bounties: allow bounties to never expire (#7723)
  run frame-omni-bencher overhead command in CI for all runtimes in the runtime matrix (#7459)
  Update README.md for Cumulus (#7930)
  FRAME: Meta Transaction (#6428)
  Follow up for: Use the umbrella crate for the parachain template #5993 (#7464)
  Add an extra_constant to pallet-treasury (#7918)
  Bump the ci_dependencies group across 1 directory with 4 updates (#7855)
  remove compromised action (#7934)
  Fixing token-economics dead link (#5302)
  [pallet-revive] Fix pallet-revive-fixtures build.rs (#7928)
  cumulus: fix pov exporter format (#7923)
  sp-api: Support `mut` in `impl_runtime_apis!` (#7924)
  Remove clones from block seal function (#7917)
  [pallet-revive] precompiles 2->9 (#7810)
  Use non-native token to benchmark xcm on asset hub (#7893)
  [CI] bump timeout wait for build in zombienet workflows. (#7871)
  taplo: split long array line to multiline array (#7905)
  ...
alindima pushed a commit that referenced this pull request Apr 8, 2025
#7464)

Resolving sc-tracing not being resolved when imported through the
polkadot sdk

---------

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Serban Iorga <serban@parity.io>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Iulian Barbu <iulian.barbu@parity.io>
Co-authored-by: Sebastian Kunert <skunert49@gmail.com>
github-merge-queue bot pushed a commit that referenced this pull request Apr 17, 2025
# Description

We made changes to `sc-tracing-proc-macro` in #7464 which weren't
carried to the prdoc, so once stable2503 got released there wasn't a
bump for `sc-tracing-proc-macro`, and now it is needed for updating
`parachain-template` repo to 2503. It seems that the prdoc check did not
catch the missing required bump (it might be because the change was in a
`proc-macro` crate).

## Integration

N/A

## Review Notes

Planning to update stable2503 prdocs retroactively (so the change itself
doesn't count for next patch release), and we should request either way
a force bump for the following crates:

1. `sc-tracing-proc-macro`
2. `sc-tracing` (depends on 1.)
3. `polkadot-sdk` (depends on 1. & 2.)

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
paritytech-release-backport-bot bot pushed a commit that referenced this pull request Apr 18, 2025
# Description

We made changes to `sc-tracing-proc-macro` in #7464 which weren't
carried to the prdoc, so once stable2503 got released there wasn't a
bump for `sc-tracing-proc-macro`, and now it is needed for updating
`parachain-template` repo to 2503. It seems that the prdoc check did not
catch the missing required bump (it might be because the change was in a
`proc-macro` crate).

## Integration

N/A

## Review Notes

Planning to update stable2503 prdocs retroactively (so the change itself
doesn't count for next patch release), and we should request either way
a force bump for the following crates:

1. `sc-tracing-proc-macro`
2. `sc-tracing` (depends on 1.)
3. `polkadot-sdk` (depends on 1. & 2.)

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
(cherry picked from commit ade1b9c)
Krayt78 pushed a commit to Krayt78/polkadot-sdk that referenced this pull request Apr 18, 2025
…ch#8276)

# Description

We made changes to `sc-tracing-proc-macro` in paritytech#7464 which weren't
carried to the prdoc, so once stable2503 got released there wasn't a
bump for `sc-tracing-proc-macro`, and now it is needed for updating
`parachain-template` repo to 2503. It seems that the prdoc check did not
catch the missing required bump (it might be because the change was in a
`proc-macro` crate).

## Integration

N/A

## Review Notes

Planning to update stable2503 prdocs retroactively (so the change itself
doesn't count for next patch release), and we should request either way
a force bump for the following crates:

1. `sc-tracing-proc-macro`
2. `sc-tracing` (depends on 1.)
3. `polkadot-sdk` (depends on 1. & 2.)

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
castillax pushed a commit that referenced this pull request May 12, 2025
# Description

We made changes to `sc-tracing-proc-macro` in #7464 which weren't
carried to the prdoc, so once stable2503 got released there wasn't a
bump for `sc-tracing-proc-macro`, and now it is needed for updating
`parachain-template` repo to 2503. It seems that the prdoc check did not
catch the missing required bump (it might be because the change was in a
`proc-macro` crate).

## Integration

N/A

## Review Notes

Planning to update stable2503 prdocs retroactively (so the change itself
doesn't count for next patch release), and we should request either way
a force bump for the following crates:

1. `sc-tracing-proc-macro`
2. `sc-tracing` (depends on 1.)
3. `polkadot-sdk` (depends on 1. & 2.)

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

R0-no-crate-publish-required The change does not require any crates to be re-published. T17-Templates This PR/Issue is related to templates

Projects

Status: Milestone 1

Development

Successfully merging this pull request may close these issues.

7 participants