Command built-in (POSIX portability) #347
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The SearchEnv does not need to return the whole built-in set. It only needs to return the built-in definition for a given name.
The SearchEnv does not need to expose the entire FunctionSet. Instead, it should only expose the function that is being searched for. This change makes the SearchEnv trait more focused and easier to implement.
When the command built-in is invoked with the -p option, it uses the standard path to search for the command. To accommodate the dynamic retrieval of the path from the underlying system, the PathEnv::path function should return an Expansion instead of a reference to a variable. The Expansion can contain an arbitrary owned string, allowing the function to return a result that doesn't reference a value retained in the environment.
To implement this method, we need to obtain the standard path from the system. This is done by calling the new method `System::confstr_path`.
The command built-in is required to report the path to the built-in if it is substitutive. To implement this, the `Builtin` variant of the `Target` enum is modified to include the path.
This is what is required by the POSIX standard.
POSIX forbids error messages in this case.
WalkthroughThe recent updates refine the Changes
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File (
|
36 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
Cargo.lockis excluded by:!**/*.lockyash-builtin/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (19)
- yash-builtin/src/command.rs (1 hunks)
- yash-builtin/src/command/identify.rs (1 hunks)
- yash-builtin/src/command/invoke.rs (1 hunks)
- yash-builtin/src/command/search.rs (1 hunks)
- yash-builtin/src/command/syntax.rs (1 hunks)
- yash-builtin/src/lib.rs (3 hunks)
- yash-env/src/builtin.rs (2 hunks)
- yash-env/src/system.rs (3 hunks)
- yash-env/src/system/real.rs (2 hunks)
- yash-env/src/system/virtual.rs (3 hunks)
- yash-env/src/variable.rs (2 hunks)
- yash-env/src/variable/quirk.rs (4 hunks)
- yash-semantics/src/command/simple_command.rs (2 hunks)
- yash-semantics/src/command/simple_command/external.rs (5 hunks)
- yash-semantics/src/command/simple_command/function.rs (1 hunks)
- yash-semantics/src/command_search.rs (19 hunks)
- yash/tests/scripted_test.rs (1 hunks)
- yash/tests/scripted_test/command-p.sh (1 hunks)
- yash/tests/scripted_test/typeset-y.sh (1 hunks)
Additional comments: 40
yash/tests/scripted_test.rs (1)
- 116-119: The addition of the
command_builtintest function correctly follows the established pattern for scripted tests inyash. It's well-structured and adheres to the best practices observed in the rest of the file.yash/tests/scripted_test/command-p.sh (1)
- 1-309: The
command-p.shscript provides a comprehensive set of test cases for thecommandbuilt-in functionality, covering a wide range of scenarios including interaction with functions, special built-ins, external commands, and the handling of environment variables. The script is well-structured, follows best practices for shell scripting, and correctly anticipates the expected outcomes for each test case.yash-builtin/src/command/syntax.rs (2)
- 52-56: The
OPTION_SPECSconstant defines the option specifications for thecommandbuilt-in, including-pfor using the standard search path,-vfor identifying the command, and-Vfor verbose identification. This setup is clear and aligns with POSIX specifications for thecommandbuilt-in. However, it's important to ensure that these options are mutually exclusive where applicable and that their precedence is correctly handled in the parsing logic.- 58-90: The
parsefunction effectively interprets the options and operands to produce aCommandinstance, eitherInvokeorIdentify, based on the presence of the-vor-Voptions. The function's logic is well-structured, but it's crucial to handle edge cases, such as when no command name is provided or when both-vand-Vare specified. The current implementation correctly prioritizes the last specified option between-vand-V, which is a good practice for command-line tools.yash-builtin/src/command/invoke.rs (2)
- 31-47: The
executemethod in theInvokestruct correctly handles the execution flow, including searching for the command and invoking the target. The use of pattern matching withSomeandelseclauses for handling the absence of a command name or target is a modern Rust feature that enhances readability. However, it's important to ensure that error messages and exit statuses are consistent with POSIX specifications when a command is not found or cannot be invoked.- 55-77: The
invoke_targetfunction effectively delegates the invocation to the appropriate handler based on the target type (built-in, function, or external utility). This modular approach simplifies the code structure and makes it easier to maintain. However, it's crucial to ensure that the environment and fields are correctly passed to and handled by each target type, especially regarding environment variable scopes and command arguments.yash-builtin/src/command.rs (1)
- 110-244: The
Commandenum, along with theInvokeandIdentifystructs, are well-defined and encapsulate the parameters and logic for invoking and identifying commands. Themainfunction effectively parses the arguments and executes the appropriate command based on the parsed input. This structure aligns with the expected functionality of thecommandbuilt-in and provides a solid foundation for further enhancements. It's important to ensure that all possible error cases are handled gracefully and that the user is provided with clear and informative error messages.yash-semantics/src/command/simple_command.rs (2)
- 221-232: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [173-233]
The pattern matching adjustments in the
impl Command for syntax::SimpleCommandblock enhance the clarity and maintainability of the command execution logic. By explicitly handling each target type (built-in, function, external utility, and absent target), the code becomes more readable and easier to extend. It's important to ensure that each execution path correctly handles environment variables, arguments, and exit statuses according to POSIX standards.
- 224-229: The introduction of new public exports (
execute_function_body,replace_current_process,start_external_utility_in_subshell_and_wait, andto_c_strings) significantly improves the modularity and reusability of the command execution logic. These exports allow for more granular control over the execution process and facilitate testing. However, it's crucial to ensure that these functions are well-documented and that their interfaces are intuitive for other developers to use effectively.yash-env/src/builtin.rs (2)
- 37-37: The addition of the
Hashtrait to theTypeenum is a positive change, enhancing the ability to use these types as keys in hash maps or sets, which can improve performance and usability in certain scenarios.- 246-246: Implementing the
Hashtrait for theBuiltinstruct is a logical step following the addition of theHashtrait to theTypeenum. This change allows instances ofBuiltinto be used in hashed collections, potentially improving lookup and insertion performance.yash-builtin/src/command/search.rs (3)
- 33-68: The implementation of the
SearchEnvstruct and its integration with thePathEnvtrait is well-structured and follows Rust's idiomatic practices. The use of thestandard_pathflag to determine the source of the path (standard vs.$PATHvariable) is a thoughtful design choice that enhances flexibility.- 70-85: The integration of the
SearchEnvstruct with theyash_semantics::command_search::SearchEnvtrait is correctly implemented. The conditional checks forCategory::BuiltinandCategory::Functionensure that only the requested types of commands are searched, aligning with the intended functionality.- 88-260: The test cases provided in the file are comprehensive and cover a variety of scenarios, including standard and non-standard paths, as well as enabling and disabling search for built-ins and functions. These tests are crucial for ensuring the reliability and correctness of the command search functionality.
yash-env/src/variable/quirk.rs (4)
- 21-21: The addition of the
eithercrate and its usage in thesplitmethod of theExpansionenum is a good choice for handling different iterator types in a clean and efficient manner. This enhances the code's readability and maintainability.- 64-69: Implementing the
Defaulttrait for theExpansionenum to returnUnsetas the default value is a logical choice, ensuring that uninitialized expansions have a clear and consistent state.- 155-163: The addition of the
From<Expansion<'_>>implementation forOption<Value>is a useful conversion feature, facilitating the transformation of expansions into optional values, which can simplify handling of variable expansions in different contexts.- 208-240: The
splitmethod added to theExpansionenum is a valuable utility for handling path-like expansions, allowing for easy splitting of scalar values by colons and handling arrays and unset values appropriately. This method enhances the flexibility and usability of theExpansionenum in path-related operations.yash-builtin/src/lib.rs (2)
- 55-56: The addition of the
commandmodule as a public entity when theyash-semanticsfeature is enabled is correctly implemented. This change aligns with the PR objectives to enhance theyashshell'scommandbuilt-in functionality. The use of the feature flag is appropriate and follows Rust's best practices for conditional compilation.- 142-148: The addition of the
commandbuilt-in to theBUILTINSarray is correctly implemented, with appropriate properties (r#type: Mandatory,executelogic). This change aligns with the PR objectives to enhance theyashshell'scommandbuilt-in functionality and follows the established pattern for built-ins.yash-semantics/src/command/simple_command/function.rs (2)
- 51-58: The modifications to the
execute_functionfunction, including the direct operation on theenvparameter and the use ofexecute_function_bodyfor executing the function body, are well-implemented. These changes enhance the modularity and flexibility of function execution within the shell, aligning with the PR objectives to improve shell functionality. The logic appears correct, and the implementation follows best practices for async function execution in Rust.- 48-89: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [60-90]
The introduction of the
execute_function_bodyfunction is a significant improvement, allowing for custom variable assignments before executing the function body. This function enhances the shell's ability to handle function execution with more flexibility and aligns with the PR objectives to refine command execution logic. The implementation is correct, and the use of a modifier function for custom assignments before execution is a thoughtful addition.yash-semantics/src/command/simple_command/external.rs (3)
- 66-66: The error handling for when the
pathto the external utility is empty has been improved by providing a more descriptive error message and setting the exit status toNOT_FOUND. This is a positive change as it enhances user feedback and error clarity.- 78-80: The call to
start_external_utility_in_subshell_and_waitand setting theenv.exit_statusbased on its return value is a significant improvement. It encapsulates the logic for starting an external utility in a subshell, waiting for it to finish, and handling the exit status in a dedicated function, which enhances modularity and readability.- 131-137: The error handling within
start_external_utility_in_subshell_and_waitwhen the execution fails due to anErrnois well-implemented. It provides clear feedback to the user about the failure to execute the external utility, including the utility name and the error description. This is a good practice for usability and debugging.yash-semantics/src/command_search.rs (5)
- 53-68: The refactoring of the
Targetenum to include apathfield forBuiltinis a thoughtful addition. It allows handling shadowed external utilities, which is crucial for a shell that aims to be POSIX-compliant. This change enhances the shell's ability to distinguish between built-in and external commands when their names collide.- 110-116: The modification to the
PathEnvtrait to return anExpansiontype instead of a direct reference is a significant improvement. It allows for more dynamic handling of the$PATHvariable, accommodating scenarios where the path may be computed at runtime. This change increases the flexibility and robustness of the command search mechanism.- 181-211: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [171-208]
The updates to the
searchfunction to handle external utility search more precisely based on the presence of slashes in the name are well-implemented. This logic aligns with POSIX standards and improves the shell's behavior in determining whether to treat a command as an external utility. The distinction between different types of built-ins (Special,Substitutive, etc.) and their handling in the search process is clear and well-structured.
- 218-226: The
search_pathfunction's implementation is concise and effective. It iterates over directories specified in the$PATHvariable and checks for the existence of the executable, which is a core requirement for command search functionality in a shell. The use ofCStringand handling of relative paths are appropriate for the context of a Unix-like environment.- 483-498: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [234-593]
The comprehensive test suite covering various scenarios, including empty environments, unmatched names, priority of built-ins over functions, and the search for external executables, is commendable. These tests ensure that the command search functionality behaves as expected under different conditions. It's particularly noteworthy how the tests account for the new
pathfield in theTarget::Builtinvariant and the dynamic computation of the$PATHvariable.yash-builtin/src/command/identify.rs (7)
- 1-15: The file header correctly includes the copyright notice and license information, ensuring compliance with the GNU General Public License version 3 or later. This is crucial for maintaining the legal integrity of the project.
- 17-42: The module documentation and imports are well-organized, providing a clear overview of the file's purpose and the external dependencies required for its functionality. This helps in understanding the context and scope of the command identification features within the
yashshell.- 43-60: The
NotFoundstruct and its implementation of theMessageBasetrait provide a structured way to handle and report command not found errors. This approach enhances error reporting by including both the command name and its origin, which can be very helpful for debugging and user feedback.- 62-80: The
NormalizeEnvtrait and its implementation forEnvare crucial for abstracting the environment-specific operations needed for command normalization. This design promotes code reusability and maintainability by decoupling the normalization logic from the specific environment implementation.- 108-169: The
describe_targetfunction effectively generates a description of the given target, which is crucial for verbose output and debugging. The use of pattern matching on thetargetparameter is clean and ensures that each command type is handled appropriately. The implementation is concise and leverages Rust's powerful formatting capabilities.- 226-263: The
Identifystruct and its methods provide a high-level interface for performing command identification. The asyncexecutemethod, in particular, demonstrates a good practice of separating the logic for generating results from the logic for handling output and errors. This separation of concerns aids in maintaining clean and testable code.- 265-582: The unit tests are comprehensive, covering various scenarios for command normalization and identification. These tests ensure that the functionality behaves as expected under different conditions. It's good practice to include such tests to validate the correctness of the code and prevent regressions in future changes.
yash-env/src/variable.rs (1)
- 49-49: The import of
Fieldis correctly added to support the new functionality inPositionalParams. This change aligns with the PR's objective to enhance command handling capabilities.yash-env/src/system.rs (2)
- 387-391: The addition of the
confstr_pathmethod to theSystemtrait is a valuable enhancement for retrieving the standard$PATHvalue. This method abstracts the underlying system call or functionality to fetch the$PATHwhere utilities are expected to be found, which is crucial for shell operations and command execution. The method's documentation and signature are clear and concise, making its purpose and usage straightforward.- 957-959: The implementation of the
confstr_pathmethod in theSharedSystemstruct correctly delegates the call to the containedSysteminstance. This approach maintains the abstraction layer and allows for the method's use in both synchronous and asynchronous contexts within the shell environment. The implementation is concise and follows the established pattern for delegatingSystemtrait method calls within theSharedSystemstruct.
The char type may be i8, so we should convert.
The NotFound error does not happen in the describe_target function, so it should not be used as the error type. Instead, the function now returns std::fmt::Error, which is the error type of the Write trait we failed to handle gracefully.
This change splits the `identify` function into two functions: `categorize` and `describe`. The `categorize` function determines the category of the given command name, and the `describe` function produces the description of the categorized command name that is finally printed to the standard output. This change makes the code more modular and easier to understand. Notably, the new `describe` function propagates the `std::fmt::Result` error type, which was not handled gracefully in the previous implementation.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- yash-builtin/src/command/identify.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- yash-builtin/src/command/identify.rs
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- yash-builtin/src/command/identify.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- yash-builtin/src/command/identify.rs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
commandbuilt-in functionality in the yash shell, allowing users to execute utilities, search for commands, and identify command types and locations.$PATHvalue across different system implementations.commandbuilt-in's functionality and compliance with POSIX standards.