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

let_users_change_the_code_page #58008

Merged
merged 21 commits into from Sep 9, 2020
Merged

Conversation

cmcmarrow
Copy link
Contributor

What does this PR do?
Lets cmd/powershell go to utf-8
#57901
#57515

Commits signed with GPG?
Yes

@cmcmarrow cmcmarrow requested a review from a team as a code owner July 22, 2020 22:30
@ghost ghost requested review from DmitryKuzmenko and removed request for a team July 22, 2020 22:30
@cmcmarrow cmcmarrow added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jul 22, 2020
@marbx
Copy link
Contributor

marbx commented Jul 23, 2020

Please remove lines 607-610 that prepend chcp 437 > nul & to the command

        else:
            # On Windows set the codepage to US English.
            if python_shell:
                cmd = "chcp 437 > nul & " + cmd

image

@marbx
Copy link
Contributor

marbx commented Jul 23, 2020

Function chcp() returns a string, chcp_code is an integer.

chcp(chcp_code) != chcp_code

is always true

Please change to

chcp(chcp_code) != str(chcp_code) 

@marbx
Copy link
Contributor

marbx commented Jul 23, 2020

As negative test, I replaced chcp_code=65001 by chcp_code=437 and got the double quotes instead of ä

marbx added a commit to marbx/LearnSalt that referenced this pull request Jul 23, 2020
@marbx
Copy link
Contributor

marbx commented Jul 23, 2020

@marbx
Copy link
Contributor

marbx commented Jul 23, 2020

@cmcmarrow looks good for me

Could you please take over my changes, I would like to mass rollout a version we agree upon.

@cmcmarrow
Copy link
Contributor Author

@markuskramerlgitt I’ll take over your changes and add this feature and add it to the startup of salt.

DmitryKuzmenko
DmitryKuzmenko previously approved these changes Jul 24, 2020
@cmcmarrow cmcmarrow added Magnesium Mg release after Na prior to Al and removed Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases has-failing-test labels Jul 27, 2020
@marbx
Copy link
Contributor

marbx commented Aug 6, 2020

@cmcmarrow I patched my 3001 Minions with these files which are taken from this PR

salt/utils/chcp.py Outdated Show resolved Hide resolved
@twangboy twangboy self-requested a review August 28, 2020 01:39
@dwoz dwoz merged commit 4805aad into saltstack:master Sep 9, 2020
28 checks passed
@s0undt3ch
Copy link
Member

This needs to be partially reverted.
It was correct to use chcp xxx & cmd, we want to tweak the code page of the command we're going to run, but not necessarily, all other commands after that one.

As for changing code page when salt starts, seen in a comment but not in any code. The admin should be responsible for properly setting his machine, and, if we already support changing the code page to a command we want to run, we're good, by we, I mean salt.

We could check the current code page in order not to chain the chcp code page when running the command, but that means an extra shell out per command.

I took a stab at trying to reuse this code, the right way, setting code page, running command, resetting codepage, but that meant 3 shells to run a single command, not good.

Thoughts? Alternatives? Why did we actually needed to shell out to set the code page in the first place and opposed to chaining the commands?

@s0undt3ch
Copy link
Member

Actually, now that I think about this a bit more, even chaining the chcp command will change the code page for the following commands.....
We should instead do something like chcp xxx > nul & the actual command & chcp previous_xxx > nul
How can we grab the current chcp in a variable to reset at the end? Using command chaining....

@twangboy
Copy link
Contributor

@s0undt3ch There was a lot of discussion about why we did this here: #57515

@s0undt3ch
Copy link
Member

Thanks @twangboy, read it.
I still think the merged PR, when setting the codepage, should set it back to how it was.
The system should be properly configured in the first place. At most, our windows bat scripts should do the codepage change at the start of each of our CLIs, not salt.
Anyway, #58478 seems to correct this behavior, but I'll try testing with the data from the issue you linked.

@marbx
Copy link
Contributor

marbx commented Sep 28, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-failing-test Magnesium Mg release after Na prior to Al
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants