Skip to content

Conversation

@anmenaga
Copy link

@anmenaga anmenaga commented Jun 5, 2017

This addresses #3789
Currently New-ModuleManifest creates psd1 manifests in UTF-16 with BOM; this creates problems for Linux tools and Git diff'er.
This fix changes encoding of New-ModuleManifest to be UTF8 (no BOM).
Also mentioned New-ModuleManifest in "RFC 0020 DefaultFileEncoding".

this,
filePath,
EncodingConversion.Unicode,
new UTF8Encoding(false), // UTF-8, no BOM
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to update the documentation for this change?

Copy link
Member

Choose a reason for hiding this comment

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

@anmenaga says he opened a doc issue, can you link it here?

Copy link
Author

Choose a reason for hiding this comment

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

This PR in RFC repo.

@joeyaiello joeyaiello added the Breaking-Change breaking change that may affect users label Jun 5, 2017
@joeyaiello
Copy link
Contributor

As it currently stands, this is a breaking change (appropriate label added).

Based on our current POR, we're okay changing the default on non-Windows platforms, but this shouldn't be changed (yet) on Windows until we've had the breaking change conversation. /cc @JamesWTruher

@joeyaiello
Copy link
Contributor

I understand that this currently breaks the git scenario, we just have to be thoughtful about this.

@vors
Copy link
Collaborator

vors commented Jun 6, 2017

Are there plans to add UTF8-NoBom to powershell supported Encodings?
I.e. for Set-Content and friends.

Changing the most important scenarios is good, but it would be great to address the problem with encodings broader and consistently to help shape the conversation about breaking changes.

@iSazonov
Copy link
Collaborator

iSazonov commented Jun 6, 2017

@vors UTF8noBOM is discussed in RFC 0020 DefaultFileEncoding.

@iSazonov
Copy link
Collaborator

iSazonov commented Jun 6, 2017

New-PSSessionConfigurationFile uses EncodingConversion.Unicode too.

@joeyaiello
Copy link
Contributor

@vors: first, yes. We're adding that (and making it the default across the board on Linux). The hard part is what to do with Windows (change the defaults? leave a legacy enum value for the "traditional" disparate defaults?) Which is why I want to have the discussion.

@anmenaga
Copy link
Author

anmenaga commented Jun 6, 2017

@joeyaiello good point;
PR updated to change encoding only for non-Windows.

@anmenaga anmenaga changed the title Change New-ModuleManifest encoding to UTF8NoBOM Change New-ModuleManifest encoding to UTF8NoBOM on non-Windows platforms Jun 6, 2017
@SteveL-MSFT
Copy link
Member

Is there an actual problem on Windows using UTF8NoBOM? If only English is used, it's equivalent to an ascii file and Windows tools handle that just fine and I expect that to be the majority of use.

@TravisEz13
Copy link
Member

Restarted AppVeyor due to MyGet connectivity issue

@joeyaiello
Copy link
Contributor

@anmenaga if you could sync with @JamesWTruher before merging this, that'd be awesome. Assuming the @PowerShell/powershell-committee votes to accept tomorrow, we want to move to UTF8NoBOM everywhere.

@iSazonov
Copy link
Collaborator

iSazonov commented Jun 7, 2017

@SteveL-MSFT

Is there an actual problem on Windows using UTF8NoBOM?

Users can add non-English comments in the manifest.

@SteveL-MSFT
Copy link
Member

@iSazonov absolutely, but I believe the majority of usage is English. Even with non-ascii characters, my understanding is that notepad has heuristics to determine encoding to display it correctly. VSCode seems to support UTF8NoBom just fine.

@iSazonov
Copy link
Collaborator

iSazonov commented Jun 7, 2017

notepad has heuristics to determine encoding to display it correctly

It seem works on last versions only not on Windows 7/8(?).

@mirichmo mirichmo self-assigned this Jun 7, 2017
@TravisEz13
Copy link
Member

@joeyaiello Are you saying this is blocked on a sign off from @JamesWTruher ?

@TravisEz13
Copy link
Member

