Skip to content

collect all Fuchsia bindings into the fuchsia module #140656

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 7, 2025
Merged

collect all Fuchsia bindings into the fuchsia module #140656

merged 1 commit into from
May 7, 2025

Conversation

joboet
Copy link
Member

@joboet joboet commented May 5, 2025 •
edited
Loading

The Fuchsia bindings are currently spread out across multiple modules in sys/pal/unix leading to unnecessary duplication. This PR moves all of these definitions into sys::pal::unix::fuchsia and additionally:

  • deduplicates the definitions
  • makes the error names consistent
  • marks zx_thread_self and zx_clock_get_monotonic as safe extern functions
  • removes unused items (there's no need to maintain these bindings if we're not going to use them)
  • removes the documentation for the definitions (contributors should always consult the platform documentation, duplicating that here is just an extra maintenance burden)

@rustbot ping fuchsia

@joboet
collect all Fuchsia bindings into the fuchsia module
7845c01 
The Fuchsia bindings are currently spread out across multiple modules in `sys/pal/unix` leading to unnecessary duplication. This PR moves all of these definitions into `sys::pal::unix::fuchsia` and additionally:
* deduplicates the definitions
* makes the error names consistent
* marks some extern functions as safe
* removes unused items (there's no need to maintain these bindings if we're not going to use them)
* removes the documentation for the definitions (contributors should always consult the platform documentation, duplicating that here is just an extra maintenance burden)
@rustbot
Copy link
Collaborator

rustbot commented May 5, 2025

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. O-fuchsia Operating system: Fuchsia labels May 5, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 5, 2025

Hey friends of Fuchsia! This issue could use some guidance on how this should be
resolved/implemented on Fuchsia. Could one of you weigh in?

cc @erickt @Nashenas88

@Nashenas88
Copy link
Contributor

I'm working on routing this internally to see who's best to review here. Erick is also out this week.

@workingjubilee
Copy link
Member

Thanks!

I will give the Fuchsia folks a bit to actually review this then.

@workingjubilee
Copy link
Member

It looks like it's almost entirely just-moving-stuff tho'.

@Nashenas88
Copy link
Contributor

@workingjubilee my concerns are around the items that are removed, e.g. zx_handle_duplicate, which is part of the public zircon API. It's not clear to me whether removing it will break anything downstream.

@workingjubilee
Copy link
Member

@Nashenas88 How would it break anything downstream? If the Rust standard library makes the declaration internally, then that doesn't introduce the declaration into other Rust namespaces. It may induce linkage in some cases, because linkage in ELF, as I understand it, is often a fundamentally "global" phenomenon even if you have "local" declarations. However, if you are obtaining linkage to something via extern "C" decls hidden in the standard library... well, please don't, and declare it yourself?

@workingjubilee
Copy link
Member

workingjubilee commented May 6, 2025 •
edited
Loading

...and I do understand that question is something you may not know how to answer for certain, for what it's worth, which is er, why you are asking around for a reviewer, I'm just really curious if there's a way for that to even hypothetically be a problem given what we know. It's an interesting puzzle.

@Nashenas88
Copy link
Contributor

Oh I see why I was so confused. I didn't realize everything in the sys is internal only. With everything marked pub (rather than pub(crate)/pub(super)) I was assuming these were part of the public API. Now that I've taken a closer look at the changes and how they're used, this looks fine to me. At some point we may also want to take a pass and mark these pub(crate) as that will also trigger the unused item warnings (which gets bypassed for anything marked pub).

@workingjubilee
Copy link
Member

Aha!

Yes that is a bit confusing, historically. Thanks for the review!

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented May 6, 2025

📌 Commit 7845c01 has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 6, 2025
@jhpratt jhpratt mentioned this pull request May 7, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 7, 2025
@bors
Auto merge of rust-lang#140726 - jhpratt:rollup-b6oxopx, r=jhpratt
891b852 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#134273 (de-stabilize bench attribute)
 - rust-lang#139534 (Added support for `apxf` target feature)
 - rust-lang#140419 (Move `in_external_macro` to `SyntaxContext`)
 - rust-lang#140483 (Comment on `Rc` abort-guard strategy without naming unrelated fn)
 - rust-lang#140607 (support duplicate entries in the opaque_type_storage)
 - rust-lang#140656 (collect all Fuchsia bindings into the `fuchsia` module)
 - rust-lang#140668 (Implement `VecDeque::truncate_front()`)
 - rust-lang#140709 (rustdoc: remove unportable markdown lint and old parser)
 - rust-lang#140713 (Structurally resolve in `check_ref_cast` in new solver)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fe97fe4 into rust-lang:master May 7, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone May 7, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 7, 2025
@rust-timer
Unrolled build for rust-lang#140656
5194352 
Rollup merge of rust-lang#140656 - joboet:fuchsia_pal, r=workingjubilee

collect all Fuchsia bindings into the `fuchsia` module

The Fuchsia bindings are currently spread out across multiple modules in `sys/pal/unix` leading to unnecessary duplication. This PR moves all of these definitions into `sys::pal::unix::fuchsia` and additionally:
* deduplicates the definitions
* makes the error names consistent
* marks `zx_thread_self` and `zx_clock_get_monotonic` as safe extern functions
* removes unused items (there's no need to maintain these bindings if we're not going to use them)
* removes the documentation for the definitions (contributors should always consult the platform documentation, duplicating that here is just an extra maintenance burden)

`@rustbot` ping fuchsia
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@workingjubilee workingjubilee

Labels
O-fuchsia Operating system: Fuchsia O-unix Operating system: Unix-like S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.88.0
Development

Successfully merging this pull request may close these issues.
5 participants
@joboet @rustbot @Nashenas88 @workingjubilee @bors