Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make MMap win32 compatible #106

Merged
merged 1 commit into from
May 30, 2023
Merged

Conversation

leonhma
Copy link
Contributor

@leonhma leonhma commented May 30, 2023

Use the system-agnostic access argument instead of the posix specific prot when initializing mmap.

Currently when trying to load a binary file, I get this error:

Traceback (most recent call last):
  File "C:\Users\leonh\AppData\Local\Programs\Python\Python310\lib\multiprocessing\process.py", line 314, in _bootstrap
    self.run()
  File "C:\Users\leonh\AppData\Local\Programs\Python\Python310\lib\multiprocessing\process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
  File "C:\Users\leonh\Documents\akyth-documents-download\program2.py", line 39, in check_seen
    seen = probables.ExpandingBloomFilter(100_000_000, 0.001, "./seen.blm")
  File "C:\Users\leonh\Documents\pyprobables\probables\blooms\expandingbloom.py", line 61, in __init__
    self.__load(filepath)
  File "C:\Users\leonh\Documents\pyprobables\probables\blooms\expandingbloom.py", line 211, in __load
    with MMap(file) as filepointer:
  File "C:\Users\leonh\Documents\pyprobables\probables\utilities.py", line 45, in __init__
    self.__m = mmap.mmap(self.__f.fileno(), 0, prot=mmap.PROT_READ)

I tested this again using WSL and everything worked fine. In the end this comes down to the fact that the prot argument in the MMap init function is POSIX-specific, while access serves as a system-agnostic way of doing the same thing. I changed the init line and tested this change on both windows and and WSL and this error is gone.

Here's the relevant docs for MMap: link

Please merge ASAP as this breaks usage on windows.

Use the system-agnostic `access` argument instead of the posix specific `prot` when initializing mmap.
@codecov-commenter
Copy link

Codecov Report

Merging #106 (666b583) into master (ee924a2) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #106   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files          14       14           
  Lines        1528     1528           
=======================================
  Hits         1521     1521           
  Misses          7        7           
Impacted Files Coverage Δ
probables/utilities.py 100.00% <100.00%> (ø)

@barrust
Copy link
Owner

barrust commented May 30, 2023

This looks good and is a great catch. I do not have a windows machine to run tests on windows. I will merge this now and will likely be able to push a new version in the next day or so.

@barrust barrust merged commit 0f59574 into barrust:master May 30, 2023
@barrust
Copy link
Owner

barrust commented Jun 1, 2023

new version 0.5.8 is now out.

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.

3 participants