Use the same temporary home directory when 'HOME' env variable is not set by daxian-dbw · Pull Request #16263 · PowerShell/PowerShell

daxian-dbw · 2021-10-17T16:49:41Z

PR Summary

Use the same temporary home directory for a user account when the environment variable 'HOME' is not set for the user.

The previous solution used for unset HOME environment variable was to create a temporary folder and delete if when the Runspace is disposed. However, when pwsh exits, Runspace.Dispose doesn't get called -- the whole process is being torn down, so no much point to dispose the Runspace anyways. Therefore, every execution of pwsh will leave a temporary folder behind.

This PR updates pwsh to create and reuse a single temporary home directory for every user account. So executions of pwsh by a specific user will only have one temporary home folder created.

PR Checklist

… set

iSazonov · 2021-10-18T04:28:24Z

-                return _tempDirectory;
+                // Directory creation may fail if the account doesn't have filesystem permission such as some service accounts.
+                // Return an empty string in this case so the process working directory will be used.
+                s_tempHome = string.Empty;


Put temp files in CWD could annoying users. Maybe try CWD + tempHomeFolderName?

Should we thing about right permission mask if we fallback?

This is the same behavior we have today. I don't know what is the right thing to do when the user account has no file system permission, so keep the existing behavior.

As for permission mask, yes, we should consider that. I have updated the code to use one temp home folder for each user account, instead of having all user accounts sharing the same one -- imagining multiple service accounts available on the same machine.

This is because the first user that creates the folder will make the folder non-writable by other non-root users by default. Also, it makes more sense for different users have different temp home folders.

iSazonov · 2021-10-19T03:55:05Z

                if (!Directory.Exists(systemResourceRoot))
                {
-                    configSystemPath = Platform.GetFolderPath(Environment.SpecialFolder.System);
+                    configSystemPath = Environment.GetFolderPath(Environment.SpecialFolder.System);


If we change this should we keep consistency through all code base? I found 6 SpecialFolder.System and 6 SpecialFolder.System and 9 usings of Platform.GetFolderPath() (not all code compiled).

If we can not rid of Platform.GetFolderPath() maybe use Environment.GetFolderPath() in Platform.GetFolderPath() ?

If you go up from this call site, you will find this call site is in the else block of the if (Platform.IsLinux || Platform.IsMacOS), meaning that this code is for windows only. So we can use Environment.GetFolderPath instead.

If we can not rid of Platform.GetFolderPath() maybe use Environment.GetFolderPath() in Platform.GetFolderPath()

We already uses Environment.GetFolderPath() within that method for Windows code path.

What I mean is that the code is unnecessarily convoluted, although all the logic could have been concentrated in the Platform.GetFolderPath()

What code is unnecessarily convoluted? My plan is to get rid of Platform.GetFolderPath() altogether. Now the blocking piece is the s_debugPreferenceCachePath cache file, as soon as that dependency is gone, we can remove Platform.GetFolderPath() (the MyDocuments dependency is easy to tackle with).

s_runspaceDebugPreference

Do you mean s_debugPreferenceCachePath? Is there plan to remove the dependency?

What code is unnecessarily convoluted? My plan is to get rid of altogether.

I see you point. But since we can not rid of the method my preference would be to put all logic to the method so that we can explicitly see why we can not remove the method and use .Net API directly. It is not a request for the PR :-)

Yes, s_debugPreferenceCachePath. My comment above was corrected.
Given that the current path for that cache file is obviously wrong, I think we will remove that dependency on Platform.GetFolderPath() when fixing that. No plan to submit a PR yet, but should be simple changes.
After that, the only dependency on Platform.GetFolderPath() is just MyDocuments, and it'd be easy to get rid of Platform.GetFolderPath().

iSazonov · 2021-10-19T05:01:32Z

+            if (envHome is not null)
            {
-                return;
+                return envHome;


Hmm, if user change the HOME env variable we return new value. Should we be stable and return s_tempHome always?
I think changing HOME on the fly can destroy current session (not only PowerShell but OS too).

That is the existing behavior. If you change HOME env variable and create a new Runspace, you will find it uses the new HOME value for that Runspace. Personally, I don't think this is practically a problem. And it could potentially be a breaking change if you change that behavior.

I always thought that .config and .cache were always created per session

They are not. All sessions share the same .config and .cache folders. .cache folder contains the module analysis cache file and the startup profile data, and those are supposed to be used across sessions.

I see the code:

string xdgConfigHomeDefault = Path.Combine(envHome, ".config", "powershell"); string xdgDataHomeDefault = Path.Combine(envHome, ".local", "share", "powershell"); string xdgModuleDefault = Path.Combine(xdgDataHomeDefault, "Modules"); string xdgCacheDefault = Path.Combine(envHome, ".cache", "powershell");

Here envHome is re-evaluated every time.

SelectProductNameForDirectory is used only at startup and new Runspace creation (for module path).
Again, this PR doesn't change the current behavior when "HOME" value gets changed during a session.

pull-request-quantifier-deprecated · 2021-11-02T16:58:21Z

This PR has 214 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Large
Size       : +79 -135
Percentile : 61.4%

Total files changed: 7

Change summary by file extension:
.cs : +68 -135
.ps1 : +11 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detetcted.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

ghost · 2021-11-19T02:00:56Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

daxian-dbw · 2021-11-19T02:12:51Z

@adityapatwardhan Can you please review this PR? Thanks!

ghost · 2021-12-16T20:01:30Z

🎉v7.3.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

… set (PowerShell#16263)

Use the same temporary home directory when 'HOME' env variable is not…

d0d7bcb

… set

ghost assigned TravisEz13 Oct 17, 2021

iSazonov reviewed Oct 18, 2021

View reviewed changes

daxian-dbw added 3 commits October 18, 2021 11:46

Use different temp home directories for different users

a5d6733

Skip test on windows

f464552

Fix test

33d18ba

daxian-dbw marked this pull request as ready for review October 18, 2021 21:01

daxian-dbw requested review from SteveL-MSFT, TravisEz13 and anmenaga as code owners October 18, 2021 21:01

iSazonov reviewed Oct 19, 2021

View reviewed changes

daxian-dbw added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Oct 20, 2021

iSazonov approved these changes Oct 20, 2021

View reviewed changes

daxian-dbw closed this Nov 2, 2021

daxian-dbw reopened this Nov 2, 2021

pull-request-quantifier-deprecated Bot added the Large label Nov 2, 2021

daxian-dbw assigned adityapatwardhan and unassigned TravisEz13 Nov 10, 2021

ghost added the Review - Needed The PR is being reviewed label Nov 19, 2021

ghost removed the Review - Needed The PR is being reviewed label Nov 19, 2021

adityapatwardhan approved these changes Nov 22, 2021

View reviewed changes

adityapatwardhan merged commit 88ba3f6 into PowerShell:master Nov 22, 2021

daxian-dbw deleted the home branch November 24, 2021 01:05

ghost mentioned this pull request Dec 16, 2021

Garbage in TMPDIR resulting in Failed to create CoreCLR, HRESULT: 0x80004005 #15299

Closed

TrapGodBrim pushed a commit to TrapGodBrim/PowerShell that referenced this pull request Jan 19, 2022

Use the same temporary home directory when 'HOME' env variable is not…

33a683a

… set (PowerShell#16263)

daxian-dbw mentioned this pull request May 4, 2022

Powerhsell 7.x Linux not honoring opt-out env variables #16112

Closed

5 tasks

Conversation

daxian-dbw commented Oct 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

iSazonov Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pull-request-quantifier-deprecated Bot commented Nov 2, 2021

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

ghost commented Nov 19, 2021

Uh oh!

daxian-dbw commented Nov 19, 2021

Uh oh!

ghost commented Dec 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

daxian-dbw commented Oct 17, 2021 •

edited

Loading

iSazonov Oct 18, 2021 •

edited

Loading

daxian-dbw Oct 19, 2021 •

edited

Loading