Closed Bug 1606970 Opened 6 months ago Closed 5 months ago

nodejs/eslint and python 3 do not play nice on Windows on more recent mozilla-nightly + updated nodejs ( UnicodeDecodeError: 'charmap' codec can't decode byte 0x8f in position 183: character maps to <undefined> )

Categories

(Firefox Build System :: Lint and Formatting, defect, P2)

Unspecified
Windows 10
defect

Tracking

(firefox75 fixed)

RESOLVED FIXED
mozilla75
Tracking Status
firefox75 --- fixed

People

(Reporter: Gijs, Assigned: gbrown)

References

(Blocks 1 open bug)

Details

(Whiteboard: dev-prod-2020)

Attachments

(1 file)

$ hg amend toolkit/components/
error: problem with lint setup, skipping android-api-lint, android-checkstyle, android-javadoc, android-lint, android-test
Exception in thread ProcessReaderStdout:
Traceback (most recent call last):
  File "path\to\mozilla-build\python3\lib\threading.py", line 916, in _bootstrap_inner
    self.run()
  File "path\to\mozilla-build\python3\lib\threading.py", line 864, in run
    self._target(*self._args, **self._kwargs)
  File "path\to\mozilla-central\testing/mozbase/mozprocess\mozprocess\processhandler.py", line 1030, in _read_stream
    line = stream.readline()
  File "path\to\mozilla-build\python3\lib\encodings\cp1252.py", line 23, in decode
    return codecs.charmap_decode(input,self.errors,decoding_table)[0]
UnicodeDecodeError: 'charmap' codec can't decode byte 0x8f in position 183: character maps to <undefined>

This looks like bug 1585565 but I haven't yet figured out exactly what is/was making this happen. I had to kill nodejs to make the amend complete - the first time, I used ctrl-c to kill things and then had to sit for 30 minutes while hg recover ran before I could use the repo again, because the amend got interrupted.

It looks like adding a space in toolkit/components/reputationservice/nsIApplicationReputationService.idl (which is the file I wanted to amend) and then re-running hg amend toolkit/components reliably triggers this for me, but running ./mach eslint toolkit/components/reputationservice/nsIApplicationReputationService.idl does not, so I don't know what's up. But given that killing nodejs fixes things, I assume it's still eslint...

This is a slightly older mozilla-build, with python3 3.6.5, nodejs 8.11.1 in my path and nodejs 8.17.0 in ~/.mozbuild/ (unsure which it's using here).

Anyway, given that it's breaking commits this is pretty annoying...

Summary: nodejs/eslint and python 3 do not play nice on Windows on more recent mozilla-nightly + updated nodejs modules → nodejs/eslint and python 3 do not play nice on Windows on more recent mozilla-nightly + updated nodejs ( UnicodeDecodeError: 'charmap' codec can't decode byte 0x8f in position 183: character maps to <undefined> )

Oh, looks like this might be because I have another modified file in my working directory that is a JS file, and lint for partial commits/amends isn't restricted to the committed files somehow. Running eslint directly on that file does produce an error:

$ ./mach eslint toolkit/mozapps/downloads/HelperAppDlg.jsm
Exception in thread ProcessReaderStdout:
Traceback (most recent call last):
  File "d:\mozilla-build\python3\lib\threading.py", line 916, in _bootstrap_inner
    self.run()
  File "d:\mozilla-build\python3\lib\threading.py", line 864, in run
    self._target(*self._args, **self._kwargs)
  File "d:\mozilla-central\testing/mozbase/mozprocess\mozprocess\processhandler.py", line 1030, in _read_stream
    line = stream.readline()
  File "d:\mozilla-build\python3\lib\encodings\cp1252.py", line 23, in decode
    return codecs.charmap_decode(input,self.errors,decoding_table)[0]
UnicodeDecodeError: 'charmap' codec can't decode byte 0x8f in position 726: character maps to <undefined>

Still not 100% clear on how to get more details out of that off-hand though...

Something like this works (based on https://stackoverflow.com/a/33470043 ) but is probably not the Right solution... Andrew, can you help suggest what is the right fix here?

FWIW, that still ends me up with:

  531:71  error  Replace `suggestedFileName` with `[?]········suggestedFileName[?]······`  prettier/prettier (eslint)

(ie garbage instead of the "newline arrows" that prettier likes to use) in my terminal... but that's preferable over it breaking entirely!

Flags: needinfo?(ahal)

Yeah, it definitely can't go in mozprocess because that solution uses Python 3-only syntax. Though you could try passing in stdout/stderr here:
https://searchfox.org/mozilla-central/rev/9b99e1d9c6cf83539674cb016c7373f549ba59ca/tools/lint/eslint/__init__.py#91

Though even then I'd be slightly wary of something like this causing new issues for other people. What version of Python are you using? You can add import sys; print(sys.version_info) if you are unsure. I saw some reports on SO saying that upgrading from 3.5 -> 3.6 fixed the problem.. maybe this is something we can fix by updating mozilla-build or something.

Finally, as a temporary workaround you might be able to use ./mach eslint -f unix (or any of the other formatters) to avoid printing unicode characters. Though I guess if eslint is generating unicode in its messages, this might not help.

Flags: needinfo?(ahal)

(In reply to Andrew Halberstadt [:ahal] from comment #3)

Though even then I'd be slightly wary of something like this causing new issues for other people. What version of Python are you using? You can add import sys; print(sys.version_info) if you are unsure. I saw some reports on SO saying that upgrading from 3.5 -> 3.6 fixed the problem.. maybe this is something we can fix by updating mozilla-build or something.

sys.version_info(major=3, minor=6, micro=5, releaselevel='final', serial=0)

So I'm already on 3.6...

(In reply to Andrew Halberstadt [:ahal] from comment #3)

Yeah, it definitely can't go in mozprocess because that solution uses Python 3-only syntax. Though you could try passing in stdout/stderr here:
https://searchfox.org/mozilla-central/rev/9b99e1d9c6cf83539674cb016c7373f549ba59ca/tools/lint/eslint/__init__.py#91

Sorry, I looked at this for a bit but I'm confused. That's creating an instance of ProcessHandler, but I don't see stdout or stderr in the arguments for either https://searchfox.org/mozilla-central/rev/a92ed79b0bc746159fc31af1586adbfa9e45e264/testing/mozbase/mozprocess/mozprocess/processhandler.py#703 or https://searchfox.org/mozilla-central/rev/a92ed79b0bc746159fc31af1586adbfa9e45e264/testing/mozbase/mozprocess/mozprocess/processhandler.py#1178 . Or are you saying we should add those arguments?

Flags: needinfo?(ahal)

FWIW, given there hasn't been a rush of dupes or CCs/votes and I also ran into bug 1607517, it's totally possible there is some weirdness about my setup. The problem is, I don't remember doing anything peculiar about charsets and python, and I have no idea how to figure out what, if anything, is peculiar about my setup - I don't normally write python myself, I just consume it as part of our infrastructure. If there is something I should check in my environment or windows that could be responsible here then I'm more than happy to do some digging.

It's super not obvious, but the **kwargs on ProcessHandler get saved to a self.keywordargs variable, which then gets passed down to the Proc class. So you should be able to pass stdout=<stream> into the ProcessHandler constructor. Though I haven't tested it myself, was just going by visual inspection.

I sort of suspect something in your environment as well, though even then our tools should be able to handle edge cases like that.. so would be good to fix either way. Are you just using the default mozilla-build shell?

If it turns out that this is something we want to solve in mozprocess (i.e, it's a legit widespread issue) we could take your initial patch but put it behind an if six.PY3 statement. Or potentially use io.open instead of open. I'm not really sure where this should get fixed tbh.

Flags: needinfo?(ahal)

(In reply to Andrew Halberstadt [:ahal] from comment #7)

I sort of suspect something in your environment as well, though even then our tools should be able to handle edge cases like that.. so would be good to fix either way. Are you just using the default mozilla-build shell?

Yes, but it's being run in conemu. I just checked and I get the same error when loading the bat file directly though, so that's probably not a factor.

My mozilla-build version is slightly out-dated, I'm on 3.2 and the latest is 3.3, which then has Python 3.7.4, apparently... it's not clear to me off-hand if/why updating would fix these issues, though I guess I could try it if you think it's likely to make a difference?

I am experiencing the same problem:

  • UnicodeDecodeError: 'charmap' codec can't decode byte 0x8f in position 183: character maps to <undefined>

When this error occurs, it is followed by the following errors:

  • OSError: IO Completion Port failed to signal process shutdown
  • A failure occurred in the eslint linter.

I don't understand why 'mach eslint' works correctly with many files, but fails with this error on some files.

Editing the failed files (e.g. removing or or re-ordering or shortening a few lines) sometimes avoids the problem.

The priority flag is not set for this bug.
:ahal, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(ahal)

(In reply to Andrew Halberstadt [:ahal] from comment #7)

It's super not obvious, but the **kwargs on ProcessHandler get saved to a self.keywordargs variable, which then gets passed down to the Proc class. So you should be able to pass stdout=<stream> into the ProcessHandler constructor. Though I haven't tested it myself, was just going by visual inspection.

I spent some more time on this, but I got a bit lost.

First off, what I seem to have been doing in the dummy patch in comment 7 is taking the created-process's (proc in https://searchfox.org/mozilla-central/rev/e076e40ab1290f4e5e67ebd21dc8af753fc05be6/testing/mozbase/mozprocess/mozprocess/processhandler.py#1039-1043 ) stdout/stderr and ensuring it gets read as utf-8 by the code in processhandler.

The stdout and stderr arguments, as far as I can tell, end up being passed to subprocess.popen which wants a file descriptor - I tried to use io.BytesIO or similar and then I get an error like:

  File "path\to\mozilla-central\tools\lint\eslint\__init__.py", line 97, in lint
    proc.run()
  File "path\to\mozilla-central\testing/mozbase/mozprocess\mozprocess\processhandler.py", line 811, in run
    self.proc = self.Process([self.cmd] + self.args, **args)
  File "path\to\mozilla-central\testing/mozbase/mozprocess\mozprocess\processhandler.py", line 123, in __init__
    universal_newlines, startupinfo, creationflags)
  File "path\to\mozilla-build\python3\lib\subprocess.py", line 667, in __init__
    errread, errwrite) = self._get_handles(stdin, stdout, stderr)
  File "path\to\mozilla-build\python3\lib\subprocess.py", line 922, in _get_handles
    c2pwrite = msvcrt.get_osfhandle(stdout.fileno())
io.UnsupportedOperation: fileno

I looked into creating a custom pipe instance, but it seems then you get no control over the encoding (os.pipe() seems to take no arguments?). Using a temporary file felt very icky so I didn't really want to go there...

I also tried overwriting the right stdout before calling proc.run from the outside (ie from the eslint file, after creating the ProcessHandler instance), but that also doesn't seem to be possible, because the ProcessHandler doesn't return control between creating proc and calling processOutput which starts the ProcessReader.

I'm probably missing something else here - but if not, I'm happy to write a patch for:

(In reply to Andrew Halberstadt [:ahal] from comment #7)

If it turns out that this is something we want to solve in mozprocess (i.e, it's a legit widespread issue) we could take your initial patch but put it behind an if six.PY3 statement. Or potentially use io.open instead of open. I'm not really sure where this should get fixed tbh.

if that makes sense?

Sorry for the slow replies here. The truth is I'm not sure off-hand why this is happening and don't have bandwidth to dig in much before the all-hands. Then I'm going to be taking vacation afterwards :/.

So if we can find a quick and dirty workaround (i.e in tools/lint/eslint/__init__.py rather than mozprocess), I'd vote we get that landed first and spend time figuring out the underlying issue after. If the approach that you came up with in comment 2 turns out to be a dead end, we could likely even stop using mozprocess in our eslint integration entirely, and just revert to normal subprocess (assuming the bug is in mozprocess). I'm not even really sure why we don't do that already.

I might be able to get something together at the all-hands. I'll leave the needinfo on myself for now as a reminder.

Priority: -- → P2

And very predictably the time to look into this never materialized at the all hands. I've talked about this with :gbrown and he's agreed to take a look while I'm on PTO. We agree in this particular instance it makes sense to simply stop using mozprocess.. though of course this will then likely happen again as soon as the next Python 3 thing uses mozprocess with unicode. So assuming we go the subprocess route, we should leave-open and morph this bug to solve the general case (or file a new one).

Flags: needinfo?(ahal) → needinfo?(gbrown)

:Gijs - Please check whether this patch fixes the issue for you.

Assignee: nobody → gbrown
Flags: needinfo?(gbrown) → needinfo?(gijskruitbosch+bugs)

Apologies for the delay; although I brought a windows device to all-hands, it was an ARM64 one and it had other issues trying to get any of our build things going on it...

Now that I'm back home, I've tried the patch and unfortunately, it doesn't seem to help. I now see:

Traceback (most recent call last):
  File "path\to\mozilla-central\python/mozlint\mozlint\roller.py", line 55, in _run_worker
    res = func(paths, config, **lintargs) or []
  File "path\to\mozilla-central\python/mozlint\mozlint\types.py", line 54, in __call__
    return self._lint(paths, config, **lintargs)
  File "path\to\mozilla-central\python/mozlint\mozlint\types.py", line 139, in _lint
    return func(files, config, **lintargs)
  File "path\to\mozilla-central\tools\lint\eslint\__init__.py", line 97, in lint
    output, _ = proc.communicate()
  File "path\to\mozilla-build\python3\lib\subprocess.py", line 830, in communicate
    stdout = self.stdout.read()
  File "path\to\mozilla-build\python3\lib\encodings\cp1252.py", line 23, in decode
    return codecs.charmap_decode(input,self.errors,decoding_table)[0]
UnicodeDecodeError: 'charmap' codec can't decode byte 0x8f in position 157: character maps to <undefined>
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gbrown)
Flags: needinfo?(gbrown)

:Gijs - I have updated the patch; give it another try? Sorry to keep asking, but I can't reproduce the problem and I want to make sure it is fixed for you.

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Geoff Brown [:gbrown] from comment #18)

:Gijs - I have updated the patch; give it another try? Sorry to keep asking, but I can't reproduce the problem and I want to make sure it is fixed for you.

No worries - yes, the current version of the patch seems to fix this. That is, I get actual output rather than UnicodeDecodeErrors or whatnot.

Admittedly, I get things like "Delete [?] prettier/prettier (eslint)" - it looks like there's no way to encode the stuff that prettier is outputting in cp-1252. But the internet also suggests that for me to try to workaround this by changing my console encoding to utf-8 by default in Windows is a Bad Idea (cf. https://superuser.com/questions/269818/change-default-code-page-of-windows-console-to-utf-8#comment1627713_269818 ). I dunno if within a mozilla-build shell, there is some other option - chcp just gets me "command not found" errors, but none of the usual LANG/LC_ALL/LC_CTYPE env vars are defined either, so I have no idea how the mozilla-build shell knows what the encoding is or how to change it.

Flags: needinfo?(gijskruitbosch+bugs)
See Also: → 1615223
Whiteboard: dev-prod-2020

(In reply to :Gijs (he/him) from comment #19)

Admittedly, I get things like "Delete [?] prettier/prettier (eslint)" - it looks like there's no way to encode the stuff that prettier is outputting in cp-1252.

I have a Windows mozilla-build environment now and have reproduced the issues you have reported - yeah! As you say, I can't find a way to display special chars from prettier, like the prettier newline, in the mozilla-build console.

But the internet also suggests that for me to try to workaround this by changing my console encoding to utf-8 by default in Windows is a Bad Idea (cf. https://superuser.com/questions/269818/change-default-code-page-of-windows-console-to-utf-8#comment1627713_269818 ). I dunno if within a mozilla-build shell, there is some other option - chcp just gets me "command not found" errors, but none of the usual LANG/LC_ALL/LC_CTYPE env vars are defined either, so I have no idea how the mozilla-build shell knows what the encoding is or how to change it.

I had the same experience, but eventually found I could run chcp in mozilla-build with the complete path, including extension: 'c:\windows\system32\chcp.com 65001'. Unfortunately, that did not seem to help anything.

Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3049e5160743
Use subprocess instead of mozprocess to run eslint; r=ahal
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
You need to log in before you can comment on or make changes to this bug.