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

Terminal locks its starting directory; perhaps it shouldn't do that? #5506

Closed
ghost opened this issue Apr 23, 2020 · 16 comments · Fixed by #15280
Closed

Terminal locks its starting directory; perhaps it shouldn't do that? #5506

ghost opened this issue Apr 23, 2020 · 16 comments · Fixed by #15280
Assignees
Labels
Area-Commandline wt.exe's commandline arguments Area-Remoting Communication layer between windows. Often for windowing behavior, quake mode, etc. Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.

Comments

@ghost
Copy link

ghost commented Apr 23, 2020

Description of the new feature/enhancement

The directory that Windows Terminal starts up in can't be deleted (in Windows Explorer / Directory Opus / etc). It would be nice if I could delete the folder without exiting wt first.

Use case:

When you configure a profile to include this:
"startingDirectory" : ".",
and then set up the right-click "Start Windows Terminal Here" functionality in Windows Explorer to start a wt instance in any folder you get to create Terminal instances quickly and conveniently.

But, if you get done with that folder and want to delete it then you need to close wt first.

It would be convenient to delete the folder without closing wt first.

What's weird is that this behavior (keeping the starting dir locked so you can't delete it) continues to happen even if you navigate away from that dir in the command line.
(Which makes sense - wt creates new tabs in that same dir so preventing it from disappearing is understandable)

Proposed technical implementation details (optional)

  • Do NOT lock the directory, allowing the user to delete it.
  • Start new tabs in the default starting directory instead

maintainer edit:

Also using to track the thing where wt needs to tell the existing window about the current CWD, for running actions in an existing window (which may have a different CWD), see #13430

@ghost ghost added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Apr 23, 2020
@zadjii-msft
Copy link
Member

@MikePanitz-CCC If you tried doing the same thing in the legacy console (opening in a specific directory, navigating the shell away from that directory, then removing that directory), does the same thing happen?

I'd think that this would be the same behavior for all applications, where you can't remove the directory when a running process has that directory as it's current working directory, but I'm not 100% sure on that

@zadjii-msft zadjii-msft added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 23, 2020
@ghost
Copy link
Author

ghost commented Apr 23, 2020 via email

@ghost ghost added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Apr 23, 2020
@DHowett-MSFT
Copy link
Contributor

I don't disagree, but I do somewhat wonder how this'll interact with future uses of profiles with "startingDirectory": ".". Like: WT can move out of the directory it's in (since I think we're only locking it by virtue of it being our process's CWD), but then any new tab spawned with a profile set up like that will spawn in the wrong directory. We would need to pre-resolve all startingDirectories against the WD at the time of first settings load.

Then, at least those new tabs would error out ("can't find directory") instead of pointing at the wrong directory ...

@DHowett-MSFT DHowett-MSFT added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Attention The core contributors need to come back around and look at this ASAP. labels Apr 23, 2020
@ghost
Copy link
Author

ghost commented Apr 27, 2020

That sounds completely reasonable to me.

@ghost ghost added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Apr 27, 2020
@ghost
Copy link
Author

ghost commented Apr 27, 2020

FWIW:
cmd does not lock the dir when it leaves (I created a new dir, started a new CMD in that dir, had the new cmd do a "cd ..", and then deleted the new dir no problem)

