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

Migrate to our own Signal type #365

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Draft

Migrate to our own Signal type #365

wants to merge 14 commits into from

Conversation

magicant
Copy link
Owner

@magicant magicant commented May 11, 2024

This is part of #353.

This PR also adds support for real-time signals on some platforms.

  • Add the new Signal definition
  • Expose conversion of Signal from and to raw numbers in System
  • Remove use of SigSet from System
  • Add iterator for all supported signals in real system
  • Move conversion from signal number to Condition out of impl std::str::FromStr for Condition
  • Rewrite conversion between ProcessState and nix's WaitStatus
  • Replace impl TryFrom<ProcessState> for ExitStatus with SystemEx::exit_status_for_process_state
  • Replace impl From<Signal> for ExitStatus with SystemEx::exit_status_for_signal
  • Remove impl TryFrom<ExitStatus> for Signal
  • Migrate to the new Signal
  • Address remaining TODO comments
  • Update documentation
  • Update CHANGELOG

Additional todo:

  • Refactor ProcessState so we don't have to unwrap the result of SystemEx::exit_status_for_process_state
    • Maybe we should add ProcessResult::{Exited,Signaled,Stopped} which is guaranteed to have a corresponding exit status. Subshell::start_and_wait (and maybe some other functions) should return the new enum. → Add ProcessResult #366

@magicant magicant self-assigned this May 11, 2024
Copy link

coderabbitai bot commented May 11, 2024

Important

Auto Review Skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@magicant magicant mentioned this pull request Apr 29, 2024
16 tasks
@magicant magicant force-pushed the signal branch 2 times, most recently from 24b6a14 to efe75d3 Compare May 12, 2024 16:06
yash-env/src/system/real/signal.rs Outdated Show resolved Hide resolved
yash-env/src/system/real/signal.rs Show resolved Hide resolved
yash-env/src/system/real/signal.rs Outdated Show resolved Hide resolved
yash-env/src/trap/cond/signal.rs Outdated Show resolved Hide resolved
yash-env/src/trap/cond/signal.rs Outdated Show resolved Hide resolved
This is part of #353. This commit prepares for the transition to our own
Signal type by defining the type and implementing some basic operations.
This commit adds a new variant Signal::Number(c_int) to allow specifying
any signal by its raw signal number. This will make the parser for the
trap built-in condition operand much simpler.
This commit replaces the SigSet type with an array of Signal in the
System trait. This change is necessary because the SigSet type is
specific to the nix crate, and the System trait should not depend on
specific types from external crates.

This commit also updates the implementations of the System trait to
reflect this change. SelectSystem::wait_mask is now an optional Vec of
Signal, and virtual Process now uses a BTreeSet of Signal instead of
SigSet.
This change makes `rt_range` return an empty range when the real system
does not support real-time signals. This simplifies the code that uses
`rt_range` because the caller does not need to handle the `None` case.
}

/// Creates an iterator that yields all signals available in the real system.
pub fn all_signals() -> impl DoubleEndedIterator<Item = Signal2> {

Choose a reason for hiding this comment

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

⚠️ [clippy] reported by reviewdog 🐶

warning: function `all_signals` is never used
   --> yash-env/src/system/real/signal.rs:411:8
    |
411 | pub fn all_signals() -> impl DoubleEndedIterator {
    |        ^^^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

The conversion from ProcessState to ExitStatus is now done by the
System trait as it depends on the system implementation. This commit
removes the TryFrom implementation and replaces it with a call to
System::exit_status_for_process_state.
The conversion should be done by `SystemEx::exit_status_for_signal`.
@magicant
Copy link
Owner Author

magicant commented May 20, 2024

This PR is quite messed up. I'm thinking of starting over with a more organized plan. 🤔


I'm thinking of defining Name, Number, and Spec in yash_env::signal.

Unresolved questions:

  • Should ProcessResult (ProcessState) contain Name or Number?
    • Having a signal Number in ProcessResult would make conversion to/from ExitStatus independent of System
      • Currently, conversion from signal numbers to ExitStatuses is needed in the fg and wait built-ins and subshell starters.
    • Having a singal Name in ProcessResult would be needed for impl Display for ProcessResult
      • Currently, impl Display for ProcessResult is only used in the jobs built-in.
    • So, ProcessResult and ProcessState should contain a signal Number. System should basically operate on Numbers. Signal Names are only used for human interfaces.
  • Should a signal Number allow representing any (possibly non-existing) signal numbers?
    • If the kill built-in and its underlying counterpart System::kill needs to allow trying to send signals with arbitrary numbers, either a Number has to represent any number or System::kill has to accept raw integers rather than Numbers.
    • If a Number only represents a valid signal, System::signal_name_from_number would be infallible, which is convenient when converting a ProcessState for human-readable display.
    • If a Number only represents a valid signal, conversion from ExitStatus to Number would be fallible and need a System. This is used in the kill built-in.
    • If a Number can represent any number, conversion from ExitStatus to Number would be infallible, but fallible conversion is needed from Number to Name.
    • To sum up, a Number representing an invalid signal does not seem to be more useful than a Number that rejects invalid signals.
  • Should a trap Condition represent the signal as Number or Name?
    • Traps should be set for valid signals only, so Conditions should specify the signal by Number (assuming that a Number can only represent a valid signal).

@magicant magicant mentioned this pull request May 25, 2024
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

None yet

1 participant