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

Support proc-macro lib type #466

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DenisGorbachev
Copy link
Contributor

Hi, thank you for the crate!

I've implemented support for proc-macro lib type (see tests/pass/proc-macro). Currently, this is done in a backwards-compatible way: there is a new option --target-name that allows to specify the target name explicitly. The target name can be specified for a target of any type (including proc-macro).

Would you like to merge this PR?

@msrd0
Copy link
Owner

msrd0 commented Jul 21, 2024

I'm not sure I understand the need for this change. Procedural macros are, afaik, just some special sort of library, and should be covered under library targets. Further, a package/crate/whateveryoucallit may only contain one library, so there is no need to distinguish between proc-macro libraries and "normal" libraries.

I ran the test case from this PR against the main branch and the test was successful.

Can you please elaborate why this change is necessary? What is the problem with the way proc-macro libraries are currently handled?

@DenisGorbachev
Copy link
Contributor Author

DenisGorbachev commented Jul 21, 2024

Actually, "proc-macro" is a separate kind. The check target.is_lib() translates to target.is_kind("lib"), which doesn't match "proc-macro". So if the package has only a single target with kind = "proc-macro", then cargo-doc2readme can't find a suitable target (because it looks for kind = "lib" or kind = "bin").

Also, the test doesn't pass on my machine.

git checkout feature/target-proc-macro
cargo metadata --format-version 1 --manifest-path tests/pass/proc-macro/Cargo.toml | jq -r '.packages[0].targets[0] | {name, kind}' 
# {
#   "name": "proc_macro",
#   "kind": [
#     "proc-macro"
#   ]
# }
cargo doc2readme --out - --manifest-path tests/pass/proc-macro/Cargo.toml                   
# Error: Failed to find a library or binary target

@msrd0
Copy link
Owner

msrd0 commented Jul 24, 2024

Oh I see, you marked the test case nightly-only, and I didn't run it with a nightly compiler 😅

@msrd0
Copy link
Owner

msrd0 commented Jul 24, 2024

I'm still not sure I support the approach you took here. From my perspective, there can be at most 1 library (proc-macro or not) per package, and I consider proc-macro to be a special case of libraries. So I would be extremely surprised if proc-macro was a separate type. Do you think it would make sense to keep the lib/bin separation but attempt to find a proc-macro target if a normal library target was not found?

@DenisGorbachev
Copy link
Contributor Author

proc-macro is a separate kind, in fact :) Per cargo metadata:

git checkout feature/target-proc-macro
cargo metadata --format-version 1 --manifest-path tests/pass/proc-macro/Cargo.toml | jq -r '.packages[0].targets[0] | {name, kind}' 
# {
#   "name": "proc_macro",
#   "kind": [
#     "proc-macro"
#   ]
# }

For comparison, here's cargo metadata output for a regular lib:

cargo metadata --format-version 1 --manifest-path tests/pass/fn-link/Cargo.toml | jq -r '.packages[0].targets[0] | {name, kind}'
# {
#   "name": "fn_link",
#   "kind": [
#     "lib"
#   ]
# }

So the kind array contains either proc-macro for proc macro libs, or lib for regular libs. I know it's confusing, but that's how cargo metadata reports it.

Because of that, the check for target.is_lib() fails for proc-macro libs. Hence the PR that fixes it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants