bpo-35802: Clean up code which checked presence of os.{stat,lstat,chmod} #11643

asottile · 2019-01-21T21:59:40Z

https://bugs.python.org/issue35802

pablogsal · 2019-01-21T22:14:22Z

Hummmm, I do not feel very comfortable removing these checks. Autotools actually is checking at least for lstat:

https://github.com/python/cpython/blob/master/configure.ac#L3505

asottile · 2019-01-21T22:17:18Z

Hummmm, I do not feel very comfortable removing these checks. Autotools actually is checking at least for lstat:

https://github.com/python/cpython/blob/master/configure.ac#L3505

Unless I'm reading incorrectly, it is unconditionally defined in the os module:

cpython/Modules/clinic/posixmodule.c.h

Lines 68 to 69 in 7a23680

    
           #define OS_LSTAT_METHODDEF    \ 
        
               {"lstat", (PyCFunction)(void(*)(void))os_lstat, METH_FASTCALL|METH_KEYWORDS, os_lstat__doc__},

The actual implementation chooses whether to use lstat or not and does the platform-specific dances:

cpython/Modules/posixmodule.c

Lines 2123 to 2170 in 7a23680

    
           static PyObject * 
        
           posix_do_stat(const char *function_name, path_t *path, 
        
                         int dir_fd, int follow_symlinks) 
        
           { 
        
               STRUCT_STAT st; 
        
               int result; 
        
           #if !defined(MS_WINDOWS) && !defined(HAVE_FSTATAT) && !defined(HAVE_LSTAT) 
        
               if (follow_symlinks_specified(function_name, follow_symlinks)) 
        
                   return NULL; 
        
           #endif 
        
               if (path_and_dir_fd_invalid("stat", path, dir_fd) || 
        
                   dir_fd_and_fd_invalid("stat", dir_fd, path->fd) || 
        
                   fd_and_follow_symlinks_invalid("stat", path->fd, follow_symlinks)) 
        
                   return NULL; 
        
               Py_BEGIN_ALLOW_THREADS 
        
               if (path->fd != -1) 
        
                   result = FSTAT(path->fd, &st); 
        
           #ifdef MS_WINDOWS 
        
               else if (follow_symlinks) 
        
                   result = win32_stat(path->wide, &st); 
        
               else 
        
                   result = win32_lstat(path->wide, &st); 
        
           #else 
        
               else 
        
           #if defined(HAVE_LSTAT) 
        
               if ((!follow_symlinks) && (dir_fd == DEFAULT_DIR_FD)) 
        
                   result = LSTAT(path->narrow, &st); 
        
               else 
        
           #endif /* HAVE_LSTAT */ 
        
           #ifdef HAVE_FSTATAT 
        
               if ((dir_fd != DEFAULT_DIR_FD) || !follow_symlinks) 
        
                   result = fstatat(dir_fd, path->narrow, &st, 
        
                                    follow_symlinks ? 0 : AT_SYMLINK_NOFOLLOW); 
        
               else 
        
           #endif /* HAVE_FSTATAT */ 
        
                   result = STAT(path->narrow, &st); 
        
           #endif /* MS_WINDOWS */ 
        
               Py_END_ALLOW_THREADS 
        
               if (result != 0) { 
        
                   return path_error(path); 
        
               } 
        
               return _pystat_fromstructstat(&st); 
        
           }

asottile · 2019-01-21T22:25:35Z

the ifdefs are a bit tricky but if lstat / fstatat are unavailable then os.lstat falls back to os.stat

pablogsal · 2019-01-21T22:31:24Z

Note that the os module falls back outside posix if the posixmodule is not compiled :

https://github.com/python/cpython/blob/master/Lib/os.py#L68

~~Windows systems (nt) usually will have chmod and lstat and friends, but I am not sure if they can be missing for some reason.~~

Nervermind, the nt module is actually posixmodule.

asottile · 2019-01-21T22:35:33Z

