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

Update vulkanSDK #825

Merged
merged 28 commits into from
Feb 26, 2019
Merged

Update vulkanSDK #825

merged 28 commits into from
Feb 26, 2019

Conversation

ImperatorS79
Copy link
Contributor

The verb did not create the reg entries, but now it only create the entry for Wow6432node.

@Zemogiter could you check ?

@ImperatorS79
Copy link
Contributor Author

Ok, tested with a .reg. If using wine regedit myreg.reg -> does not work. But if I import int from regedit itself from the GUI -> works.

Maybe a bug in wine so I will simply add a wizard asking the user to add the key manually for now.

@ImperatorS79
Copy link
Contributor Author

Ready to be merged if no errors.


return this;
this.wizard().message(message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why showing this to the user

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the part using regedit does not work, so the user has to create the key himself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it does not work?
This is not acceptable to ask him to do it in terms of user experience

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems my comment did not get uploaded ^^.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just seen it. This could not be a bug in wine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just tested it and it is. The second key is created but not the first one. Also importing the .reg in regedit gui works, but not with wine regedit myreg.reg.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file must not be correctly formatted. Why would it not work just with this particular reg file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look, you've left a trailing "n" character

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea. But importing it in the gui works -> not a format problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look, you've left a trailing "n" character in your file...

@qparis
Copy link
Member

qparis commented Jan 20, 2019

Now that you have fixed the reg file, wine should be able to import it

@ImperatorS79
Copy link
Contributor Author

That was not that .reg file which causes problem, and the n was just a copy paste error.

@qparis
Copy link
Member

qparis commented Jan 20, 2019

Please send us the .reg file that causes problem then.

@ImperatorS79
Copy link
Contributor Author

ImperatorS79 commented Jan 20, 2019

Just copy paste this in a .reg:

REGEDIT4

[HKEY_LOCAL_MACHINE\Software\Khronos\Vulkan\Drivers]
"C:\\Windows\\winevulkan.json"=dword:00000000

You can interchangeREGEDIT 4 and Windows Registry Editor Version 5.00.

@qparis
Copy link
Member

qparis commented Jan 20, 2019

Ok, and what is the key that works correctly?

@ImperatorS79
Copy link
Contributor Author

REGEDIT4

[HKEY_LOCAL_MACHINE\Software\Wow6432Node\Khronos\Vulkan\Drivers]
"C:\\Windows\\winevulkan.json"=dword:00000000

@qparis
Copy link
Member

qparis commented Jan 20, 2019

Both work for me

@ImperatorS79
Copy link
Contributor Author

ImperatorS79 commented Jan 20, 2019

With wine regedit hello.reg ? Which wine version ?

@qparis
Copy link
Member

qparis commented Jan 20, 2019

Ensure that the line returns are the same in both files

@Zemogiter
Copy link
Contributor

Zemogiter commented Jan 21, 2019

I've tried the old script for VulkanSDK and it only created the registry entry for Wow6432Node. I'm going to test your commit then mine. I was sure it was working when I created the PR.
UPDATE: Your commit works the same as the old script while mine fails to create registry entries for both x32 and x64. Maybe we should do it like I did with my Subnautica script before switching to Vulkan.

@ImperatorS79
Copy link
Contributor Author

The two errors are strange since those identifiers in the json have to be like this.

@plata
Copy link
Collaborator

plata commented Jan 27, 2019

It's ok. You may ignore them.

@plata
Copy link
Collaborator

plata commented Jan 27, 2019

@qparis do you want to do the review (you checked also before)?

@ImperatorS79
Copy link
Contributor Author

I think this is ready to be merged.

@Zemogiter
Copy link
Contributor

Zemogiter commented Feb 10, 2019

Just tested that and everything works expect for 32bit registry entry creation. Winetricks works though.

@plata
Copy link
Collaborator

plata commented Feb 11, 2019

@Zemogiter so the 32bit registry issue should be fixed before merging this?

@Zemogiter
Copy link
Contributor

Yes

@plata
Copy link
Collaborator

plata commented Feb 12, 2019

@ImperatorS79 can you do that?

@ImperatorS79
Copy link
Contributor Author

ImperatorS79 commented Feb 12, 2019

I have no idea where this issue come from. Nevertheless, the code was not working at all before this PR, so I suggest to merge this and open a new issue.

@Zemogiter
Copy link
Contributor

OK so here's the thing. I've checked on winetricks source code and saw they did not include "\ in

"\"C:\\\\Windows\\\\winevulkan.json\"=dword:00000000" ;

After I've removed that and installed in osu wineprefix the Drivers key inside Vulkan is created which didn't happen before. Drivers is empty though but this confirms there might be something wrong with the script despite the 64bit section working.

@ImperatorS79
Copy link
Contributor Author

ImperatorS79 commented Feb 12, 2019

This is a \" actually, which just allows to type " in a string delimited by ... "

@plata
Copy link
Collaborator

plata commented Feb 13, 2019

@ImperatorS79 please merge master to resolve the conflict.

@ImperatorS79
Copy link
Contributor Author

ImperatorS79 commented Feb 22, 2019

I will open an issue for the registry key as soon as this is merged.

@plata
Copy link
Collaborator

plata commented Feb 26, 2019

@ImperatorS79 don't forget to add some details to the merge description (not title) when you merge this.

@ImperatorS79 ImperatorS79 merged commit 7a5412a into PhoenicisOrg:master Feb 26, 2019
@ImperatorS79 ImperatorS79 deleted the vulkanSDK branch February 27, 2019 13:00
@Kreyren Kreyren mentioned this pull request Mar 8, 2019
petermetz pushed a commit to petermetz/scripts that referenced this pull request Jun 7, 2020
- Use a fixed sdkVersion variable
- Use JSON.stringify to make winevulkan.json
- Use regedit plugin instead of "wine reg add ..."
- Fix \\ in .reg
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

4 participants