Support linting when missing sem_open syscall - [merged] #1282

asottile · 2021-04-03T18:28:18Z

In GitLab by @nekokatt on Aug 27, 2020, 05:18

Merges bugfix/cpython-3770-semopen-missing -> master

Platforms such as Termux on Android, and other exotic devices
do not provide a sem_open implementation on the OS level. This
is problematic, as the error resulting from this occurs when
calling multiprocessing.Pool, throwing an unhandled ImportError.

The issue itself is outlined in https://bugs.python.org/issue3770.

This change allows devices missing this system call to respond
to the missing feature by falling back to synchronous execution,
which appears to be the default behaviour if the multiprocessing
module is not found.

This change also adds a potential fix for developers working
on platforms where multiprocessing itself cannot be imported.
The existing code would set the name referencing the import to
None, but there are no clear checks to ensure this does not
result in an AttributeError later when multiprocessing.Pool
has accession attempts.

Existing users should see no difference in functionality, as they
will assumably already be able to use flake8, so will not be
missing this sem_open call.

Users on devices without the sem_open call will now be able
to use flake8 where they would be unable to before due to
unhandled ImportErrors.

asottile · 2021-04-03T18:28:23Z

In GitLab by @codecov on Aug 27, 2020, 05:19

Codecov Report

Merging #448 into master will increase coverage by 0.01%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##           master     #448      +/-   ##
==========================================
+ Coverage   90.74%   90.75%   +0.01%     
==========================================
  Files          59       59              
  Lines        4193     4209      +16     
  Branches      417      419       +2     
==========================================
+ Hits         3805     3820      +15     
  Misses        335      335              
- Partials       53       54       +1

Impacted Files	Coverage Δ
src/flake8/checker.py	`85.48% <93.75%> (-0.05%)`	⬇️
tests/integration/test_checker.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abdc9b1...e6a5f6a. Read the comment docs.

asottile · 2021-04-03T18:28:26Z

In GitLab by @asottile on Aug 27, 2020, 06:57

can you show an actual failing output, I don't believe the patch is correct (catching too many exceptions, handling them incorrectly)

for instance, it is impossible for AttributeError to occur as far as I can tell (the linked bpo issue is 10+ years old and about versions of python we don't support)

asottile · 2021-04-03T18:28:28Z

In GitLab by @asottile on Aug 27, 2020, 07:09

Commented on tests/integration/test_checker.py line 300

you can use side_effect=ImportError(...) directly

asottile · 2021-04-03T18:28:31Z

In GitLab by @asottile on Aug 27, 2020, 07:09

Commented on src/flake8/checker.py line 342

this would be better as a free function not on self, also please type annotate new code

asottile · 2021-04-03T18:28:34Z

In GitLab by @asottile on Aug 27, 2020, 07:09

Commented on src/flake8/checker.py line 352

this should be a separate except clause

asottile · 2021-04-03T18:28:36Z

In GitLab by @nekokatt on Aug 27, 2020, 08:43

Is there a case where Manager.run does set the multiprocessing import or check it beforehand if it is None?

I assumed that

try:
    import multiprocessing
except ImportError:
    multiprocessing = None

...over at https://gitlab.com/pycqa/flake8/-/blob/681b2bd751d8a07a2a1776d9aed077411c43e7c7/src/flake8/checker.py#L11 was still a valid case that could occur; and since the run method is marked as part of the public interface, there is a risk that invoking that would result in unexpected behaviour.

Will upload a reproduction of the multiprocessing ImportError from my phone, as I can reproduce this on there. Give me five minutes :)

asottile · 2021-04-03T18:28:39Z

In GitLab by @nekokatt on Aug 27, 2020, 08:45

CPython 3.8.3.

They had the same problem in Black for a long while until recently which needed the same functional fix to resolve.

asottile · 2021-04-03T18:28:41Z

In GitLab by @nekokatt on Aug 27, 2020, 09:06

Commented on tests/integration/test_checker.py line 300

changed this line in version 2 of the diff

asottile · 2021-04-03T18:28:44Z

In GitLab by @nekokatt on Aug 27, 2020, 09:06

Commented on src/flake8/checker.py line 342

changed this line in version 2 of the diff

asottile · 2021-04-03T18:28:47Z

In GitLab by @nekokatt on Aug 27, 2020, 09:06

Commented on src/flake8/checker.py line 352

changed this line in version 2 of the diff

asottile · 2021-04-03T18:28:49Z

In GitLab by @nekokatt on Aug 27, 2020, 09:06

added 1 commit

a454e1dd - PR feedback from asottile.

Compare with previous version

asottile · 2021-04-03T18:28:52Z

In GitLab by @nekokatt on Aug 27, 2020, 09:12

Likewise with this patch:

asottile · 2021-04-03T18:28:55Z

In GitLab by @nekokatt on Aug 27, 2020, 09:17

added 1 commit

caf5867 - PR feedback from asottile.

Compare with previous version

asottile · 2021-04-03T18:28:57Z

In GitLab by @nekokatt on Aug 27, 2020, 09:22

added 1 commit

8ef32de - PR feedback from asottile.

Compare with previous version

asottile · 2021-04-03T18:29:00Z

In GitLab by @nekokatt on Aug 27, 2020, 09:24

Amended suggested changes, ensured new code runs under mypy --strict without erroring on the range of lines in the files that have changed.

Verified that bug is resolved on CPython 3.8.3 ARMv7.

asottile · 2021-04-03T18:29:03Z

In GitLab by @nekokatt on Aug 27, 2020, 09:35

added 1 commit

71fae997 - Amended typehint for final_statistics in checker.py.

