Implement Windows Registry API #6698

straight-shoota · 2018-09-11T14:16:04Z

This PR adds an API for accessing the Windows Registry.

Access to the Registry is required in the standard library to get proper time zone names (see Crystal::System::Time.translate_zone_name, included in this PR) and MIME type mappings (extension to #5765, not yet included).

However, the Registry API is obviously only available on win32. Previous discussions (most recently in #6668) have manifested a common understanding that the Crystal standard library should be as much cross-platform as possible.
Having a highly platform-specific Registry API conflicts that proposition. Still, the stdlib needs that API to provide an implementation on win32 for cross-platform features.

I considered stripping down the implementation and moving it to the Crystal::System namespace. This would only contain the bare minimum required to implement the missing features for accessing the time and mime databases in the registry. But they already require a huge portion of the features and thus doesn't seem to make much sense as a stripped-down version.

Looking at Go, they have the registry API in an external package golang.org/x/sys/windows/registry which needs to be imported to use it in user code. But, they also need the registry for the same stdlib implementations and thus provide a copy of that package in internal/syscall/windows/registry which is only used by the internal implementation.
We could probably go a similar way, but I'm not too happy about maintaining the same code in two repositories.
Maybe it could be somewhat manageable using git subtrees or submodules, but I don't have a clear strategy in mind.

So, before reviewing the implementation of this PR, please consider options for how to include this into the stdlib.

This is a prerequisite for registry's EXPAND_SZ data type.

This is a prerequisite for registry API.

asterite · 2018-09-11T14:25:18Z

src/registry.cr

+# * `HKEY_LOCAL_MACHINE`
+# * `HKEY_USERS`
+# * `HKEY_CURRENT_CONFIG`
+module Registry


This should be in a Windows namespace.

src/crystal/system/win32/time.cr

src/registry.cr

wooster0 · 2018-09-11T15:07:23Z

src/lib_c/x86_64-windows-msvc/c/winreg.cr

+    # Windows 2000: This flag is not supported.
+    WOW64_64KEY = 0x00100
+
+    # Combines the STANDARD_RIGHTS_WRITE, `KEY_SET_VALUE`, and `KEY_CREATE_SUB_KEY` access rights.


` is missing around STANDARD_RIGHTS_WRITE.

wooster0 · 2018-09-11T15:11:44Z

src/registry.cr

+
+      getter value_count : UInt32
+      # size of the key's longest value name, in Unicode characters, not including the terminating null byte.
+


Newline between comment and getter

RX14 · 2018-09-11T15:25:15Z

Please make this a shard. A fully-featured registry wrapper shouldn't be required to access this for timezones, it should be possible to do this using a few raw libc calls. It'll be ugly but thats far better than implementing a purely windows API.

After all, they do that in C.

straight-shoota · 2018-09-11T15:45:44Z

it should be possible to do this using a few raw libc calls.

Yes, it should be possible. But including all error handling etc. around these libc calls, such a bare-bone implementation is far from trivial and would include most of the functionality proposed in this PR. Thus it seems much better to have a full-fledged and fully tested implementation than an ugly purpose-specific one.

RX14 · 2018-09-11T16:06:49Z

Whatever option gets chosen, I don't want a registry or anything 100% platform-specific exposed directly to the user.

I'd even rather shell out to reg.exe than expose something like this to users outside a shard. And shelling out is a lot more reasonable on windows where paths to executables are fixed

ysbaddaden · 2018-09-11T17:31:48Z

I'm fine with a Windows::Registry class only available for Windows. Ruby has Win32::Registry in stdlib, Python has _winreg in stdlib, Go has an official package golang.org/x/sys/windows/registry copied as src/internal/windows/registry in stdlib.

If we only need to read from the registry to extract data in core/stdlib, then maybe we could have a stripped down Crystal::System::Registry with only read methods.

Anyway, having a properly implemented and tested Registry class is a better idea than shelling out to reg.exe or wrapping C calls over and over (increasing bugs), whatever if it's private or public.

RX14 · 2018-09-11T19:14:42Z

@ysbaddaden i'm fine with whatever as long as it's in Crystal::System and undocumented, but I think it's certainly possible to have a "nice enough" interface that's very mininal: no structs with methods, no OO, just calling methods that return handles/strings and raise. This also means minimal effect on the compile time of small programs on windows.

bew · 2018-09-11T20:29:17Z

i'm fine with whatever as long as it's in Crystal::System and undocumented

Or documented for us, but not public (please!)

straight-shoota · 2018-09-13T20:38:30Z

I don't want a registry or anything 100% platform-specific exposed directly to the user.

What about UNIXSocket? 🤷‍♂️

asterite · 2018-09-13T20:52:02Z

I'm fine with having a full binding to the windows registry in the std. It's not much code either, just 600 lines of code which include comments.

RX14 · 2018-09-14T12:00:44Z

@straight-shoota https://docs.microsoft.com/en-us/windows/desktop/ipc/named-pipes can substitute for unix sockets most of the time. Might be worth abstracting over both and removing unix sockets in name.

ysbaddaden · 2018-09-14T12:19:14Z

AF_UNIX is coming to windows;
don't "abstract" something as fundamental as UNIX sockets under some shady new term just because "it's not cross platform"; that would just bring confusion;
let's get back on topic: the windows registry.

RX14 · 2018-09-14T12:30:30Z

@ysbaddaden thanks for the info! Nice to hear that supporting real unix sockets on windows is possible. However, that further underlines my point that bringing something as intensely platform specific as a user-visible registry implementation into the stdlib, let alone the prelude would be unprecedented and unnecessary.

Fryguy · 2018-11-08T18:40:01Z

src/lib_c/x86_64-windows-msvc/c/winreg.cr

+    BINARY                     = 3
+    DWORD                      = 4
+    DWORD_LITTLE_ENDIAN        = DWORD
+    DWORD_BIG_ENDIAN           =  5


minor, but weird alignment here (though it's probably crystal tool format)... the 4 is aligned different from the 5, likely because the DWORD breaks up the 2 sequences.

Fryguy · 2018-11-08T18:41:45Z

src/registry.cr

+# * `HKEY_CURRENT_CONFIG`
+module Registry
+  # Information about file types and their properties.
+  CLASSES_ROOT = Key.new(LibC::HKEY_CLASSES_ROOT, "HKEY_CLASSES_ROOT")


I'm curious why you removed the HKEY_ prefix on the const name? As a user of this module I'd expect to find it with the HKEY_ prefix

Because they're redundant. There are no namespaces in C, hence constants and functions are often prefixed by a type/library identifier (like HKEY). In Crystal it's obvious that they are registry constants because they're defined in the Registry module.

I can see your argument, but outside of the C-code context, the HKEY prefix has pervaded into everything. It's even in the GUI regedit to the point that that is the actual name of the key now. That was really the thought process behind my statement of "As a user of this module I'd expect to find it with the HKEY_ prefix"

straight-shoota · 2021-09-14T17:01:59Z

Closing.
The immediate functionality for time zone and mime db support on Windows was resolved by #11137.

If we decide to provide a public API for access to the Windows Registry in stdlib later, we can re-issue this implementation. Otherwise it may serve as a basis for a shard.

straight-shoota added 2 commits September 11, 2018 14:21

Add Crystal::System::Env.expand (windows only)

09f3baf

This is a prerequisite for registry's EXPAND_SZ data type.

Add Crystal::System.retry_buffer (windows only)

905a7cf

This is a prerequisite for registry API.

asterite requested changes Sep 11, 2018

View reviewed changes

straight-shoota added 3 commits September 11, 2018 16:45

Add Registry API

377acc1

Add registry spec for loading time zone name

483e93d

Implement Time::Location.translate_zone_name

94f3fbe

straight-shoota force-pushed the jm/feature/windows-registry branch from b9b2aec to 94f3fbe Compare September 11, 2018 14:46

wooster0 reviewed Sep 11, 2018

View reviewed changes

Fryguy reviewed Nov 8, 2018

View reviewed changes

HertzDevil added kind:feature platform:windows Windows support based on the MSVC toolchain / Win32 API labels Aug 4, 2021

straight-shoota mentioned this pull request Aug 25, 2021

Add internal registry implementation for win32 #11137

Merged

straight-shoota closed this Sep 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Windows Registry API #6698

Implement Windows Registry API #6698

straight-shoota commented Sep 11, 2018 •

edited

Loading

asterite Sep 11, 2018

wooster0 Sep 11, 2018 •

edited

Loading

wooster0 Sep 11, 2018 •

edited

Loading

RX14 commented Sep 11, 2018 •

edited

Loading

straight-shoota commented Sep 11, 2018

RX14 commented Sep 11, 2018 •

edited

Loading

ysbaddaden commented Sep 11, 2018

RX14 commented Sep 11, 2018 •

edited

Loading

bew commented Sep 11, 2018

straight-shoota commented Sep 13, 2018

asterite commented Sep 13, 2018

RX14 commented Sep 14, 2018 •

edited

Loading

ysbaddaden commented Sep 14, 2018

RX14 commented Sep 14, 2018 •

edited

Loading

Fryguy Nov 8, 2018

Fryguy Nov 8, 2018

straight-shoota Nov 8, 2018 •

edited

Loading

Fryguy Nov 20, 2018 •

edited

Loading

straight-shoota commented Sep 14, 2021


		getter value_count : UInt32
		# size of the key's longest value name, in Unicode characters, not including the terminating null byte.

Implement Windows Registry API #6698

Implement Windows Registry API #6698

Conversation

straight-shoota commented Sep 11, 2018 • edited Loading

asterite Sep 11, 2018

Choose a reason for hiding this comment

wooster0 Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

wooster0 Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

RX14 commented Sep 11, 2018 • edited Loading

straight-shoota commented Sep 11, 2018

RX14 commented Sep 11, 2018 • edited Loading

ysbaddaden commented Sep 11, 2018

RX14 commented Sep 11, 2018 • edited Loading

bew commented Sep 11, 2018

straight-shoota commented Sep 13, 2018

asterite commented Sep 13, 2018

RX14 commented Sep 14, 2018 • edited Loading

ysbaddaden commented Sep 14, 2018

RX14 commented Sep 14, 2018 • edited Loading

Fryguy Nov 8, 2018

Choose a reason for hiding this comment

Fryguy Nov 8, 2018

Choose a reason for hiding this comment

straight-shoota Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Fryguy Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

straight-shoota commented Sep 14, 2021

straight-shoota commented Sep 11, 2018 •

edited

Loading

wooster0 Sep 11, 2018 •

edited

Loading

wooster0 Sep 11, 2018 •

edited

Loading

RX14 commented Sep 11, 2018 •

edited

Loading

RX14 commented Sep 11, 2018 •

edited

Loading

RX14 commented Sep 11, 2018 •

edited

Loading

RX14 commented Sep 14, 2018 •

edited

Loading

RX14 commented Sep 14, 2018 •

edited

Loading

straight-shoota Nov 8, 2018 •

edited

Loading

Fryguy Nov 20, 2018 •

edited

Loading