On windows, the nt module is created from the very same posixmodule.c (yes it's confusing 😭):

cpython/Modules/posixmodule.c

Lines 4 to 5 in 7a23680

    
           /* This file is also used for Windows NT/MS-Win.  In that case the 
        
              module actually calls itself 'nt', not 'posix', and a few

pablogsal · 2019-01-21T22:40:00Z

Apparently, there is nothing missing in the logic that leads to clean up the code as this PR is doing, but I would like Gregory to take a look in case I am missing something :)

asottile · 2019-02-13T04:51:27Z

@giampaolo @gpshead friendly ping :)

giampaolo

LGTM (good change).

asottile · 2019-02-25T21:50:52Z

@gpshead @giampaolo anything left for me to do here?

giampaolo · 2019-02-25T22:22:04Z

It was already approved. You can merge.

asottile · 2019-02-25T22:28:38Z

I am a mere pleb without green-button access <3

bedevere-bot · 2019-02-25T22:32:30Z

@giampaolo: Please replace # with GH- in the commit message next time. Thanks!

giampaolo · 2019-02-25T22:49:46Z

Sorry, my bad.

bedevere-bot · 2019-02-25T23:04:59Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Non-Debug with X 3.x has failed when building commit 8377cd4.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/99/builds/2198) and take a look at the build logs.
Check if the failure is related to this commit (8377cd4) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/99/builds/2198

Click to see traceback logs

Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.nondebug/build/Lib/test/test_descr.py", line 4341, in test_vicious_descriptor_nonsense
    self.assertEqual(c.attr, 1)
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.nondebug/build/Lib/test/test_descr.py", line 4328, in __eq__
    del C.attr
AttributeError: attr

----------------------------------------------------------------------

Ran 140 tests in 2.449s

FAILED (errors=1, skipped=2, expected failures=2)


Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.nondebug/build/Lib/test/test_descr.py", line 4341, in test_vicious_descriptor_nonsense
    self.assertEqual(c.attr, 1)
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.nondebug/build/Lib/test/test_descr.py", line 4328, in __eq__
    del C.attr
AttributeError: attr

----------------------------------------------------------------------

Ran 140 tests in 2.229s

FAILED (errors=1, skipped=2, expected failures=2)

* Clean up code which checked presence of os.{stat,lstat,chmod} (GH-11643) (cherry picked from commit 8377cd4) * bpo-36725: regrtest: add TestResult type (GH-12960) * Add TestResult and MultiprocessResult types to ensure that results always have the same fields. * runtest() now handles KeyboardInterrupt * accumulate_result() and format_test_result() now takes a TestResult * cleanup_test_droppings() is now called by runtest() and mark the test as ENV_CHANGED if the test leaks support.TESTFN file. * runtest() now includes code "around" the test in the test timing * Add print_warning() in test.libregrtest.utils to standardize how libregrtest logs warnings to ease parsing the test output. * support.unload() is now called with abstest rather than test_name * Rename 'test' variable/parameter to 'test_name' * dash_R(): remove unused the_module parameter * Remove unused imports (cherry picked from commit 4d29983) * bpo-36725: Refactor regrtest multiprocessing code (GH-12961) Rewrite run_tests_multiprocess() function as a new MultiprocessRunner class with multiple methods to better report errors and stop immediately when needed. Changes: * Worker processes are now killed immediately if tests are interrupted or if a test does crash (CHILD_ERROR): worker processes are killed. * Rewrite how errors in a worker thread are reported to the main thread. No longer ignore BaseException or parsing errors silently. * Remove 'finished' variable: use worker.is_alive() instead * Always compute omitted tests. Add Regrtest.get_executed() method. (cherry picked from commit 3cde440) * bpo-36719: regrtest always detect uncollectable objects (GH-12951) regrtest now always detects uncollectable objects. Previously, the check was only enabled by --findleaks. The check now also works with -jN/--multiprocess N. --findleaks becomes a deprecated alias to --fail-env-changed. (cherry picked from commit 75120d2) * bpo-34060: Report system load when running test suite for Windows (GH-8357) While Windows exposes the system processor queue length, the raw value used for load calculations on Unix systems, it does not provide an API to access the averaged value. Hence to calculate the load we must track and average it ourselves. We can't use multiprocessing or a thread to read it in the background while the tests run since using those would conflict with test_multiprocessing and test_xxsubprocess. Thus, we use Window's asynchronous IO API to run the tracker in the background with it sampling at the correct rate. When we wish to access the load we check to see if there's new data on the stream, if there is, we update our load values. (cherry picked from commit e16467a) * bpo-36719: Fix regrtest re-run (GH-12964) Properly handle a test which fail but then pass. Add test_rerun_success() unit test. (cherry picked from commit 837acc1) * bpo-36719: regrtest closes explicitly WindowsLoadTracker (GH-12965) Regrtest.finalize() now closes explicitly the WindowsLoadTracker instance. (cherry picked from commit 00db7c7)

Clean up code which checked presence of os.{stat,lstat,chmod}

c764b47

asottile requested a review from a team as a code owner January 21, 2019 21:59

the-knights-who-say-ni added the CLA signed label Jan 21, 2019

bedevere-bot added the awaiting review label Jan 21, 2019

pablogsal requested review from giampaolo and gpshead January 21, 2019 22:39

giampaolo approved these changes Feb 17, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Feb 17, 2019

gpshead approved these changes Feb 18, 2019

View reviewed changes

giampaolo merged commit 8377cd4 into python:master Feb 25, 2019

bedevere-bot removed the awaiting merge label Feb 25, 2019

asottile deleted the os_stat_lstat_bop_35802 branch February 25, 2019 22:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

bpo-35802: Clean up code which checked presence of os.{stat,lstat,chmod} #11643

bpo-35802: Clean up code which checked presence of os.{stat,lstat,chmod} #11643

Uh oh!

asottile commented Jan 21, 2019 •

edited by bedevere-bot

Loading

Uh oh!

pablogsal commented Jan 21, 2019 •

edited

Loading

Uh oh!

asottile commented Jan 21, 2019

Uh oh!

asottile commented Jan 21, 2019

Uh oh!

pablogsal commented Jan 21, 2019 •

edited

Loading

Uh oh!

asottile commented Jan 21, 2019

Uh oh!

pablogsal commented Jan 21, 2019 •

edited

Loading

Uh oh!

asottile commented Feb 13, 2019

Uh oh!

giampaolo left a comment

Uh oh!

asottile commented Feb 25, 2019

Uh oh!

giampaolo commented Feb 25, 2019

Uh oh!

asottile commented Feb 25, 2019

Uh oh!

bedevere-bot commented Feb 25, 2019

Uh oh!

giampaolo commented Feb 25, 2019

Uh oh!

bedevere-bot commented Feb 25, 2019

Uh oh!

Uh oh!

Uh oh!

bpo-35802: Clean up code which checked presence of os.{stat,lstat,chmod} #11643

bpo-35802: Clean up code which checked presence of os.{stat,lstat,chmod} #11643

Uh oh!

Conversation

asottile commented Jan 21, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal commented Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asottile commented Jan 21, 2019

Uh oh!

asottile commented Jan 21, 2019

Uh oh!

pablogsal commented Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asottile commented Jan 21, 2019

Uh oh!

pablogsal commented Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asottile commented Feb 13, 2019

Uh oh!

giampaolo left a comment

Choose a reason for hiding this comment

Uh oh!

asottile commented Feb 25, 2019

Uh oh!

giampaolo commented Feb 25, 2019

Uh oh!

asottile commented Feb 25, 2019

Uh oh!

bedevere-bot commented Feb 25, 2019

Uh oh!

giampaolo commented Feb 25, 2019

Uh oh!

bedevere-bot commented Feb 25, 2019

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

Uh oh!

asottile commented Jan 21, 2019 •

edited by bedevere-bot

Loading

pablogsal commented Jan 21, 2019 •

edited

Loading

pablogsal commented Jan 21, 2019 •

edited

Loading

pablogsal commented Jan 21, 2019 •

edited

Loading