@joeyaiello @JamesWTruher Do we want to document the output format of Cmdlets?

@joeyaiello
Copy link
Contributor

@TravisEz13 yes and yes. I think the uber-point is that @JamesWTruher is doing so much work here, that it might get changed anyway (and it might just muddle his extensive notes).

Ultimately up to him, though. I just wanted to make sure everyone was on the same page.

Regarding docs though: heck yes. We'll have a Legacy value for Encoding, and that will be well-documented.

Copy link
Collaborator

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

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

This is fine for now, I'll be changing all the cmdlets which provide for encoding as part of https://github.com/PowerShell/PowerShell-RFC/blob/master/1-Draft/RFC0020-DefaultFileEncoding.md. This doesn't muddy the waters for me.

@TravisEz13 TravisEz13 added the Documentation Needed in this repo Documentation is needed in this repo label Jun 13, 2017
(Get-Content -Encoding Byte -Path $testModulePath -TotalCount $expected.Length) -join ',' | Should Be ($expected -join ',')
}

It "Verify module manifest encoding on Windows " -Skip:(-not $IsWindows) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you just calculate the expected value in a beforeall then have one test for all platforms?

Copy link
Author

Choose a reason for hiding this comment

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

Good idea; updated.

Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

Please respond to my comment on your code.

@TravisEz13
Copy link
Member

@anmenaga Also, as a PowerShell team member, you need to submit the PR to update the docs before I merge this. Please add a link to the PR here. Thanks.

@iSazonov
Copy link
Collaborator

For encoding changes, you might want to delay documenting as we expect big changes in this.

@anmenaga
Copy link
Author

@TravisEz13

  1. This PR in RFC repo was merged;
  2. I've ran a search in PowerShell-Docs and didn't find any mentioning of encoding for New-ModuleManifest, so there is nothing to update there. Besides, this PR will be anyway overwritten by Jim's encoding work that is arriving soon.

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

We should update https://github.com/PowerShell/PowerShell/blob/master/docs/FAQ.md with the recommended settings in VSCode for automatic removing of trailing whitespace for resx files

@SteveL-MSFT SteveL-MSFT dismissed their stale review June 19, 2017 15:58

wrong PR

@TravisEz13
Copy link
Member

@anmenaga Per the conversation in this PR, we want to add encoding documentation to the CmdLet docs.

@mirichmo mirichmo assigned TravisEz13 and unassigned mirichmo Jun 20, 2017
@anmenaga
Copy link
Author

@TravisEz13 Here is the doc PR: MicrosoftDocs/PowerShell-Docs#1405

@kilasuit
Copy link
Collaborator

My only issue here is that although this is a change that I agree with, I think that this is where I have concerns of this causing a disparate and inconsistent behaviour between all versions of PowerShell below v6 due to the fact that New-ModuleManifest is baked into the Microsoft.PowerShell.Core module.

I personally would much prefer to see that this update to the cmdlet gets released in a manner that fully supports being used on downlevel systems where PowerShell v6 is unable to be installed.

However the only real solutions that I can really see available to be able to make that to happen would be to

  • Investigate the decoupling of a number of the cmdlets from the Microsoft.PowerShell.Core module into their own separate individual modules or a condensed updated singular module that has all the updates to any module in Microsoft.PowerShell.Core (not an overly fantastic solution) - similar in the idea put forward in Decouple the bundled modules to seperate Repo's #1979
  • Look to recreate this cmdlet within the PowerShellGet module.
  • Alongside the v6 updates look to release a completely updated and downlevel compatible version of the Microsoft.PowerShell.Core module to the PowerShell Gallery, which isn't really an overly viable solution either.

If this change could be easily replicated for downlevel versions of PowerShell I think that would be the best outcome all around.

@TravisEz13
Copy link
Member

@kilasuit Thanks for the feedback. I think most of the concerns raised would be best tracked in #1979 or a new issue.

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

Labels

Breaking-Change breaking change that may affect users

Projects

None yet

Development

Successfully merging this pull request may close these issues.