Skip to content

Conversation

@andrewliebenow
Copy link
Contributor

Also: move some platform-specific code into plib.

Also: move some platform-specific code into plib.
@jgarzik
Copy link
Contributor

jgarzik commented Sep 16, 2024

Looks pretty good, and close.

Nits:

  • linux CI failing
  • prefer to name plib ipc module ipc.rs not sys.rs

@andrewliebenow
Copy link
Contributor Author

I suspect that tests in which the program does some processing and then later starts reading from stdin to get user confirmation are subject to intermittent failure like happened here.

    let mut child = command
        .args(args)
        .stdin(Stdio::piped())
        .stdout(Stdio::piped())
        .stderr(Stdio::piped())
        .spawn()
        .expect("failed to spawn head");

    let stdin = child.stdin.as_mut().expect("failed to get stdin");
    stdin
        .write_all(stdin_data)
        .expect("failed to write to stdin");

In order to make these tests totally reliable, there would probably need to be some logic to only start writing to stdin after the program writes something to stdout indicating that it's waiting on user input. Not sure it's worth doing that vs. just putting up with random test failures.

@andrewliebenow andrewliebenow force-pushed the fix-musl-build-only branch 2 times, most recently from 04412be to ca5e999 Compare September 19, 2024 04:31
@jgarzik
Copy link
Contributor

jgarzik commented Sep 19, 2024

Yes, after pounding on the "re-run failed tests" a few times, skipping grep test inconsistencies, the checks eventually pass.

This PR is looking pretty good. Will give it another review post-travel (~Saturday), and probably merge.

As a more general comment, it is preferred to separate out commits into logical changes, for example in this PR, something like
C1: create and use platform module
C2: create and use plib/terminal
C3: create and use plib/priority
C4: add support for musl

See this doc from my old Linux kernel buddy for more examples: https://github.com/axboe/liburing/blob/master/CONTRIBUTING.md

@jgarzik
Copy link
Contributor

jgarzik commented Sep 21, 2024

Cherry-picked some of the changes - with slight modification - whittling this core PR down a bit.

@jgarzik
Copy link
Contributor

jgarzik commented Sep 21, 2024

With #232, I think all of this is merged (inclusive of my own modifications), with the exception of IPC-related.

ipcs/ipcrm could probably use some considered attention for macos and linux as well as musl. I am hopeful that a good strategy would emerge.

@andrewliebenow
Copy link
Contributor Author

Awesome, thanks for getting this in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants