GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Allow PowerShell to start even if Data, Home, and Cache folders cannot be created by SteveL-MSFT · Pull Request #3244 · PowerShell/PowerShell

Conversation

@SteveL-MSFT

@SteveL-MSFT Steve Lee (SteveL-MSFT) commented Mar 3, 2017

Copy link
Copy Markdown
Member

Allow Directory Creation failures to continue for Service Account use scenarios that don't have home directories

addresses #3011

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SteveL-MSFT
Steve Lee (SteveL-MSFT) requested a review from Jason Shirk (lzybkr) March 3, 2017 01:42
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Mar 3, 2017
lzybkr
Jason Shirk (lzybkr) reviewed Mar 3, 2017
View reviewed changes
Comment thread test/powershell/Host/ConsoleHost.Tests.ps1 Outdated

@lzybkr Jason Shirk (lzybkr) Mar 3, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

While extremely unlikely - this test could cause spurious failures, if for example a background process installed a new module between runs.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Mar 3, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Do you think it would sufficient to just check it's greater than 0?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/Modules/AnalysisCache.cs Outdated

@lzybkr Jason Shirk (lzybkr) Mar 3, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think this check can be removed - it would be redundantly checked in QueueSerialization and it will rarely be true, so the extra code isn't saving anything in the normal case.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Mar 3, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I considered this originally and perhaps over optimizing for avoiding a function call. I'll remove.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/Modules/AnalysisCache.cs Outdated

@lzybkr Jason Shirk (lzybkr) Mar 3, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think this check can also be removed, though it wouldn't hurt to assert here.

The design is that nobody can call Serialize without going through QueueSerialization, so the check isn't needed here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Mar 3, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will remove and add assert

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/Modules/AnalysisCache.cs Outdated

@lzybkr Jason Shirk (lzybkr) Mar 3, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I haven't thought through the implications - but should we have a fallback? e.g. $env:TEMP?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Mar 3, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This usage is an edge case, I'm not sure how much we want to optimize particularly as one can use the XDG_*_HOME env vars. Also, I don't think it's guaranteed the user account has write access to $env:TEMP so we'd have to check that anyways.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/CoreCLR/CorePsPlatform.cs Outdated

@lzybkr Jason Shirk (lzybkr) Mar 3, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

What sort of contract should this api have? If the directory can't be used, should this api return null instead?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Mar 3, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Path.Combine() is used often with this api and would have a problem with null. My thinking was that this api returns the expected path which may be used in error message, so even returning an empty string may produce undesired results.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw Dongbo Wang (daxian-dbw) assigned Jason Shirk (lzybkr) Mar 3, 2017
@SteveL-MSFT
Allow Directory Creation failures to continue for Service Account use…
aa2edb7 
… scenarios that don't have home directories

Removed redundant checks if saving to analysis cache
Updated test to avoid race condition where a module is added in between checks

addresses PowerShell#3011

Removed unnecessary check to save to disk
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the serviceaccount branch from 5fdcbd2 to aa2edb7 Compare March 3, 2017 21:46
lzybkr
Jason Shirk (lzybkr) approved these changes Mar 6, 2017
View reviewed changes
@lzybkr
Jason Shirk (lzybkr) merged commit a2687bf into PowerShell:master Mar 6, 2017
@SteveL-MSFT
Steve Lee (SteveL-MSFT) deleted the serviceaccount branch March 21, 2017 19:53
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@SteveL-MSFT @lzybkr
Allow profile directory creation failures for Service Account scenari…
7e6209d 
…os (PowerShell#3244)

XDG profile directory creation can fail for accounts that do not have home directories.

The module analysis was trying to persist it's cache in an XDG profile directory.
The cache is less critical than it once was, so it's reasonable to not cache if there is no good place to do so.

Fixes PowerShell#3011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@lzybkr Jason Shirk (lzybkr) lzybkr approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@lzybkr Jason Shirk (lzybkr)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@SteveL-MSFT @lzybkr @msftclas
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.