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

[MNG-8587] mvnsh navigation #2117

Merged
merged 17 commits into from
Feb 24, 2025
Merged

[MNG-8587] mvnsh navigation #2117

merged 17 commits into from
Feb 24, 2025

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Feb 19, 2025 •
edited
Loading

Ability to navigate on disk and new commands:

  • ! execute shell command (executable or script)
  • cd change cwd
  • pwd print cwd

https://issues.apache.org/jira/browse/MNG-8587

@cstamas cstamas requested a review from gnodet February 19, 2025 08:48
@cstamas cstamas self-assigned this Feb 19, 2025
@cstamas cstamas added this to the 4.0.0-rc-3 milestone Feb 19, 2025
@cstamas
Copy link
Member Author

cstamas commented Feb 19, 2025 •
edited
Loading

@gnodet help needed, tried to implement the "shell" command as usually done with !, but for some reason did not work, !ls invoked the ls command instead doing shell (the ! got removed for some reason). Why? Fixed

@cstamas
Copy link
Member Author

cstamas commented Feb 21, 2025 •
edited
Loading

Applied proposed changes. And some explanations:

  • removing special handling for ".." and simplified things
  • we deal with 3 paths: installationDir, userDir and CWD. Former two are never changing, latter is mutable (but only in mvnsh).
  • hence, installation and user made plain Path, but it shows the intent of Function<String, Path> now: I just wanted to prevent repeated use of normalize() (as user may say -Dfoo=../../bar.xml or alike). But given there is a handful access to installation and user, this is fine.
  • renamed Directory to CWD to have clear intent (and it is accessed MUCH MORE than former two, hence i made here a new method resolve(seg) (equivalent of former function). Also, I tried to made clear how CWD is mutable, but allows transitions to existing directories ONLY.

@cstamas cstamas marked this pull request as ready for review February 21, 2025 10:50
@cstamas cstamas requested a review from gnodet February 21, 2025 10:50
@cstamas
Copy link
Member Author

cstamas commented Feb 21, 2025

Anyone?

slawekjaranowski
slawekjaranowski reviewed Feb 22, 2025
View reviewed changes
...in/java/org/apache/maven/cling/invoker/mvnsh/builtin/BuiltinShellCommandRegistryFactory.java Outdated
processArgs.add("sh");
processArgs.add("-c");
}
processArgs.add(String.join(" ", args));
Copy link
Member

Choose a reason for hiding this comment

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

we should add each item as new argument ... not a one arument with spaces

Copy link
Member Author

Choose a reason for hiding this comment

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

This is shell -c and it must be one argument (space separated)

gnodet
gnodet reviewed Feb 24, 2025
View reviewed changes
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnenc/EncryptInvoker.java
Thread executeThread = Thread.currentThread();
context.terminal.handle(Terminal.Signal.INT, signal -> executeThread.interrupt());
context.terminal.handle(
Terminal.Signal.INT, signal -> Thread.currentThread().interrupt());
Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds weird. The current thread cannot be the Maven execution thread, unless maybe it actually reads from stdin. It should be a terminal's pump thread.

Copy link
Member Author

Choose a reason for hiding this comment

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

mvnenc command is always single threaded, and did install this handler from beginning to handle ctrl+c while inside of ConsolePrompt (ESC is back, Ctrl+C is "abort").

cstamas
cstamas commented Feb 24, 2025
View reviewed changes
...in/java/org/apache/maven/cling/invoker/mvnsh/builtin/BuiltinShellCommandRegistryFactory.java

private void pwd(CommandInput input) {
try {
shellContext.writer.accept(shellContext.cwd.get().toString());
Copy link
Member Author

Choose a reason for hiding this comment

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

Am inconsistent here: sometimes am using context.writer and sometimes context.logger... let's settle on one of these

Copy link
Member Author

@cstamas cstamas Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

command output does not go to logger, EXCEPT for:

  • shell stderr
  • exitCode != 0
  • command wrongly invoked by user (cd with more than 1 or 0 param)

These above go to context.logger.error

@cstamas cstamas merged commit e607433 into apache:master Feb 24, 2025
13 checks passed
@cstamas cstamas deleted the MNG-8587 branch February 24, 2025 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slawekjaranowski slawekjaranowski slawekjaranowski left review comments

@gnodet gnodet gnodet approved these changes

Assignees

@cstamas cstamas

Labels
maintenance
Projects
None yet
Milestone
4.0.0-rc-3
Development

Successfully merging this pull request may close these issues.
3 participants
@cstamas @gnodet @slawekjaranowski