Path resolution based on OpenNI2.dll directory (plus xnOSGetDirName() fix) #7
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Problem: - Windows version of xnOSGetDirName() returns the directory name followed by a directory separator, which was (seemingly) different from Linux version's behavior. This difference could be a problem when you tried to write a portable code. Solution: - Windows version of xnOSGetDirName() now returns the directory name without the directory separator, which should be the same as Linux version. - Another helper functions, xnOSStripDirSep() and xnOSIsDirSep(), are introduced to implement this in a clean and portable way. Description of changes: - XnWin32Files.cpp: Added a code to strip the directory separator in xnOSGetDirName(). - XnOS.h: Utility functions to handle the directory separators are added (xnOSStripDirSep, xnOSIsDirSep) - XnLibWin32.h, XnOSLinux-x86.h: String constant XN_FILE_DIR_SEPS is added to support both "\" and "/" in Windows. This is helpful for the application programmers to write a portable code by consistently using "/" in any platforms. Note for Linux: Tested on Windows only.
Fix 1: Drivers' path is now resolved based on OpenNI2.dll's directory.
Problem:
- The drivers' path ("OpenNI2/Drivers" by default) is resolved based on the current directory, which could be a problem because it would restrict the application to be runnable only in a specific directory. See the OpenNI community at https://community.openni.org/openni/topics/search_path_for_redist_files where I originally pointed out this issue.
Solution:
- Now the drivers' path is resolved based on OpenNI2.dll's directory. By default, <OpenNI2.dll's directory>/OpenNI2/Drivers is used. If the custom path is specified by OpenNI.ini (by Repository property in Drivers section), it will be resolved based on OpenNI2.dll's directory if it is relative, or used as it is when it is absolute.
- To specify the custom path, the application can use "/" as the separater even in Windows so that the application can use the same OpenNI.ini regardless of the platform.
- The path resolution by the environment variable (OPENNI2_DRIVERS_PATH) is kept untouched to preserve the current behavior, but I would suggest removing it because it could be no longer useful but might break the consistency between the DLL and the drivers.
Description of changes:
- OniContext.cpp: The drivers' path is resolved based on OpenNI2.dll's directory instead of using the string constant.
- XnOS.h, XnWin32SharedLibrary.cpp, XnLinuxSharedLibs.cpp: Function to get the DLL's path is added (xnOSGetModulePathForProcAddress). See "Note for Linux" also.
- XnOS.h, XnFiles.cpp, XnWin32Files.cpp, XnLinuxFiles.cpp: Utility functions to handle file paths are added (xnOSAppendFilePath, xnOSIsAbsoluteFilePath). See "Note for Linux" also.
- Config/OpenNI.ini: Comment is added to explain how to specify the drivers' path in detail.
Note for Linux:
- This fix has not been compiled or tested on Linux, but intended not to break anything existing at least.
- xnOSGetModulePathForProcAddress() in XnLinuxSharedLibs.cpp needs to be implemented. It should be easily done by using dladdr(). For now, it has the dummy implementation that should not break existing behavior.
- xnOSIsAbsoluteFilePath() in XnLinuxFiles.cpp is implemented but not tested.
Fix 2: OpenNI.ini is now read from OpenNI2.dll's directory.
Problem:
- OpenNI.ini is read from the current directory, which could be a problem by the same reason as Fix 1.
Solution:
- Now OpenNI.ini is read from the same directory as OpenNI2.dll.
- If there is OpenNI.ini in the current directory, it overrides OpenNI.ini in OpenNI2.dll's directory. This feature is kept to preserve the current behavior, but I would suggest removing it because the application's behavior should be independent from the current directory unless the application itself was so designed.
Description of changes:
- OniContext.cpp: OpenNI.ini's path is resolved based on OpenNI2.dll's directory instead of using the string constant.
Fix 3: Improvement of the log configuration timing.
Problem:
- Some logs are not outputted because the log configuration in OpenNI.ini is read too late.
Solution:
- Complete log configuration before any log outputs as long as possible.
Description of changes:
- OniContext.cpp: The order of log configuration and log output was slightly exchanged.
As described in the previous commit, resolving the driver path based on environment variable (OPENNI2_DRIVERS_PATH) and reading OpenNI.ini from the current directory could be only confusing if we started to resolve it based on OpenNI2.dll's directory. If you agree, please use this commit to remove these features. It will make the behavior and the code much simpler and more explainable.
|
I may add another commit if I can apply the same method for PS1080.ini. Thanks. |
Problem: - PS1080.ini is read from the current directory, which could be a problem because (1) it would restrict the application to be runnable only in a designated directory and (2) the current directory would be messed up by these driver dependent files. See the OpenNI community at https://community.openni.org/openni/topics/search_path_for_redist_files where I originally pointed out this issue. Solution: - Now PS1080.ini is read from the same location as PS1080.dll. Description of changes: - XnSensor.cpp: ResolveGlobalConfigFileName() is modified to resolve the config file's path based on the DLL's directory when the strConfigDir is specified as NULL, and now XnSensor's constructor uses it to initialize m_strGlobalConfigFile. - XnSensor.h: Comment is added for ResolveGlobalConfigFileName(). - Config/PS1080.ini: Moved to Config/OpenNI2/Drivers/PS1080.ini. Note: - Please test the installer (Redist.py) if it deploys PS1080.ini as intended, i.e. along with PS1080.dll.
|
I have done the work for PS1080.ini and committed. Thanks. |
Closed
|
Benn (piedar) found my code did not compile with gcc (on Linux). We are working on it at tomoto#1. |
- OniContext.cpp, XnSensor.cpp: Cast is applied to function pointer to avoid gcc compilation errors. (Thanks to Benn for contribution) - XnLinuxSharedLibs.cpp: xnOSGetModulePathForProcAddress() is implemented. xnOSLoadLibrary() is also fixed to let dladdr() return the desired information.
|
I decided to build my own Linux environment to investigate the problems reported by Benn, and eventually completed the working implementation! My quick testing confirmed the paths were resolved based on the lib*.so's. Please see the commit 86efc7a. |
|
This path resolution branch is working nicely for me on Gentoo Linux x86_64. |
|
Hi Benn, Thank you very much for your contribution to find the issue and validate the fix! I really appreciate it! Tomoto |
tomoto
added a commit
that referenced
this pull request
Jan 15, 2013
…rary location See #7 for details.
|
I merged this pull request into the develop branch after the peer review by Eddie. |
|
This was merged in, but then immediately overwritten by the following commit. Was this intentional or an accident? |
|
When I asked the same thing, I was told it was an accident. But I think this ended up getting stranded in the develop branch. I wonder what the plan is for merging from there to master... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi OpenNI folks,
Here is a patch to address the issue I pointed out in the community at https://community.openni.org/openni/topics/search_path_for_redist_files. With this patch, the path for the drivers and OpenNI.ini will be resolved based on OpenNI2.dll's directory so that the users are no longer required to launch the application from the designated directory. It also allows them to use the shared driver repository just by setting PATH environment variable to its master OpenNI2.dll.
This patch includes three commits as follows. All the details are explained in their commit comments.
I hope this patch to be reviewed (and accommodated if you agree) as early as possible since this is an external behavior change that may impact on the beta users although it is (I believe) a significant improvement.
Please get back to me if you have any questions.
Thank you very much,
Tomoto