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

PATH editing only needed in Windows #55

Merged
merged 2 commits into from Dec 1, 2017
Merged

PATH editing only needed in Windows #55

merged 2 commits into from Dec 1, 2017

Conversation

native-api
Copy link
Contributor

AFAICS, FFMpeg DLL is only bundled in the Windows version.

@skvark
Copy link
Member

skvark commented Nov 24, 2017

Thanks! However, I'm not sure if that PATH modification is needed anymore (and Python might not be capable to inject that new PATH to the environment of the running process). The structure of the packages has changed after that a lot and it would be good to do a test run without that line and see if tests pass.

@native-api
Copy link
Contributor Author

https://docs.python.org/3/library/os.html?highlight=os%20environ#os.environ:

If the platform supports the putenv() function, this mapping may be used to modify the environment

I can also confirm that opencv_ffmpeg330_64.dll library load works in the 3.3.0 release even without PATH modification -- yet PATH is explored first when looking for it.

@native-api
Copy link
Contributor Author

At this rate, the entire wrapper __init__.py stops being necessary. PyCharm is already smart enough to autocomplete symbols from an extension module.

@skvark
Copy link
Member

skvark commented Nov 24, 2017

I think it's safe to remove that PATH line. __init__.py must be kept since it's the only way for Python to detect that the folder is actually a package and keep the import interface (import cv2) backwards compatible. Without those couple of lines the import would have to be from cv2 import cv2 and this repository would be full of issues like "can't import cv2" because most of the users don't bother to read any documentation :)

Installing to the root of site-packages is a no go (which is the way how OpenCV installation guides usually do it). That's probably not even possible without manual copying.

@native-api
Copy link
Contributor Author

native-api commented Nov 26, 2017 via email

@skvark
Copy link
Member

skvark commented Nov 26, 2017

You are correct, it's possible to distribute standalone modules but I'm not willing to ditch the cv2 directory. OpenCV installs the DLL files to the root, but it doesn't mean that it's good practice. Afaik OpenCV's installer just copies the files into the site-packages. There's no package metadata which means that for example the cv2.pyd and its dependencies can't be uninstalled via pip.

The main reason why I added the cv2 folder when I started developing this project was that it serves as a namespace and isolates the package contents in a more convenient way. This of course doesn't still take into account the fact that there might be a conflicting manually installed OpenCV in the user's site-packages, but at least this package will install/uninstall cleanly without issues in that case. Also one thing to consider is that dropping the namespace at this point would most likely break all existing installations during upgrade.

IDE support (PyCharm) is related to the namespace in a different way since the import logic in the __init__.py was originally flawed (PyCharm did not pick up the cv2 module, see this commit: 3140bce). This was fixed in a recent release.

@native-api
Copy link
Contributor Author

Anyway, I removed the PATH line entirely. Guess we can wrap up this PR now.

@skvark
Copy link
Member

skvark commented Dec 1, 2017

Thanks!

@skvark skvark merged commit 424ec6a into opencv:master Dec 1, 2017
@native-api native-api deleted the wrapper_path_ branch December 6, 2017 22:33
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.

None yet

2 participants