powershell does lock the directory (I created a new dir is PS, cd'd to that new directory started powershell.exe in that new dir, had the original PS do a "cd ..", and then had the NEW powershell instance do a "cd ..", and was NOT able to delete the new dir.)
(I had the new ps.exe do an exit and was able to delete the new dir no problem which makes me very confident that the ps.exe was holding that dir open)

bash (from Git Bash - MinGW64) does not lock the current directory. At least, I was able to start a new bash process in the background ( bash & ) in a new dir, then switch back to my original bash process, have it cd out of the new dir, and then delete the new dir no problem.

What was really weird is that the child bash kept it's current working directory as being the folder I'd just deleted. When I asked it to do an 'ls' it reasonably said ls: cannot open directory '.': No such file or directory

@DHowett-MSFT DHowett-MSFT changed the title Don't lock the starting directory Terminal locks its starting directory; perhaps it shouldn't do that? May 1, 2020
@DHowett-MSFT DHowett-MSFT added Area-Commandline wt.exe's commandline arguments Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels May 1, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 1, 2020
@DHowett-MSFT DHowett-MSFT added this to the Terminal Backlog milestone May 1, 2020
@DHowett-MSFT DHowett-MSFT removed the Needs-Attention The core contributors need to come back around and look at this ASAP. label May 1, 2020
@DHowett-MSFT
Copy link
Contributor

I'm throwing this one on the backlog as a "Help Wanted" bug; We need to figure out the right thing to do here and how that plays with the user's profile settings and what an appropriate fallback looks like.

@ghost
Copy link
Author

ghost commented May 1, 2020

I may try to look at this myself. Obviously anyone else who's interested feel free to jump in.

Especially since I'm pretty busy until, like, August :)

Thanks for looking at this @DHowett-MSFT!

@zadjii-msft
Copy link
Member

You know what, there's actually another issue on the repo that's effectively the same thing: #10849. Both threads have great discussion.

@zadjii-msft
Copy link
Member

@DHowett I know you're out this week, but I know you're still uncontrollably checking github 😝 I'm pulling this into 1.18, because "Use parent process directory" pretty much instantly stops working as expected once all windows are in one process.

  • I'm gonna give each window a "virtual working directory".
  • startingDirectorys will be evaluated relative to that
  • running wt -w new in c:\some\path will set that window's virtual CWD to c:\some\path
  • The terminal will always use c:\windows\system32 as it's actual CWD
  • "Use parent process directory" I suppose now just means "use the window's CWD"

This feels extra dangerous to be yolo'ing in this late in the cycle, so thoughts might be helpful

@zadjii-msft zadjii-msft modified the milestones: Up Next, Terminal v1.18 May 2, 2023
@zadjii-msft zadjii-msft added the Severity-Blocking We won't ship a release like this! No-siree. label May 2, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label May 3, 2023
zadjii-msft added a commit that referenced this issue May 12, 2023
Before process model v3, each Terminal window was running in its own process, each with its own CWD. This allowed `startingDirectory: .` to work relative to the terminal's own CWD. However, now all windows share the same process, so there can only be one CWD. That's not great - folks who right-click "open in terminal", then "Use parent process directory" are only ever going to be able to use the CWD of the _first_ terminal opened. 

This PR remedies this issue, with a theory we had for another issue. Essentially, we'll give each Terminal window a "virtual" CWD. The Terminal isn't actually in that CWD, the terminal is in `system32`. This also will prevent the Terminal from locking the directory it was originally opened in. 

* Closes #5506
* There wasn't a 1.18 issue for "Use parent process directory is broken" yet, presumably selfhosters aren't using that feature
* Related to #14957

Many more notes on this topic in #4637 (comment)


> **Warning** 
> ## Breaking change‼️

This will break a profile like 

```json
{
    "commandline": "media-test.exe",
    "name": "Use CWD for media-test",
    "startingDirectory": "."
},
```

if the user right-clicks "open in terminal", then attempts to open that profile. There's some theoretical work we could do in a follow up to fix this, but I'm inclined to say that relative paths for `commandline`s were already dangerous and should have been avoided.
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label May 12, 2023
DHowett pushed a commit that referenced this issue May 12, 2023
Before process model v3, each Terminal window was running in its own process, each with its own CWD. This allowed `startingDirectory: .` to work relative to the terminal's own CWD. However, now all windows share the same process, so there can only be one CWD. That's not great - folks who right-click "open in terminal", then "Use parent process directory" are only ever going to be able to use the CWD of the _first_ terminal opened.

This PR remedies this issue, with a theory we had for another issue. Essentially, we'll give each Terminal window a "virtual" CWD. The Terminal isn't actually in that CWD, the terminal is in `system32`. This also will prevent the Terminal from locking the directory it was originally opened in.

* Closes #5506
* There wasn't a 1.18 issue for "Use parent process directory is broken" yet, presumably selfhosters aren't using that feature
* Related to #14957

Many more notes on this topic in #4637 (comment)

> **Warning**
> ## Breaking change‼️

This will break a profile like

```json
{
    "commandline": "media-test.exe",
    "name": "Use CWD for media-test",
    "startingDirectory": "."
},
```

if the user right-clicks "open in terminal", then attempts to open that profile. There's some theoretical work we could do in a follow up to fix this, but I'm inclined to say that relative paths for `commandline`s were already dangerous and should have been avoided.

(cherry picked from commit 5c08a86)
Service-Card-Id: 89180224
Service-Version: 1.18
zadjii-msft added a commit that referenced this issue Jul 20, 2023
Adds an action to display a toast containing the Terminal's "virtual"
CWD.

As described in
#4637 (comment).

Useful for debugging #5506 et. al. I almost left it as a debug-only
feature, but figured it would be helpful for others in the #4637
landscape of things.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Commandline wt.exe's commandline arguments Area-Remoting Communication layer between windows. Often for windowing behavior, quake mode, etc. Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants