Unix-only crypt should not be present on Windows. #69359
Comments
|
Except for crypt, all the modules labeled 'Unix' or 'Linux' on the module index https://docs.python.org/3/py-modindex.html are absent from /Lib on Windows. 'import xyz' fails with "ImportError: no module named 'zyz'". (I presume the same is true on unix for Windows-only modules.) However, crypt is present, and 'import crypt' fails with "...'_crypt'", leading one to think that the C accelerator should be present but is not. Assuming that _crypt should (cannot) not be present, please add crypt to the list of modules omitted from the Windows installer, however this is done. And if 'unix-only' is obsolete and _crypt should be present, please fix this instead. ;-) This is 3.x issue only, as crypt is not present in my Windows 2.7.10 /Lib. The fact that is was somehow added to some 3.x prompted me to look and see if there is anything useful without _crypt present. I think not. |
serhiy-storchaka
added
3.7
|
Marking this easy/newcomer friendly. This should catch the ModuleNotFoundError raised when _crypt is missing and raise a more informative ImportError saying that crypt is unsupported. All the other modules that are missing are native extension modules that are not built - this is the only one that is a .py file. We don't exclude any other Lib\*.py files from the distribution. |
|
Hello Thank you |
|
That sounds good to me, though you may want to propose the error message here first so we can get the wording right - probably not everyone will be watching github PR. It probably makes sense to raise a different error message on platforms where we do expect it to have been built - the same confusion could arise there. Some starting points (that will likely need improvement): win32: raise ImportError("The crypt module is not supported on Windows") not win32: raise ImportError("The required _crypt module was not built as part of CPython") |
|
What is the recommended way to check for platform in cpython? Is it using the sys module or os module? As for the error messages I couldn't think of anything better then what you suggested :). win32: raise ImportError("The crypt module is not supported on Windows") not win32: raise ImportError("The required _crypt module was not built as part of CPython") |
|
( |
|
Sounds good. Can I submit my PR now? |
|
I forgot to ask about tests? I see there is test_crypt.py under Lib\test. Do you have any thoughts on how to test this? |
Sure, though if others want to weigh in on wording they should feel free to do it here.
It should continue to "successfully" skip tests when _crypt isn't built. So as long as you raise ImportError, this should be fine. |
|
The proposed wording seems good to me. |
|
Sorry, I should have waited 1 day after submitting the CLA before submitting the PR. The system is waiting for my CLA to be cleared. |
|
It might be worth adding a test (running only on Windows) to confirm that if you try to import crypt you get an import error with a message that includes the phrase "not supported". That will ensure that we don't get regressions here in future, while still not tying us too strictly to the exact wording. |
|
How do I write tests which only run for a given platform? Looking at test_crypt.py, I see that the crypt module is imported using test.support module so the exception will be raised even before the tests run. Is there a way to test imports? |
|
I agree with Paul about the wording. Note that the proposed platform-specific catch and raise are for crypt.py, not test_crypt.py. If a test module should be skipped entirely, import_module is correct. For more refinement, test_idle has, for example, tk = import_module('tkinter') # Also imports _tkinter.
if tk.TkVersion < 8.5:
raise unittest.SkipTest("IDLE requires tk 8.5 or later.")Testing only continues if tkinter and _tkinter and tk >= 8.5. I presume that crypt = import_module('crypt') will only continue if crypt and _crypt, which is what you want. It is apparently not an error for _crypt to be missing on unix, just an inconvenience for people who expect it. FYI, Individual test classes and methods can be skipped with |
|
On reflection, adding a test for the Windows-specific behaviour looks like it could be more complex than I'd anticipated, so I'm happy for this to go in without an extra test. I'll look at adding a test in a follow-up PR (or if someone else wants to, that's fine as well). |
|
I am curious how one would create a test for this? Would this be a new test script? The new script would skip if platform not win32 and the test case would be to import the crypt module and check for "not supported" string in the ImportError? |
|
That's where I decided it was too complex for now :-) The whole test_crypt.py file is skipped if you can't import crypt, via the use of the test.support.import_module() function. To be able to write a test for a platform where the whole point is that you can't import crypt, you'd have to rewrite that. So you'd get something like try: But you can't use "assert" like that in top-level code (or at least I don't think you can - I'm not that familiar with unittest, but I think assertions have to live in classes in unittest). At this point I gave up. It starts to be quite a big rewrite for a relatively minor benefit. The alternative would be, if there was a "Windows-specific tests" test module, we could have put a test for this situation in there. But I don't think there is such a module. |
Maybe test___all__? |
|
Or better yet (since my last suggestion was bad), add a second test class to test_crypt try: @skipIf(crypt)
class TestWhyCryptDidNotImport(TestCase):
...
@skipUnless(crypt)
class CryptTestCase(TestCase):
... |
|
Are you suggesting using the try/except block instead of import_module in test_crypt.py |
|
Yes. support.import_module is going to raise a skip when the module can't be imported, but we want to handle ImportError differently. |
|
Can I have a go at it? Do I need to open a new issue to post a new PR? |
|
Sure. You can post a new PR with the same bug number (and it won't need a NEWS file this time). |
|
When I try to put the skipUnless decorator on CryptTestCase, I am still seeing a failure when I try to run this. The error is - Line 59 is the following decorator and method - I tried a simple @Skip decorator and that too fails on the same error. The class level skip is not helping here. Does it make sense to create a different test file for windows? |
|
I submitted a new PR for the Windows test case. Please take a look. Also how do I attach the label "skip news" to this new PR? |
|
I added the label for you. |
|
To clarify, I think adding the label needs core dev (or maybe triager) rights on github, so I don't think it's something you can do yourself. |
|
Thank you @paul.moore |
|
Thanks for the patches, Srinivas! |
|
Thank you Steve for accepting my pull requests. I was surprised to see the methods in the class and its decorators getting evaluated and causing the failures initially, but I then realized that the code is parsed before execution and that's when I was seeing the failure. |
|
test_crypt fails on android following last changes made at 243a73d. generic_x86_64:/data/local/tmp/python $ python
Python 3.9.0a0 (heads/abifa-dirty:cf805c25e6, Nov 18 2019, 16:40:26)
[Clang 8.0.2 (https://android.googlesource.com/toolchain/clang 40173bab62ec7462 on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import crypt
Traceback (most recent call last):
File "/data/local/tmp/python/lib/python3.9/crypt.py", line 6, in <module>
import _crypt
ModuleNotFoundError: No module named '_crypt'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/data/local/tmp/python/lib/python3.9/crypt.py", line 11, in <module>
raise ImportError("The required _crypt module was not built as part of CPython")
ImportError: The required _crypt module was not built as part of CPython
>>>generic_x86_64:/data/local/tmp/python $ python -m test -v test_crypt ====================================================================== Traceback (most recent call last):
File "/data/local/tmp/python/lib/python3.9/test/test_crypt.py", line 16, in test_failure_only_for_
windows
self.assertEqual(sys.platform, 'win32')
AssertionError: 'linux' != 'win32'
- linux
+ win32====================================================================== Traceback (most recent call last):
File "/data/local/tmp/python/lib/python3.9/test/test_crypt.py", line 19, in test_import_failure_message
self.assertIn('not supported', IMPORT_ERROR)
AssertionError: 'not supported' not found in 'The required _crypt module was not built as part of CPython'Ran 9 tests in 0.008s FAILED (failures=2, skipped=7) == Tests result: FAILURE == 1 test failed: Total duration: 165 ms |
|
Not interested anymore in android stuff. |
|
@steve.dower - Does this fix need more work for android? @xdegaye says he does not need this for android anymore. |
|
I started creating an example of improving the skip cases, and ended up just making a PR. It should:
|
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: