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

Extract platform-specifics from ENV to Crystal::System::Env and implement for win32 #6333

Merged
merged 11 commits into from Jul 24, 2018

Conversation

Projects
None yet
5 participants
@straight-shoota
Contributor

straight-shoota commented Jul 4, 2018

Requires #5623 (for win32).

@straight-shoota straight-shoota referenced this pull request Jul 4, 2018

Open

Coordinate porting to Windows #5430

11 of 21 tasks complete
module Crystal::System::Env
# Sets an environment variable.
def self.set(key : String, value : String) : Nil
raise ArgumentError.new("Key contains null byte") if key.byte_index(0)

This comment has been minimized.

@RX14

RX14 Jul 4, 2018

Member

key.check_no_bull_byte and down the diff

This comment has been minimized.

@straight-shoota

straight-shoota Jul 4, 2018

Contributor

That's what I initially thought, too. But check_no_null_byte loses the information which of the arguments was invalid (it's String contains null byte instead of Key contains null byte). Doesn't make much difference, so I'm not sure about it...

This comment has been minimized.

@RX14

RX14 Jul 7, 2018

Member

Then refactor it to take an argument

System.retry_wstr_buffer do |buffer, small_buf|
length = LibC.GetEnvironmentVariableW(key.to_utf16, buffer, buffer.size)
if 0 < length < buffer.size
break String.from_utf16(buffer[0, length])

This comment has been minimized.

@RX14

RX14 Jul 4, 2018

Member

This is probably slightly clearer using return instead of break.

src/env.cr Outdated
value
end
# Returns `true` if the environment variable named *key* exists and `false`
# if it doesn't.
def self.has_key?(key : String) : Bool
!!LibC.getenv(key)
!!Crystal::System::Env.set?(key)

This comment has been minimized.

@RX14

RX14 Jul 4, 2018

Member

rename this to has_key? and remove the !!

@@ -6,4 +6,6 @@ lib LibC
alias WCHAR = UInt16
alias LPSTR = CHAR*
alias LPWSTR = WCHAR*
alias LPCWSTR = WCHAR*

This comment has been minimized.

@RX14

RX14 Jul 4, 2018

Member

LPCWSTR shouldn't be an alias since it's just constant which means nothing ABI-wise.

This comment has been minimized.

@straight-shoota

straight-shoota Jul 5, 2018

Contributor

So... what should it be?

This comment has been minimized.

@RX14

RX14 Jul 7, 2018

Member

LPWSTR...

@@ -5,6 +5,8 @@ require "c/int_safe"
lib LibC
fun GetLastError : DWORD
ERROR_ENVVAR_NOT_FOUND = 203_u32

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/windows-env branch 2 times, most recently from ca2905f to 7efc63e Jul 5, 2018

straight-shoota added some commits Jul 4, 2018

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/windows-env branch 3 times, most recently from 52da7cc to a68fd42 Jul 5, 2018

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Jul 5, 2018

Rebased after #5623 was merged and ready for review 👍

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/windows-env branch from a68fd42 to 9bf5cb8 Jul 5, 2018

#
# Invalid values are encoded using the unicode replacement char with
# codepoint `0xfffd`.
def self.from_utf16(pointer : Pointer(UInt16)) : {String, Pointer(UInt16)}

This comment has been minimized.

@RX14

RX14 Jul 7, 2018

Member

This is a breaking change and as such needs some more discussion.

This comment has been minimized.

@straight-shoota

straight-shoota Jul 8, 2018

Contributor

Sure. When reading from a pointer it needs to advance the amount of UInt16 read. We can't determine that afterwards from String#bytesize because UTF-8 and UTF-16 have different bytesizes. We could rename the method with this changed behaviour (to maybe String.from_utf16_advancing_pointer). Or change the signature in some way. But I don't think it makes much sense to have both a version of this method (with pointer) that returns the advanced pointer and one that doesn't. It's easy to just ignore the returned pointer value if you don't need it.

@asterite

This comment has been minimized.

Contributor

asterite commented Jul 8, 2018

This is why I recommended keeping the UTF-16 implementation internal until we got it working well in Windows, otherwise we are bound to make breaking changes.

I'd say for now don't worry about breaking changes here, I doubt anyone is using these methods, and if they are, well, it's a simple fix.

@RX14

This comment has been minimized.

Member

RX14 commented Jul 8, 2018

I'm fine with this breaking change actually

def check_no_null_byte(name = nil)
if byte_index(0)
name = "`#{name}` "
raise ArgumentError.new("String #{name}contains null byte")

This comment has been minimized.

@RX14

RX14 Jul 8, 2018

Member

Why is this on two lines??

