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

Update bindgen script and bindings #125

Merged
merged 13 commits into from May 30, 2023
Merged

Update bindgen script and bindings #125

merged 13 commits into from May 30, 2023

Conversation

Meziu
Copy link
Member

@Meziu Meziu commented May 23, 2023

This is a continuation of #110, rebased on a local branch to aid collaboration on this issue.

Techie-Pi and others added 8 commits April 4, 2023 18:49
- Added the `bindgen-ctru-sys` to be able to use `ParseCallbacks` when generating the bindings.
- Updated `doxygen-rs` to 0.4, which has a faster engine and more extensible internal (even though there are some regressions).
- Removes the `docstring-to-rustdoc` package because it is no longer needed.
Add `bindgen-ctru-sys` package and update bindings
@Meziu Meziu requested a review from a team as a code owner May 23, 2023 18:48
@Meziu
Copy link
Member Author

Meziu commented May 26, 2023

@ian-h-chamberlain It seems the CI is running this command: cargo 3ds clippy --color=always --workspace --verbose --all-targets. However, due to the workspace flag, crates which are meant for the host machine (like clang-sys) are getting built by clippy (and fail to do so). Is it safe to remove this requirement?

@Meziu
Copy link
Member Author

Meziu commented May 26, 2023

@Techie-Pi Some comments within the generated bindings are getting mistaken as doctests by cargo test. Is this an error within bindgen-rs?

@Techie-Pi
Copy link
Member

doxygen-rs just parses and transpiles the Doxygen comments using the ParseCallbacks::process_comment API. This doesn't look like an issue from doxygen-rs.
I'm not really sure why rustdoc thinks those are doctests...

@ian-h-chamberlain
Copy link
Member

@ian-h-chamberlain It seems the CI is running this command: cargo 3ds clippy --color=always --workspace --verbose --all-targets. However, due to the workspace flag, crates which are meant for the host machine (like clang-sys) are getting built by clippy (and fail to do so). Is it safe to remove this requirement?

Yeah, I think it's probably fine. I think we can just remove --workspace, and the default-members in Cargo.toml will pick the right packages to check - but it would be good to double check.

@Techie-Pi Some comments within the generated bindings are getting mistaken as doctests by cargo test. Is this an error within bindgen-rs?

It looks like some of the documentation contains entries with 4 spaces, is treated as "rust code" by the doctest runner. Example:

extern "C" {
    #[must_use]
    #[doc = "Process management\n# **\n* Gets the handle of a process.\n* # Arguments\n\n* `process` (direction out) -   The handle of the process\n*      processId The ID of the process to open\n*/"]
    pub fn svcOpenProcess(process: *mut Handle, processId: u32_) -> Result;
}

The docstring expanded looks like this:

Process management
# **
* Gets the handle of a process.
* # Arguments

* `process` (direction out) -   The handle of the process
*      processId The ID of the process to open
*/

The second-to-last line gets treated like a bullet containing a code block, which of course fails to compile:

Process management

**

  • Gets the handle of a process.

  • Arguments

  • process (direction out) - The handle of the process

  •  processId The ID of the process to open
    

*/

The original comment looks like it uses the spaces to align params:

///@name Process management
///@{
/**
 * @brief Gets the handle of a process.
 * @param[out] process   The handle of the process
 * @param      processId The ID of the process to open
 */

@Techie-Pi
Copy link
Member

Techie-Pi commented May 30, 2023

That's interesting tbh, I'll fix it later and publish a new version.

Edit: Just published 0.4.2 which should fix (or at least improve) it.

@Meziu Meziu merged commit 54c6359 into master May 30, 2023
4 checks passed
@Meziu Meziu deleted the update-sys branch May 30, 2023 17:27
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.

None yet

3 participants