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

Fix Linux manifest file search #20

Merged
merged 2 commits into from
May 6, 2019
Merged

Conversation

Ralith
Copy link
Contributor

@Ralith Ralith commented Mar 27, 2019

This fixes various unintended behavior (such as trying to parse json files from completely unrelated applications) and gets the loader working with an out-of-the-box Monado installation.

@Ralith
Copy link
Contributor Author

Ralith commented Mar 27, 2019

It's unclear what the intended behavior is in the case of multiple runtime manifests; there seems to be some suggestion that only files named active_runtime.json should be considered, but the prior code is clearly attempting to traverse various search paths for any *.json file, then processing all of them in order, with the (redundant-at-best) hardcoded path checked last.

@rpavlik
Copy link
Contributor

rpavlik commented Apr 2, 2019

I've tagged the two most relevant folks who can address design intent here. Hopefully this can be merged soon.

@daveh-lunarg
Copy link
Contributor

I think there's confusion between different json file searches. Loader should only look in 1 (possibly poorly chosen) location for active_runtime.json, with an optional override via an environment variable.

The loader then looks at every .json file it finds within a half dozen paths (including XDG*) to see if it's a layer description json, but only paths ending in openxr/<major_version>/api_layers/implicit.d or openxr/<major_version>/api_layers/explicit.d. If it's opening json files in paths without one of these suffixes, it sounds like a loader bug.

I don't mid seeing this file locking code go away, AFAICT there's no need for it.

@Ralith
Copy link
Contributor Author

Ralith commented Apr 2, 2019

If the objective is to find exactly one active_runtime.json file, it would be more useful to still walk the full set of search paths, but discard all but the most-specific file found. For example, this would allow users to modify the default behavior in their environment by installing a ~/.local/share/openxr/$VERSION/active_runtime.json. Note also that different distros keep the relevant files in different locations (NixOS would probably want it in /run/opengl-driver/share/... for example) and referencing the XDG environment vars will make this Just Work more often, which is particularly important if the loader's being statically linked into binaries.

The pre-existing behavior is to try to load every json file floating around loose in $XDG* (not in the openxr/$VERSION subdir), and then try the hardcoded path.

@daveh-lunarg
Copy link
Contributor

I have a PR up to add the loader design document, which might be helpful for context.

I think ideas about how to change the active_runtime.json search behavior should go into an issue, rather than this PR discussion.

@Ralith
Copy link
Contributor Author

Ralith commented Apr 2, 2019

Opened https://github.com/KhronosGroup/OpenXR-SDK/issues/28 to discuss improving the intended behavior.

@Ralith Ralith force-pushed the search-paths branch 2 times, most recently from d8335c8 to 67b96d3 Compare April 3, 2019 00:48
@Ralith
Copy link
Contributor Author

Ralith commented Apr 3, 2019

In the interest of reducing bugs ASAP, I've modified this PR to get the intended behavior working as documented in #27; changes to the intended behavior can be handled in a future PR.

@rpavlik
Copy link
Contributor

rpavlik commented Apr 8, 2019

@Ralith can you take care of the CLA?

@daveh-lunarg when you return, can you re-review?

@Ralith
Copy link
Contributor Author

Ralith commented Apr 9, 2019

I think I found the right CLA thing, though it wasn't really clear where it was. Also stripped out a bit of leftover code from the PR's previous incarnation.

@Ralith
Copy link
Contributor Author

Ralith commented Apr 22, 2019

In case it wasn't clear, this fixes (among other things) behavior where the loader will try to parse loose json files in e.g. /etc/xdg, fail, and bail out even when a runtime is correctly installed.

@daveh-lunarg daveh-lunarg requested a review from rpavlik April 30, 2019 22:39
src/loader/manifest_file.cpp Show resolved Hide resolved
src/common/platform_utils.hpp Outdated Show resolved Hide resolved
The draft calls for a single hardcoded path to be used.
@rpavlik
Copy link
Contributor

rpavlik commented May 6, 2019

Thanks for splitting out that other part - this I can easily merge.

Thanks for the contribution!

@rpavlik rpavlik merged commit b0e62b6 into KhronosGroup:master May 6, 2019
@Ralith Ralith deleted the search-paths branch May 6, 2019 21:43
rpavlik added a commit that referenced this pull request May 9, 2019
No API changes, and only minimal consistency changes to the spec/registry.
Mostly an update for tooling, layers, loader, and sample code.
Header version has been bumped to 43, but no symbols that should have actually been in use have changed.

### GitHub Pull Requests

These had been integrated into the public repo incrementally.

- General, Build, Other
  - #8, #11, #12 - Improve BUILDING and README
  - #9 - Make Vulkan SDK dependency optional
  - #17 - Add install target to CMake files
  - #17 - API dump layer, build: timespec extension fixes
  - #19 - build: fix CMAKE_PRESENTATION_BACKEND default on linux
  - #34 - list: Fix list test output
- validation layer
  - #18, #22, #23 - Fix build and execution
  - #24 - Fix crash and refactor
- hello_xr
  - #13 - Do not query GL context API version before creating context
  - #26 - Fix a warning
- Loader
  - #3 - Don't cross 32/64 registry silos
  - #14 - Initialize XrExtensionProperties array parameter for rt_xrEnumerateInstanceExtensionProperties
  - #20 - Fix Linux manifest file search
  - #30 - Add default implementations of API functions to dispatch chains
  - #32 - Avoid crash when evaluating layer disable environment vars
  - #35 - Add 'unknown' strings to loader's *ToString fallback functions
  - #36 - Allow null instance in xrGetInstanceProcAddr() for certain entry points
  - #39 - Default to static loader only on Windows

### Internal Issues

- General, Build, Other
  - Unify (for the most part) the OpenXR and Vulkan generator scripts. (internal MR 1166)
  - List instance extensions in the "list" test. (internal MR 1169)
  - Avoid dllexport for all apps compiled with `openxr_platform_defines.h` (internal MR 1187)
  - Don't offer `BUILD_SPECIFICATION` unless the spec makefile is there. (internal MR 1179)
  - Add simple input example to hello_xr. (internal MR 1178)
  - Add a clang-format script for ease of development.
- API Registry and Headers
  - Remove impossible and undocumented error codes. (internal MR 1185 and 1189)
  - Mark layers in `XrFrameEndInfo` as optional. (internal MR 1151, internal issue 899)
  - Remove unused windows types from `openxr_platform.h` (internal MR 1197)
  - Make `openxr_platform.h` include `openxr.h` on which it depends. (internal MR 1140, internal issue 918)
  - Remove unused, undocumented defines. (internal MR 1238, internal issue 1012)
- Loader
  - Fix loader regkey search logic so 64bit application loads 64bit regkey value. (internal MR 1180)
  - Modify loader to be friendly to UWP (Universal Windows Platform) build target. (internal MR 1198)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants