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

New 'listWindowsRegistry' OS API method for enumerating content of single registry subkey #1345

Merged
merged 1 commit into from
Oct 14, 2019

Conversation

kaldap
Copy link

@kaldap kaldap commented Oct 10, 2019

What does this PR do?

This PR is extending the 'os' API by adding new method called 'listWindowsRegistry' which returns table of all subkeys and values in the specified path. Nil is returned in the case of failure.

How does this PR change Premake's behavior?

No breaking changes, just extended API.

Anything else we should know?

  • Useful in some cases, f.e. automatic selection of latest GCC toolchain on Windows.
  • Only REG_SZ and REG_DWORD values are covered in test suite - did not find any reliable values of other types. But I did manual testing of all value types, so it should work properly.

Did you check all the boxes?
Yes I did with single exception. Method name is in camel-case. I did it on purpose to keep it consistent with existing 'getWindowsRegistry' method.

Copy link
Member

@starkos starkos left a comment

Choose a reason for hiding this comment

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

Camel case is good, we’re moving in that direction anyway. But please use it consistently: you’re using a mix of camelCase and snake_case in the C code. FWIW, I’d personally prefer getWindowsRegistry to listWindowsRegistry. And we’ll need to get the AppVeyor CI tests passing, if you get a chance to check the output and see what’s failing.

This is a nice addition, thanks so much for the contribution!

@kaldap
Copy link
Author

kaldap commented Oct 10, 2019

Found a registry value for a DWORD test which should be around there for some time already, so hopefully should make the test passing on all supported Windows versions.

What do you mean by "I’d personally prefer getWindowsRegistry to listWindowsRegistry"? Merge those two methods into single one with different behaviour for keys/values?

Copy link
Member

@starkos starkos left a comment

Choose a reason for hiding this comment

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

Back at my desk now, I added some comments inline.

src/host/os_getWindowsRegistry.c Outdated Show resolved Hide resolved
typedef void (*ListCallback)(const RegNodeInfo * info, void * user);
extern HKEY get_registry_key(const char** path);

static const char* get_type_string(DWORD type)
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this getTypeString?

src/host/os_listWindowsRegistry.c Outdated Show resolved Hide resolved
src/host/os_listWindowsRegistry.c Show resolved Hide resolved
@starkos
Copy link
Member

starkos commented Oct 14, 2019

Looking good. If you could rebase against the latest master branch (and if you could squash the commits too, that would be a bonus), this looks ready to merge.

@starkos starkos merged commit 3f0cf0b into premake:master Oct 14, 2019
@starkos
Copy link
Member

starkos commented Oct 14, 2019

Merged. Thanks for this!

@kaldap kaldap deleted the registry branch October 15, 2019 07:50
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.

2 participants