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

Conversation

Projects
None yet
3 participants
@Ralith
Copy link
Contributor

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 Ralith force-pushed the Ralith:search-paths branch from 18359f9 to 6237edb Mar 27, 2019

@Ralith

This comment has been minimized.

Copy link
Contributor Author

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.

@Ralith Ralith force-pushed the Ralith:search-paths branch from 6237edb to 424c896 Mar 27, 2019

@rpavlik rpavlik requested review from daveh-lunarg and MarkY-LunarG Apr 2, 2019

@rpavlik

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

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

@rpavlik rpavlik added the in loader label Apr 2, 2019

@daveh-lunarg

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Opened #28 to discuss improving the intended behavior.

@Ralith Ralith force-pushed the Ralith:search-paths branch 2 times, most recently from d8335c8 to 67b96d3 Apr 3, 2019

@Ralith

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

@Ralith can you take care of the CLA?

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

@Ralith Ralith force-pushed the Ralith:search-paths branch from 67b96d3 to f3e3efd Apr 9, 2019

@Ralith

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

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 Apr 30, 2019

Show resolved Hide resolved src/loader/manifest_file.cpp
Show resolved Hide resolved src/common/platform_utils.hpp Outdated
Fix active runtime manifest search on Linux
The draft calls for a single hardcoded path to be used.

@Ralith Ralith force-pushed the Ralith:search-paths branch from f3e3efd to 6184555 May 3, 2019

@rpavlik

This comment has been minimized.

Copy link
Contributor

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

1 check passed

license/cla Contributor License Agreement is signed.
Details

@Ralith Ralith deleted the Ralith:search-paths branch May 6, 2019

rpavlik added a commit that referenced this pull request May 9, 2019

Change log for OpenXR 0.90.1 provisional spec update (8-May-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
You can’t perform that action at this time.