Skip to content

Add info about enabling projFS re: badimageformat#58

Merged
cgallred merged 2 commits into
microsoft:mainfrom
brphelps:patch-1
Jan 27, 2021
Merged

Add info about enabling projFS re: badimageformat#58
cgallred merged 2 commits into
microsoft:mainfrom
brphelps:patch-1

Conversation

@brphelps
Copy link
Copy Markdown
Contributor

No description provided.

@cgallred cgallred self-assigned this Jan 22, 2021
@cgallred cgallred requested a review from erikmav January 22, 2021 19:13
@cgallred
Copy link
Copy Markdown
Collaborator

@Erikma this changes some text in README.md that you added. Can you please have a look at this change?

@brphelps
Copy link
Copy Markdown
Contributor Author

@Erikma this changes some text in README.md that you added. Can you please have a look at this change?

This is necessary to get the nuget package to work on a windows machine, I suspect it's also necessary just to get the codebase to run on a fresh windows install at all.

Comment thread README.md Outdated
Comment on lines +74 to +76
then it's likely that a dependent assembly of this library is missing. The most common cause for BadImageFormatExceptions is that you haven't enabled the ProjFS windows component yet, which is now optional:
` Enable-WindowsOptionalFeature -Online -FeatureName Client-ProjFS -NoRestart `
([source](https://docs.microsoft.com/en-us/windows/win32/projfs/enabling-windows-projected-file-system))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, I misread this change at first. I mistakenly read it as changing what Erik wrote rather than adding to it.

The information about having to enable ProjFS is noted right above this section; see line 66 ("Note: The Windows Projected File System optional component must be enabled in Windows...").

Since we already tell the reader to enable ProjFS (and it has always been an optional component), I'd suggest changing the wording here:

Suggested change
then it's likely that a dependent assembly of this library is missing. The most common cause for BadImageFormatExceptions is that you haven't enabled the ProjFS windows component yet, which is now optional:
` Enable-WindowsOptionalFeature -Online -FeatureName Client-ProjFS -NoRestart `
([source](https://docs.microsoft.com/en-us/windows/win32/projfs/enabling-windows-projected-file-system))
then it's likely that a dependent assembly of this library is missing. The most common cause for BadImageFormatExceptions is that you haven't enabled the ProjFS optional component yet. See the note at the end of the previous section for more information.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Leaving someone a pointer to how to do that seems pretty straightforward -- I think ideally the library would throw an exception if the dependent feature is missing that explicitly says "You need to install projfs, go here to do so" so this type of doc wouldn't be necessary -- Also, I think the readme is a little too casual in its mention of proj fs being an optional component, it looks like it's ancillary information but it should be a strongly worded note that you need a prereq installed for this lib to work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(I think this is especially true because this is so uncommon for package dependencies)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think my proposed compromise here would be that we say "proj fs may not be installed, see prerequisites section" and more formally call out the prereq instead of it being the last line of the intro. Thoughts? I can propose via PR :D

Copy link
Copy Markdown
Contributor

@erikmav erikmav Jan 26, 2021

Choose a reason for hiding this comment

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

Generally having all installation info in one spot and referring to it from this section makes sense. I agree with @brphelps that it's a bit understated. How about combining the proposed change from @cgallred with converting he paragraph just above starting "Note: The Windows Projected File System optional component must be enabled..." to be a new H2 or H3 section with a title like "Installing the Windows Projected File System"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Works for me, I'll get some proposed changes out.

@cgallred cgallred merged commit 4ccce1d into microsoft:main Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants