Skip to content

Conversation

@bridadan
Copy link
Contributor

Should fix #263 and #264.

In #241, I modified the detection process for Windows to use the fsutil program that ships with Windows. It provides a very fast way to detect what drive letters are currently available on the system. However, I missed this very important fact (spelled out in plain ol' English on their doc page):

You must be logged on as an administrator or a member of the Administrators group to use fsutil. The fsutil command is quite powerful and should be used only by advanced users who have a thorough knowledge of Windows operating systems.

😞

This PR changes the drive detection to the following:

  • Take the results that are returned from the registry query
    • This includes mountpoints that may or may not be present on the system
  • Loop over each mountpoint and call the Windows dir command
    • If it fails, don't include it in the returned list
    • The downside to this approach is we have to make more system calls
      • "But why not just use os.path.exists ?!?!"
      • If you call that function on a drive that is in the ejected state (happens directly after flashing), Python will throw a blocking error box and halt, and the workaround isn't that great
    • The upside is this doesn't require administrator privileges

FYI @toyowata @MarceloSalazar

The 'fsutil' command requires administrator privileges, this change
changes the detection to use multiple calls to the Windows command
'dir' to check which drives are ready.
@bridadan
Copy link
Contributor Author

Could you please review @theotherjimmy ?

@bridadan
Copy link
Contributor Author

Well that's weird, works just fine on my Windows box. I'll try it in my VM.

test/os_win7.py Outdated

_cliproc.return_value = (b'\nDrives: C:\ F:\ Z:\ \n', None, 0)

def _mounted_drives_check(drives):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this function outside of the test?

@coveralls
Copy link

coveralls commented Nov 10, 2017

Coverage Status

Coverage decreased (-0.7%) to 73.272% when pulling 87af6d1 on bridadan:no_fsutil_win_detection into 3dfbc83 on ARMmbed:master.

@theotherjimmy
Copy link
Contributor

Thanks for the update. @MarceloSalazar You could use a new release with this change set correct?

@theotherjimmy theotherjimmy merged commit 7a505b1 into ARMmbed:master Nov 10, 2017
@bridadan
Copy link
Contributor Author

Thanks for the update. @MarceloSalazar You could use a new release with this change set correct?
@theotherjimmy my understanding is he has a workshop to put on so yes to a new release!

@MarceloSalazar
Copy link

Thanks @bridadan @theotherjimmy for the quick fix!
I confirm v1.3.4 works well on Windows 7 :)

@toyowata
Copy link
Contributor

@bridadan @theamirocohen me too!! Thanks for your fix! :-)

@bridadan
Copy link
Contributor Author

Great to hear, thanks for testing @MarceloSalazar and @toyowata!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mbedls v1.3.3 requires admin rights on Window 7

5 participants