Compare with previous version

asottile · 2021-04-03T18:29:06Z

In GitLab by @nekokatt on Aug 27, 2020, 09:39

added 1 commit

d74d7f2 - Amended typehint for final_statistics in checker.py.

Compare with previous version

asottile · 2021-04-03T18:29:08Z

In GitLab by @asottile on Aug 27, 2020, 10:52

Commented on src/flake8/checker.py line 318

probably can't change the signature of this function, can you go back to your original approach?

asottile · 2021-04-03T18:29:11Z

In GitLab by @asottile on Aug 27, 2020, 10:52

Commented on src/flake8/checker.py line 646

as I said before, let's keep the change to just your change and not AttributeError

it's possible / likely that the AttributeError case is impossible given it hasn't been "correct" for so long (this may be code that's left over from python2.4 or something)

asottile · 2021-04-03T18:29:14Z

In GitLab by @asottile on Aug 27, 2020, 10:52

Commented on src/flake8/checker.py line 655

this function should not have side-effects

asottile · 2021-04-03T18:29:16Z

In GitLab by @asottile on Aug 27, 2020, 10:52

Commented on tests/integration/test_checker.py line 266

no need to set return_value here, it will be populated automatically (and you can access it with pool.return_value

asottile · 2021-04-03T18:29:19Z

In GitLab by @asottile on Aug 27, 2020, 10:53

just because I'm curious, does concurrent.futures suffer from the same limitation?

asottile · 2021-04-03T18:29:21Z

In GitLab by @nekokatt on Aug 27, 2020, 14:36

yeah, it does; you can't use a process pool executor either sadly.

The issue is that multiprocessing.Pool is actually just a function in the BaseContext class. Seems that each method is named after a class (e.g. Semaphore, Queue, Pool, etc).

Each one first imports the synchronization submodule. This is where the ImportError is coming from.

concurrent.futures is importing multiprocessing.Queue from a glance.

asottile · 2021-04-03T18:29:24Z

In GitLab by @nekokatt on Aug 27, 2020, 14:36

Commented on src/flake8/checker.py line 655

How would you suggest structuring this best?

asottile · 2021-04-03T18:29:27Z

In GitLab by @asottile on Aug 27, 2020, 14:40

Commented on src/flake8/checker.py line 655

either remove the log line entirely or put it where is none: do serial happens

asottile · 2021-04-03T18:29:30Z

In GitLab by @nekokatt on Aug 27, 2020, 14:40

Commented on src/flake8/checker.py line 318

changed this line in version 7 of the diff

asottile · 2021-04-03T18:29:32Z

In GitLab by @nekokatt on Aug 27, 2020, 14:40

Commented on src/flake8/checker.py line 646

changed this line in version 7 of the diff

asottile · 2021-04-03T18:29:35Z

In GitLab by @nekokatt on Aug 27, 2020, 14:40

Commented on src/flake8/checker.py line 655

changed this line in version 7 of the diff

asottile · 2021-04-03T18:29:37Z

In GitLab by @nekokatt on Aug 27, 2020, 14:40

added 1 commit

6f7ed86 - PR feedback on structuring.

Compare with previous version

asottile · 2021-04-03T18:29:40Z

In GitLab by @nekokatt on Aug 27, 2020, 15:07

Commented on tests/integration/test_checker.py line 266

changed this line in version 8 of the diff

asottile · 2021-04-03T18:29:43Z

In GitLab by @nekokatt on Aug 27, 2020, 15:07

added 1 commit

77454afd - PR feedback on structuring.

Compare with previous version

asottile · 2021-04-03T18:29:45Z

In GitLab by @nekokatt on Aug 27, 2020, 15:08

Commented on src/flake8/checker.py line 655

amended. Thanks.

asottile · 2021-04-03T18:29:48Z

In GitLab by @nekokatt on Aug 27, 2020, 15:08

Commented on src/flake8/checker.py line 318

done

asottile · 2021-04-03T18:29:51Z

In GitLab by @nekokatt on Aug 27, 2020, 15:08

resolved all threads

asottile · 2021-04-03T18:29:53Z

In GitLab by @nekokatt on Aug 27, 2020, 15:09

Looks like everything else is resolved now. Let me know if there is any other feedback and I'll get on it ASAP tomorrow morning.

Cheers :)

asottile · 2021-04-03T18:29:56Z

In GitLab by @asottile on Aug 27, 2020, 15:39

added 3 commits

77454afd...abdc9b14 - 2 commits from branch pycqa:master
7e1bd950 - Support linting when missing sem_open syscall

Compare with previous version

asottile · 2021-04-03T18:29:59Z

In GitLab by @asottile on Aug 27, 2020, 15:41

added 1 commit

e6a5f6a - Support linting when missing sem_open syscall

Compare with previous version

asottile · 2021-04-03T18:30:01Z

In GitLab by @asottile on Aug 27, 2020, 15:42

approved this merge request

asottile · 2021-04-03T18:30:04Z

In GitLab by @asottile on Aug 27, 2020, 15:43

enabled an automatic merge when the pipeline for e6a5f6a succeeds

asottile · 2021-04-03T18:30:07Z

In GitLab by @asottile on Aug 27, 2020, 15:44

mentioned in commit 3765318

asottile added this to the 3.8.4 milestone Apr 3, 2021

asottile added the gitlab merge request label Apr 3, 2021

asottile closed this as completed Apr 3, 2021

Support linting when missing sem_open syscall - [merged] #1282

Support linting when missing sem_open syscall - [merged] #1282

Comments

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

Codecov Report

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021

asottile commented Apr 3, 2021