-
Notifications
You must be signed in to change notification settings - Fork 6
fix(search,windows): show installed location for all config levels #207
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
base: main
Are you sure you want to change the base?
fix(search,windows): show installed location for all config levels #207
Conversation
|
|
||
| // For drivers in the registry, set FilePath to the registry key instead | ||
| // of the filesystem path since that's technically where the driver exists. | ||
| di.FilePath = fmt.Sprintf("%s\\%s", lvl.rootKeyString(), regKeyADBC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have k and dkey, can't we get the full key names from those instead of needing the new function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried and failed to get a name from the k (which appears to be just a handle not a rich object). I'll try again though since it'd be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, you're right. That really sucks, there's no way to get the current name of the key from the handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked again and I don't see how. It looks like keys are comparable so we can at least simplify it a bit. I'd be happy if you took a look at the package too just in case I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 106e5c3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hrm, maybe not. the new change results in this,
Installed Versions:
╰── 1.8.0
├── user => UNK\SOFTWARE\ADBC\Drivers
├── system => UNK\SOFTWARE\ADBC\Drivers
╰── env => C:\Users\Bryce\src\columnar-tech\dbc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, my first attempt I was comparing the wrong handles. But comparing the right handles would require changing the function signature of driverInfoFromKey or moving setting FileInfo out of driverInfoFromKey. So driverInfoFromKey unfortunately has to be modified.
I like what I have now enough. Take another look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any way we can add a test for this? otherwise, this looks good to me
Should we add tests for CI for this case? |
Yeah we should. I'll resurrect the PR for testing user and system, IIRC it's basically ready. |
|
Latest output |
|
Let me file another PR to fix the test failure. |
|
Filed as #212. We can merge that first if we want. |
e997658 to
cda221d
Compare
|
Rebased. |
Fixes an issue where the driver install location wasn't showing up when a user runs
dbc search -von Windows. This was only impacting user and system level installs and the reason we didn't catch this is that we don't test those levels in CI.This PR updates the Windows code paths that set the FileInfo member on the DriverInfo struct so it's set to the registry root for the appropriate level. We just weren't setting it before for registry user and system levels. I went with the registry key root instead of the location of the DLL because the driver is really installed to the registry and we don't want users thinking they can uninstall a driver from %APPDATA% or something.
Here's an example of what this looks like on my system:
Closes #203