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

Attemp to fix Error: no dep #43

Merged
merged 7 commits into from
Dec 12, 2024
Merged

Attemp to fix Error: no dep #43

merged 7 commits into from
Dec 12, 2024

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Dec 9, 2024

Closes: #42

This PR fixes two things:

  • Error: no dep
  • small optimization for for (name, _) in deps { - it iterates over and over the same stuff unnecessary

Error: https://github.com/paritytech/polkadot-sdk/actions/runs/12237889294/job/34134815479?pr=6781

PR Doc validation is best effort
We can only detect the minimum guaranteed semver change
It's possible to not detect changes or for changes to be greater than detected
Always reason about semver changes yourself

validating prdocs...
checking file changes...
checking dep changes...
Error: no dep

Stack backtrace:
   0: parity_publish::prdoc::get_dep
   1: parity_publish::prdoc::manifest_deps_changed
   2: parity_publish::prdoc::handle_prdoc
   3: parity_publish::main::{{closure}}
   4: tokio::runtime::park::CachedParkThread::block_on
   5: tokio::runtime::context::runtime::enter_runtime
   6: tokio::runtime::runtime::Runtime::block_on
   7: parity_publish::main
   8: std::sys::backtrace::__rust_begin_short_backtrace
   9: std::rt::lang_start::{{closure}}
  10: std::rt::lang_start_internal
  11: main
  12: __libc_start_main
  13: _start

After some debugging, looks like that job is failing on added sp-core:
image

@bkontur bkontur added bug Something isn't working enhancement New feature or request labels Dec 9, 2024
@bkontur bkontur requested a review from Morganamilo December 9, 2024 15:54
Copy link
Collaborator

@Morganamilo Morganamilo left a comment

Choose a reason for hiding this comment

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

Assuming this has been tested to fix the CI issue the core change seems fine for now to get a fix in. I'll need to double check the root cause in future so we don't possibly just skip renamed dependencies.

src/prdoc.rs Outdated Show resolved Hide resolved
src/prdoc.rs Outdated
continue;
}
(Err(_), Err(_)) => {
// strange, not found in both?
Copy link
Collaborator

Choose a reason for hiding this comment

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

May be an issue with dep renames not properly handled? E.g. foo = {version = "1", package = "bar"}. I'll need to look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And what exactly is "dep renames not properly handled"? How should it be done correctly?

Ok, I change it here to throw error with exact message: 8eb07d5

So now, it won't fail in most cases when adding or removing dependencies, but it will fail in this specific corner 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.

aha, ok :), so continue/skip (I can put just some log there) or fail here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Originally posted on the wrong account)

Sorry for the confusion, I was just commenting on the comment. Dependency renames are when the dependency has a package = and this exists under different names to the registry and local dependency tree. I'm just noting it as a possible issue that I'll look into in the future. It's not a user error,

aha, ok :), so continue/skip (I can put just some log there) or fail here?

I think skipping is fine for now if it fixes the CI. I'll look into it further when I get the chance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And no log as the message implies there's user action to take when there's not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I reverted back continue here: f482052

and created an issue for you :) #44

src/prdoc.rs Outdated Show resolved Hide resolved
@iulianbarbu
Copy link
Contributor

iulianbarbu commented Dec 11, 2024

The fix is helping in the sense that it fixes the Error: no dep locally on the PR branch where the previous 0.10.2 failed for me: paritytech/polkadot-sdk#6450.

It fails though with: Error: Failed to build rustdoc JSON (see stderr), and nothing shows up in stderr (which is linked to my terminal). I think this is something specific to my branch though, will have to check more.
Made it work by following exactly the merge strategy done in the check-semver job.

Out of curiosity @bkontur , did you find a workaround for the PR that failed initially with Error: no dep? Otherwise, I think we need to wait until this PR merging is decided, and then a new parity-publish version released and used in the polkadot-sdk CI.

@bkontur
Copy link
Contributor Author

bkontur commented Dec 11, 2024

Out of curiosity @bkontur , did you find a workaround for the PR that failed initially with Error: no dep? Otherwise, I think we need to wait until this PR merging is decided, and then a new parity-publish version released and used in the polkadot-sdk CI.

I think that failed CI semver does not block merging, so, no, I was not looking for any workaround, just this fix :)

@iulianbarbu
Copy link
Contributor

Out of curiosity @bkontur , did you find a workaround for the PR that failed initially with Error: no dep? Otherwise, I think we need to wait until this PR merging is decided, and then a new parity-publish version released and used in the polkadot-sdk CI.

I think that failed CI semver does not block merging, so, no, I was not looking for any workaround, just this fix :)

Yup, as long as you ran same thing ran in check-semver locally and did the necessary changes if any, then all good.

@Morganamilo Morganamilo merged commit b8fcf53 into main Dec 12, 2024
1 check passed
@bkontur bkontur deleted the bko-fix branch December 12, 2024 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when validating prdoc in a PR that introduce a new crate
3 participants