-
Notifications
You must be signed in to change notification settings - Fork 225
bindeps failing build fix #3111
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
base: main
Are you sure you want to change the base?
Changes from all commits
1ceb54c
4d7e80e
71bc093
b67da7f
8154a5e
124aaba
87bd166
bb17e6c
2c0c3a5
aaa0156
126571c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| [package] | ||
syphar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| name = "bindeps-test" | ||
| version = "0.1.0" | ||
| edition = "2021" | ||
|
|
||
| [package.metadata.docs.rs] | ||
| cargo-args = ["-Zbindeps"] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More a question, because I don't know: Would this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hi again! thanks for your kind words :) i rebased the branch and removed artifacts from my builds and tests, so the PR now only contains the intended files. about your question on i also fixed the new audit failure by updating
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is the last piece missing for me :) Thank you for working on this! One thing I'm still not sure about (might be lack of knowledge) is if this test crate would really fail to build without this PR? Wouldn't a cleaner example be one where we actually artifact dependencies? like the one mentioned in #2710?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done, thanks for the suggestion. changed |
||
|
|
||
| [build-dependencies] | ||
| # This requires `-Z bindeps` to be accepted by cargo metadata/fetch. | ||
| bindeps-helper = { path = "bindeps-helper", artifact = "bin" } | ||
|
|
||
| [lib] | ||
| name = "bindeps_test" | ||
| path = "src/lib.rs" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| [package] | ||
| name = "bindeps-helper" | ||
| version = "0.1.0" | ||
| edition = "2021" | ||
|
|
||
| [[bin]] | ||
| name = "bindeps-helper" | ||
| path = "src/main.rs" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| fn main() {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| //! Test crate for bindeps support | ||
| //! | ||
| //! This crate uses unstable cargo feature `bindeps` (artifact dependencies). | ||
| //! It should build on docs.rs when the fix for #2710 is applied. | ||
|
|
||
| pub fn hello() -> &'static str { | ||
| "Hello from bindeps-test!" | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI failed cause new artifact-based test hit rustwide local manifest validation before our build path.
i changed the test to check
cargo metadataflag forwarding directly - fails without-Zbindeps, passes with it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thing I would like to keep a full build test to be sure the whole feature keeps working.
Which part fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fails in rustwide’s local crate manifest validation step (before the docs.rs build path): rustwide::prepare::validate_manifest runs cargo metadata --manifest-path Cargo.toml --no-deps without -Zbindeps, so Cargo rejects artifact = "...".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully follow.
I can only see that
build_local_packagecallsload_metadata_from_rustwide, which you both adapt in this PR?So from what I see, any local crate manifest validation path would also be called in the "normal" build path?
It's totally possible I'm just missing context or details, but since I'll need to maintain it, I want to fully understand :)
Can you push a commit where I can see the failing test how you tried it? That would help .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally I'm super happy this can make progress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried to build the crate from the issue using your branch, using the "normal" docs.rs build, not test.
This looks like the failing test that you saw was a sign that this feature wouldn't have worked at all for builds.
Did you manually run a build to test it?
In any case: I believe you should re-add the test, and then we can figure out what is necessary to fix it.
Perhaps even a change to rustwide?