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

Mac install #1145

Closed
wants to merge 13 commits into from
Closed

Mac install #1145

wants to merge 13 commits into from

Conversation

freesig
Copy link
Contributor

@freesig freesig commented Dec 27, 2018

  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes - in this repository

This PR automatically downloads and installs the latest Vulkan SDK from lunarg.
It will only do this if VULKAN_SDK is not set.
The sdk will be installed at $HOME/.vulkan_sdk
and the following environment variables will be set in the users .profile or .bash_profile

export VULKAN_SDK=$HOME/.vulkan_sdk/macOS
export PATH=$VULKAN_SDK/bin:$PATH
export DYLD_LIBRARY_PATH=$VULKAN_SDK/lib:$DYLD_LIBRARY_PATH
export VK_ICD_FILENAMES=$VULKAN_SDK/etc/vulkan/icd.d/MoltenVK_icd.json
export VK_LAYER_PATH=$VULKAN_SDK/etc/vulkan/explicit_layer.d

The only thing that isn't automated is the user will get a error about failing to link the vulkan libs. This is because the changes to their profile are not automatically refreshed.
They will need to either source ~/.profile or source ~/.bash_profile or reopen their terminal after the first time they build vulkano.

If anyone knows how to automate this that would be super useful.
So far I have tried to pass the env var settings along from the build script to the lib using cargo:rustc_env but that only works if you are directly using Vulkano and not if it's a dependency.

One thing we could do is assume that if the env vars are not set when Vulkano is run then they must just be set to the defaults and but the user has not sourced their profile. Then just std::env::set_var to set them temporarily.

@mitchmindtree
Copy link
Contributor

cc @mcclure this is an attempt at what you were after in #927 - any chance you're still interested and would like to review/test this branch? No worries if you're no longer interested, that issue is pretty old :)

@mitchmindtree
Copy link
Contributor

Here's the moltenvk_deps src in case anyone would like to see what it's doing in detail.

@mitchmindtree
Copy link
Contributor

Hey I just tried running this on my dad's MBP!

It almost worked fine, however the env vars did not load automatically when I closed and re-opened the terminal. However, if I manually did source ~/.profile it worked fine afterwards. Perhaps .profile is not sourced automatically when bash starts up? Maybe we're better off putting these env vars in .bash_profile?

@freesig
Copy link
Contributor Author

freesig commented Jan 1, 2019

Yeh I think you're right I might remove the .profile stuff and just have a .bash_profile. It means we only support bash but that is the default on macos since 10.1 and I'll put a note in the readme for other shells.

@freesig
Copy link
Contributor Author

freesig commented Jan 2, 2019

Ok I've made those changes, you may need to run cargo update before testing it.

README.md Show resolved Hide resolved
@rukai
Copy link
Member

rukai commented Jan 6, 2019

There is also this PR which wanted to make homebrew the recommend way to install vulkan sdk #1028
My understanding is this would automatically handle updates. Is that something this PR can do?

@mitchmindtree
Copy link
Contributor

@rukai I think recommending homebrew is a little controversial on macos as users are often split between macports and homebrew. It's also worth noting neither are installed by default, whereas this PR just uses default mac tooling which should already exist if they have rust setup.

I guess it wouldn't hurt to mention it as an option in the case that the user already has homebrew installed?

@rukai
Copy link
Member

rukai commented Jan 6, 2019

I think adding a mention of macports and homebrew would just further complicate an already large readme without much gain.

What we need is a way to update the vulkan sdk installed by vulkano.
Currently moltenvk_deps will install the vulkan sdk if VULKAN_SDK is not set.
However we should also install if VULKAN_SDK is set to the value that moltenvk_deps sets it to and the path points to a directory that does not exist.
This way the user can update by just deleting ~/.vulkan_sdk.
This will also need to be documented in the readme.

@freesig
Copy link
Contributor Author

freesig commented Jan 13, 2019

I would hesitate to use homebrew to install the sdk as the user would need to call homebrew update to get the new sdk anyway and it just adds another dependency.
I like the idea of moving the install to runtime. I think it should be possible to check the version is the latest with moltenvk_deps but I'm not sure all users would want to automatically update. So maybe it's better to just print a notice saying it's out of date with an option to update.

I'll make these changes and see what I can come up with.

@freesig
Copy link
Contributor Author

freesig commented Jan 15, 2019

I'm stuck on automatic updates because I cannot find anyway to reliably get the latest version of the vulkan_sdk without downloading it.

Additionally I can't rely on the user deleting ~/.vulkan_sdk because they may have chosen to install it manually in a different location.
I cannot get them to unset VULKAN_SDK because that is the case when they haven't source'd bash anyway.

