Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Use relative docify paths #14672

Merged
merged 2 commits into from
Aug 5, 2023
Merged

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Jul 28, 2023

Bumping the docify version and using relative paths is needed for the monorepo to find the test files in the new workspace.

Bumping the docify version and using relative paths is needed for
the monorepo to find the test files in the new workspace.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jul 28, 2023
@ggwpez ggwpez requested review from sam0x17 and a team July 28, 2023 10:35
@ggwpez
Copy link
Member Author

ggwpez commented Jul 28, 2023

@sam0x17 could you please check the Ci output here? I dont get why it tries to search for frame/paged-list/fuzzer/src/tests.rs instead of frame/paged-list/src/tests.rs.
Running the doc command locally on both crates works fine...

PS: It reproduces with SKIP_WASM_BUILD=1 cargo check --locked --benches --all.
PPS: It gets weirder since on my server it reproduced with cargo check -p pallet-paged-list but not locally... although using the same rust version.

@sam0x17
Copy link
Contributor

sam0x17 commented Jul 31, 2023

@sam0x17 could you please check the Ci output here? I dont get why it tries to search for frame/paged-list/fuzzer/src/tests.rs instead of frame/paged-list/src/tests.rs. Running the doc command locally on both crates works fine...

PS: It reproduces with SKIP_WASM_BUILD=1 cargo check --locked --benches --all. PPS: It gets weirder since on my server it reproduced with cargo check -p pallet-paged-list but not locally... although using the same rust version.

So the weird thing here is we are embedding stuff from paged-list/fuzzer/src/tests.rs in paged-list/src/lib.rs (this is fine), however the docify::embed! just says docify::embed!("src/tests.rs") whereas I assume it should be docify::embed!("fuzzer/src/tests.rs"), since paths are crate-relative from the perspective of the embed! macro

TLDR: paths for embed!/embed_run! should be crate-relative from the perspective of embed!/embed_run!

@sam0x17
Copy link
Contributor

sam0x17 commented Jul 31, 2023

ok so looking into this further, no idea why it is trying to look in fuzzer, so will get to the bottom of that

@sam0x17
Copy link
Contributor

sam0x17 commented Jul 31, 2023

update: so the main barrier here is that because docify injects within a #[doc = ..] attribute, we have no ability to use things like #[cfg(doc)] to only look for docify embed files when docs are building. If we could somehow limit this to only happen when we are #[cfg(doc)], most issues like this would likely vanish.

So the first natural thing to try would be "ok, what if we just make it an attribute" so like:

/// something
#[docify::embed("some/file.rs")
/// something else

The problem with this is attributes actually cannot discover what their index is within an items underlying list of attributes, since the internal proc_macro system removes this information before tokens are passed to the proc macro attribute implementation. So because of this, we don't know how to position our attribute's doc comment among the other doc comments, so the ordering is lost. This would be fine if we controlled every attribute since we could use a convention like always appending to the end, but we do not control regular doc comments (which internally are just #[doc = ..] and so this information is lost). The .. only accepts a string literal which is why we have so little manuverability here, btw.

So Yandros (@danielhenrymantilla), who is a macro wizard and rustc maintainer, suggested what I think is a much better syntax and one that would also probably solve this issue:

#[docify::docify]
/// some comments some comments
/// @embed("examples/samples.rs", MyCoolStruct)
/// another example:
/// @embed("src/something.rs", MyThing)
/// some closing comments
pub struct Foo;

This is the moral equivalent of switching to an outer macro pattern, but for attributes. The #[docify::docify] attribute would be able to expand to different versions of #item with #[cfg(doc)] and #[cfg(not(doc))] respectively, and only for the #[cfg(doc)] version would it try to expand the @embed commands.

Now we could just match exactly what the markdown parsing already does which is to use the rather verbose:

# this is markdown
<!-- docify::embed!("something.rs", Something) -->

However the @embed syntax would be very simple to implement and is easier to write, and that was what Yandros suggested so I have preserved it here.

So I may experiment with implementing this syntax if other debugging options don't yield anything.

All of that said, @ggwpez provided me with some additional debugging information and I think I have enough info to try to directly fix this (false positive on caller_crate_root() thinking the fuzzer dir is the crate root even though it doesn't match CARGO_MANIFEST_DIR).

So going to try to directly fix this first

@sam0x17
Copy link
Contributor

sam0x17 commented Aug 4, 2023

Going to try with a tweaked version of docify now

@sam0x17
Copy link
Contributor

sam0x17 commented Aug 4, 2023

OK I've identified the issue. It's actually quite simple.

The code that looks for the crate_root looks for Cargo.toml files where name = "{{crate_name}}" is in the file. Normally this is an OK assumption but the fuzzer has this line in the bin section:

[[bin]]
name = "pallet-paged-list"

So my assumption is bad and what I should be doing is real TOML parsing here. So will add that.

@sam0x17
Copy link
Contributor

sam0x17 commented Aug 4, 2023

note: this now bumps us to docify 0.2.1 which includes the fix for the above 👍🏻

@sam0x17 sam0x17 added the E2-dependencies Pull requests that update a dependency file. label Aug 4, 2023
@ggwpez
Copy link
Member Author

ggwpez commented Aug 5, 2023

bot merge

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit E2-dependencies Pull requests that update a dependency file.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants