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.

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

solarish: Restrict openpty and forkpty polyfills to illumos, replace Solaris implementation with FFI #4329

Merged
merged 1 commit into from
Apr 2, 2025
Merged

solarish: Restrict openpty and forkpty polyfills to illumos, replace Solaris implementation with FFI #4329

merged 1 commit into from
Apr 2, 2025

Conversation

yuvraj-wale
Copy link
Contributor

@yuvraj-wale yuvraj-wale commented Mar 13, 2025 •
edited
Loading

Description

  • This PR modifies the openpty and forkpty implementations in the Solarish layer:

    • Restricts the existing polyfill implementations to illumos using cfg(illumos).
    • Removes the Solaris-specific polyfill implementations.
    • Replaces them with FFI definitions where necessary.

  • Related to Remove solarish compat for openpty and forkpty #4045

Sources

Checklist

  • Relevant tests in libc-test/semver have been updated
  • No placeholder or unstable values like *LAST or *MAX are
    included (see #3131)
  • Tested locally (cd libc-test && cargo test --target mytarget);
    especially relevant for platforms that may not be checked in CI

@rustbot
Copy link
Collaborator

rustbot commented Mar 13, 2025

r? @tgross35

rustbot has assigned @tgross35.
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
Copy link
Collaborator

rustbot commented Mar 13, 2025

Some changes occurred in solarish module

cc @jclulow, @pfmooney

pfmooney
pfmooney suggested changes Mar 13, 2025
View reviewed changes
Copy link
Contributor

@pfmooney pfmooney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

illumos still relies on these implementations. If you've confirmed that they are not necessary for Solaris (in which case wouldn't defining them as FFI symbols be necessary?), you should put them behind a cfg(illumos) guard rather than outright removing them.

@yuvraj-wale
Copy link
Contributor Author

Yes my bad, I’ll update it right away. Thanks!

pfmooney
pfmooney reviewed Mar 13, 2025
View reviewed changes
src/unix/solarish/compat.rs Outdated
Comment on lines 179 to 195
#[cfg(not(target_os = "illumos"))]
extern "C" {
pub fn openpty(
amain: *mut c_int,
asubord: *mut c_int,
name: *mut c_char,
termp: *const crate::termios,
winp: *const crate::winsize,
) -> c_int;

pub fn forkpty(
amain: *mut c_int,
name: *mut c_char,
termp: *const crate::termios,
winp: *const crate::winsize,
) -> crate::pid_t;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should go in solaris.rs, where they don't require the guard.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, Thanks!

@tgross35
Copy link
Contributor

@psumbera would you mind double checking this for Solaris?

@psumbera
Copy link
Contributor

As can be seen from failed CI test. Solaris don't define these with 'const`.

So it's just:

       int openpty(int *amaster, int *aslave, char *name,
            struct termios *termp, struct winsize *winp);

       pid_t forkpty(int *amaster, char *name, struct termios *termp,
            struct winsize *winp);

@yuvraj-wale
Copy link
Contributor Author

Updated, thanks for the help!

cc @tgross35

tgross35
tgross35 approved these changes Apr 1, 2025
View reviewed changes
Copy link
Contributor

@tgross35 tgross35 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please rebase and squash. Also the title & PR description should be updated.

@rustbot label +stable-nominated

@rustbot rustbot added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Apr 1, 2025
@yuvraj-wale yuvraj-wale changed the title solarish: remove openpty and forkpty polyfill implementations solarish: Restrict openpty and forkpty polyfills to illumos, replace Solaris implementation with FFI Apr 1, 2025
@yuvraj-wale yuvraj-wale force-pushed the remove-solarish-compat-openpty-forkpty branch 2 times, most recently from a957ede to 8909387 Compare April 1, 2025 08:15
@tgross35
Copy link
Contributor

tgross35 commented Apr 1, 2025

Sorry, could you also update the commit message? It is also out of date (and GH doesn't let me edit that with the merge queue UI)

@yuvraj-wale yuvraj-wale force-pushed the remove-solarish-compat-openpty-forkpty branch from 8909387 to a67d297 Compare April 1, 2025 08:37
@yuvraj-wale yuvraj-wale force-pushed the remove-solarish-compat-openpty-forkpty branch from a67d297 to e9d29ec Compare April 1, 2025 08:38
@tgross35 tgross35 added this pull request to the merge queue Apr 2, 2025
@tgross35
Copy link
Contributor

tgross35 commented Apr 2, 2025

Thanks!

Merged via the queue into rust-lang:main with commit 2235559 Apr 2, 2025
42 of 43 checks passed
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Apr 3, 2025
@yuvraj-wale @tgross35
solarish: restrict openpty and forkpty polyfills to illumos, replace …
8e5fa76 
…Solaris implementation with FFI

(backport <rust-lang#4329>)
(cherry picked from commit e9d29ec)
@tgross35 tgross35 mentioned this pull request Apr 3, 2025
Merged
@tgross35 tgross35 added stable-applied This PR has been cherry-picked to libc's stable release branch and removed stable-nominated This PR should be considered for cherry-pick to libc's stable release branch labels Apr 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pfmooney pfmooney pfmooney requested changes

@tgross35 tgross35 tgross35 approved these changes

Assignees

@tgross35 tgross35

Labels
O-solarish O-unix S-waiting-on-review stable-applied This PR has been cherry-picked to libc's stable release branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yuvraj-wale @rustbot @tgross35 @psumbera @pfmooney