We could however get them to set UPDATE_VULKAN_SDK=1 temporarily. This would then trigger an update. So the command would be used like:

UPDATE_VULKAN_SDK=1 cargo run

Is this an appropriate solution?
(I've implemented it locally and it works fine)

@rukai
Copy link
Member

rukai commented Jan 15, 2019

So earlier I said "However we should also install if VULKAN_SDK is set to the value that moltenvk_deps sets it to and the path points to a directory that does not exist."
Because I figured that we dont want vulkano touching anything if the user has already, at least partially, setup their system. (just to be on the safe side)
However we could definitely change this to just "However we should also install if VULKAN_SDK points to a directory that does not exist."

Then we don't need the UPDATE_VULKAN_SDK=1 which is imo less intuitive.
Although being able to use as UPDATE_VULKAN_SDK=1 cargo run isnt too bad. Its just annoying to have too lookup what the magic env var is when you need to update. Its easier to just remember to delete the vulkan directory.

@freesig
Copy link
Contributor Author

freesig commented Jan 15, 2019

Yeh good call, that's a lot simpler.
I've made it so it will update if the VULKAN_SDK is set to the default directory and that directory is empty as you said. (I agree it's probably not good to touch anything if the user has manually set it)
So all you need to do now is remove the ~/.vulkan_sdk directory to update the sdk to latest.

I'll update the README with your with your changes and to reflect this behavior then push these changes.

@mitchmindtree
Copy link
Contributor

@freesig it looks like travis is failing due to trailing whitespace in one file?

Builds fine on Linux (as expected) and also can confirm the auto-install runs smoothly on @JoshuaBatty's MBP!

@mitchmindtree
Copy link
Contributor

Hmm now it seems that although linux is building fine, macos is error-ing on a timeout in the tests. @freesig are you able to run the tests locally on macos or do they timeout for you too? I wonder if something can be done about the culprit test.

@freesig
Copy link
Contributor Author

freesig commented Jan 27, 2019

Yeh this is a little weird. If the test was triggering a moltenvk_deps to ask for a sdk install then there would be text in the log. It can't try and download without the users permission. I will run the test locally and report back.

@freesig
Copy link
Contributor Author

freesig commented Jan 27, 2019

I get different failures on my machine

    memory::device_memory::tests::oom_single
    pipeline::compute_pipeline::tests::spec_constants
    sync::event::tests::event_create
    sync::event::tests::event_pool
    sync::event::tests::event_reset
    sync::event::tests::event_set

I'll keep digging. I wonder which version of the sdk travis is using.

@freesig
Copy link
Contributor Author

freesig commented Jan 27, 2019

I just remembered that cargo test captures output. I think it might be detecting that the sdk isn't installed and asking the user if it would like to install it.
I'm not sure the best way to get around this. I could time out, just not install it if the user doesn't respond in a certain time, although that's not great.

Found this:
#[cfg(not(test))]

@mitchmindtree
Copy link
Contributor

I would personally be fine without any command-line user prompt whatsoever and just going ahead and installing if we can't find an existing installation. I think it would be better to have a function/method/parameter of some sort which allows a developer to opt out of auto-installation in their code in case they want to run their own GUI installer or something. I'd imagine most downstream games would want to go ahead and install vulkan or provide their own GUI installer rather than have a terminal open up for the user to answer?

@freesig
Copy link
Contributor Author

freesig commented Jan 27, 2019

Yeh I see what you mean but also its ~125mb file and the user might not be in the best position to start a download (bad net connection etc.) and would rather continue the install themselves later or perhaps even manually. Not everyone would be comfortable with an automated install and without a prompt there's not really any good way to warn them (vulkano could be a downstream dependency and they don't read the vulkano docs).

Perhaps I could make a function that in the vulkano api for the developer that's using vulkano to manually create a check for the dependencies before the instance is loaded (and us that in their gui). (although I'm not sure how easy this would be as the instance get's the library path with lazy_static

@rukai
Copy link
Member

rukai commented Jan 27, 2019

Another option here is to not use moltenvk_deps in vulkano but recommend it to the developer as an easy way to setup moltenvk. Then they can call it however they like and it will never do anything unexpected. We can include it in the tutorial on https://vulkano.rs

@freesig
Copy link
Contributor Author

freesig commented Feb 20, 2019

I managed to get this working on the current vulkano master so I will close this PR now. But I'm happy to make another PR if you would like instructions for how to use moltenvk_deps in your readme.

@freesig freesig closed this Feb 20, 2019
@knappador knappador mentioned this pull request Mar 11, 2019
2 tasks
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