Also, I'd rather the entire exception message was passed in, i.e. def check_no_null_byte(message = "String contains null byte").

This comment has been minimized.

@asterite

asterite Jul 8, 2018

Contributor

I agree with the first comment, disagree with the second one.

This comment has been minimized.

@RX14

RX14 Jul 8, 2018

Member

The only thing you save by doing it the other way is a bit of typing, and the benefit of having fully customized error messages. For "Key contains null byte" vs "String `key` contains null byte" - the former is way better. Please, let's keep it simple, understandable and flexible.

This comment has been minimized.

@straight-shoota

straight-shoota Jul 8, 2018

Contributor

Two lines because I forgot a if name in the first line. To avoid empty argument to be printed. That's why the specs fail.

Maybe full message is better. But actually, I don't think there is much benefit from it. Yes, it's more flexible, but I don't see any use for that. check_no_null_byte is only used to validate method arguments and for such it makes sense to declare the name of the argument if there are more than one. But apart from that, I don't see any need for customization.

fixup! Extract Crystal::System::Env interface
Refactor String#check_no_null_byte

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/windows-env branch from ec4b780 to 5de7a7a Jul 8, 2018

raise ArgumentError.new("String contains null byte") if byte_index(0)
def check_no_null_byte(name = nil)
if byte_index(0)
name = "`#{name}` " if name

This comment has been minimized.

@ysbaddaden

ysbaddaden Jul 9, 2018

Member

Why the backticks?

This comment has been minimized.

@straight-shoota

straight-shoota Jul 9, 2018

Contributor

To show that this refers to a name. This formatting is used in many other compiler and stdlib error messages.

This comment has been minimized.

@ysbaddaden

ysbaddaden Jul 9, 2018

Member

Weird, I never noticed that.

This comment has been minimized.

@RX14

RX14 Jul 9, 2018

Member

I think it's not that common

This comment has been minimized.

@Sija

Sija Jul 9, 2018

Contributor

IMHO Key contains null byte reads way better than String 'key' contains null byte.

This comment has been minimized.

@straight-shoota

straight-shoota Jul 9, 2018

Contributor

@asterite Do you mean name should be required?

This comment has been minimized.

@RX14

RX14 Jul 9, 2018

Member

Perhaps check_no_null_byte(what = "String") then just #{what} contains null byte?

Or just go with making it fully configurable.

This comment has been minimized.

@asterite

asterite Jul 9, 2018

Contributor

I'm looking at the code, it always seems to be a check against an argument. To avoid repeating the argument name, maybe we can have:

class String
  macro check_no_null_byte(name)
    raise ArgumentError.new("Argument '{{name}}' contains null byte") if {{name}}.byte_index(0)
    {{name}}
  end
end

Then it can be used like this:

def some_method(arg)
  String.check_no_null_byte(arg)
  # do something
end

That way there's no string interpolation, and there's no need to repeat the argument name.

This comment has been minimized.

@RX14

RX14 Jul 9, 2018

Member

This is bikeshedding and overengineering. The diff is fine as-is.

This comment has been minimized.

@straight-shoota

straight-shoota Jul 9, 2018

Contributor

@RX14 I was writing the same thing. All are valid options and it doesn't make a difference regarding ENV.
Anyone can send a PR later to refactor check_no_null_byte if you care enough.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/windows-env branch from 5de7a7a to 2d01d82 Jul 9, 2018

fixup! Implement Crystal::System::Env for win32
Use String#check_no_null_byte

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/windows-env branch from 2d01d82 to 7adb2cc Jul 9, 2018

@RX14

RX14 approved these changes Jul 9, 2018

straight-shoota added some commits Jul 10, 2018

fixup! Implement Crystal::System::Env for win32
Add spec for empty environment variable. This fails on win32 because `GetEnvironmentVariableW` doesnt differentiate between unset variable and empty string.
In contrast, `GetEnvironmentStringsW` contains empty variables.
@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Jul 10, 2018

I discovered an issue on win32: GetEnvironmentVariableW doesn't differentiate between unset variable and empty string. Both have length == 0 and return value ERROR_ENVVAR_NOT_FOUND. The latter seems incorrect in case of an empty string value. In contrast, GetEnvironmentStringsW contains variables with empty values.

This could be used to identify empty values if length == 0 but will of course have a performance impact, and it's probably not really worth it for this edge case. There will however be a discrepancy between ENV[] and ENV.each regarding empty strings.

Unless someone has a idea how to prevent this, I don't think there is anything to be done.

@RX14

This comment has been minimized.

Member

RX14 commented Jul 11, 2018

@straight-shoota i'm pretty skeptical that GetEnvironmentVariable is behaving this way. Can you print the result of GetLastError (pp WinError.new) before and directly after the API call?

@RX14

This comment has been minimized.

Member

RX14 commented Jul 11, 2018

This stackoverflow answer seems to show the Win32 API correctly returning the value of an empty env var: https://stackoverflow.com/questions/26993642/call-to-environment-getenvironmentvariable-affects-subsequent-calls

But it does show there's definitely weirdness around this part of the API - perhaps GetEnvironmentVariable is failing to clear GetLastError correctly as all syscalls should.

fixup! fixup! Implement Crystal::System::Env for win32
Fix implementation for empty string on win32 using `SetLastError`.
@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Jul 11, 2018

It actually seems to be the case that GetEnvironmentVariableW doesn't clear the last error. GetLastError is even ERROR_ENVVAR_NOT_FOUND if a positive length was returned.
And indeed, that's documented for SetLastError:

Most functions call SetLastError or SetLastErrorEx only when they fail. However, some system functions call SetLastError or SetLastErrorEx under conditions of success; those cases are noted in each function's documentation.

I fixed it by adding a call to SetLastError to set it to 0 before the call to GetEnvironmentVariableW. Now everything works 👍

@RX14

RX14 approved these changes Jul 11, 2018

@RX14

This comment has been minimized.

Member

RX14 commented Jul 11, 2018

Oh actually, can you uncomment out the file/dir specs which were commented out due to using ENV?

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Jul 11, 2018

As far as I can see, they're all pending for other reasons.

@RX14

This comment has been minimized.

Member

RX14 commented Jul 11, 2018

@straight-shoota these should go from macroed out to just pending

# TODO: these specs don't compile on windows because ENV isn't ported

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/windows-env branch from 66166cb to 0b33619 Jul 11, 2018

@@ -582,41 +582,38 @@ describe "File" do
File.expand_path("~/a", "/tmp/gumby/ddd").should eq(File.join([home, "a"]))
end
# TODO: these specs don't compile on windows because ENV isn't ported
# TODO: remove /\A\/\// hack after this is removed from macros

This comment has been minimized.

@RX14

RX14 Jul 11, 2018

Member

please check the diff for #5623 and do this TODO too please

This comment has been minimized.

@straight-shoota

straight-shoota Jul 11, 2018

Contributor

Sorry, I'm not sure what you mean... reinstating %r{\A//}?

This comment has been minimized.

@RX14

RX14 Jul 12, 2018

Member

yes, and remove the TODO comment

@RX14

RX14 approved these changes Jul 12, 2018

@RX14

This comment has been minimized.

Member

RX14 commented Jul 24, 2018

ping, needs a second review

@RX14 RX14 added this to the Next milestone Jul 24, 2018

@RX14 RX14 merged commit 4f720ab into crystal-lang:master Jul 24, 2018

3 of 4 checks passed

ci/circleci: test_linux Your tests failed on CircleCI
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@straight-shoota straight-shoota deleted the straight-shoota:jm/feature/windows-env branch Jul 24, 2018

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Jul 26, 2018

@RX14 The commit message is a mess... it should've been squashed without all these fixups.

@RX14 RX14 modified the milestones: Next, 0.26.0 Jul 30, 2018

@straight-shoota straight-shoota referenced this pull request Aug 1, 2018

Merged

Release 0.26.0 #6475

@straight-shoota straight-shoota referenced this pull request Aug 7, 2018

Merged

Fix ENV#each #6499

omarroth added a commit to omarroth/crystal that referenced this pull request Nov 5, 2018

Extract platform-specifics from ENV to Crystal::System::Env and imple…
…ment for win32 (crystal-lang#6333)

* Extract  Crystal::System::Env interface

* Implement Crystal::System::Env for win32

* Fix use correct bytesize of UTF-16 strings

* fixup! Implement Crystal::System::Env for win32

Remove LPCWSTR

* fixup! Extract  Crystal::System::Env interface

Refactor String#check_no_null_byte

* fixup! Implement Crystal::System::Env for win32

Use String#check_no_null_byte

* fixup! Fix use correct bytesize of UTF-16 strings

* fixup! Implement Crystal::System::Env for win32

Add spec for empty environment variable. This fails on win32 because `GetEnvironmentVariableW` doesnt differentiate between unset variable and empty string.
In contrast, `GetEnvironmentStringsW` contains empty variables.

* fixup! fixup! Implement Crystal::System::Env for win32

Fix implementation for empty string on win32 using `SetLastError`.

* Uncomment specs depending on ENV in file_specs

* fixup! Uncomment specs depending on ENV in file_specs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment