Implemented #2770

[paulkass]

Implemented #2770.

Processed the hostname of the server in the OpenSSL Client with punycode to support non-ASCII characters in hostnames. I used punycode from #2543 and also used the same method he used for implementing this (addrinfo.cr).

First PR, feel free to rip it apart :).

[RX14]

This actually breaks the build: @hostname : String was previously inferred from this method, now adding this method call breaks that inference. So the instance variable type restriction needs to be manually added.

[paulkass]

@asterite sorry habit :)

[paulkass]

What's interesting is that the error that I get here happens even when I take out all of the changes that I made. After moving the file back to before my initial commit, the same error persisted on my local machine (running make std_spec gave an error about Undefined symbols for architecture x86_64).

[RX14]

@paulkass can you please paste the error you get when you run make std_spec?

[paulkass]

@RX14

Using /usr/local/bin/llvm-config [version=6.0.0]
./bin/crystal build  -o .build/std_spec spec/std_spec.cr
Undefined symbols for architecture x86_64:
  "_LLVMInitializeInstCombine", referenced from:
      _*LLVM::PassRegistry#initialize_inst_combine:Nil in L-L-V-M-5858P-assR-egistry.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Error: execution of command failed with code: 1: `cc "${@}" -o '/Users/<name>/crystal-scripts/crystal/.build/std_spec'  -rdynamic  -lxml2 -lyaml -lreadline `command -v pkg-config > /dev/null && pkg-config --libs --silence-errors libssl || printf %s '-lssl -lcrypto'` `command -v pkg-config > /dev/null && pkg-config --libs --silence-errors libcrypto || printf %s '-lcrypto'` /Users/<name>/crystal-scripts/crystal/src/llvm/ext/llvm_ext.o `/usr/local/bin/llvm-config --libs --system-libs --ldflags 2> /dev/null` -lstdc++ -lgmp -lz -lpcre -lgc -lpthread /Users/<name>/crystal-scripts/crystal/src/ext/libcrystal.a -levent -liconv -ldl -L/usr/lib -L/usr/local/lib`
make: *** [.build/std_spec] Error 1```

[RX14]

make clean std_spec

[paulkass]

@RX14 error persists after running that. Can it be that that's because my computer does not support the x86_64 architecture? Also, the Travis CI build is failing but it's failing in the compiler, and I didn't touch the compiler at all so I don't know what that issue is all about.

[asterite]

Re-run CI, now it passes. It was a hiccup.

[asterite]

@paulkass I think Crystal is not compatible with LLVM 6 yet, only LLVM 5.

[paulkass]

oh nice! thanks @asterite! i guess i'll revert to an older version then.

[asterite]

@paulcsmith Please don't set the default value of "". Just set it in the only place that's needed. For example it's not needed in the method that uses punicode. If later we add another initialize it's easy to forget to initialize @hostname. So always before doing changes, please ask why :-)

[paulcsmith]

@asterite I think you meant to ping @paulkass :)

[paulkass]

@asterite I'm kind of confused about this as we need to initialize @hostname somewhere (otherwise we get a compile time error). So the definition of the instance variable seems like the most logical place to me. Without variable restrictions (which must be done at the class level) it won't compile as @RX14 said.

[asterite]

But if you initialize it there, the compiler won't detect when you don't initialize it later. In one of the methods it's initialized from the method argument, so putting first an empty string and then overwriting it is not the best thing to do. It's a minor thing, but the way you did it originally was better.

This can be merged as it is right now, but usually reviewers are more strict, I don't know why they are not in this particular case.

[paulkass]

@asterite I've played around a bit, and it seems like "casting" (adding a .as(String)) will do the trick. Will that work better?

[asterite]

Casting? What do you mean?

[paulkass]

More like "casting". Instead of declaring @hostname and it's type at the class level, we just assign it in the method with @hostname = hostname.as(String).

[asterite]

Yes, that's similar but probably less clear. I'd go with a type declaration.

[asterite]

Actually, a type declaration is better. If you use that cast, but in another method assign another type that the compiler can infer, it will result in a union. So please use a type declaration.

[paulkass]

Yeah, I don't think I know a way to get a type declaration without some sort of initialization somewhere (at the class level or initializate method). I tried doing a type declaration in the method and at the class level without initialization, and they both gave errors.

[asterite]

@hostname : String

That should work

[paulkass]

@asterite That gives me this error:

Error in src/openssl/ssl/context.cr:78: instance variable '@hostname' of 
\ OpenSSL::SSL::Context::Client was not initialized directly 
\ in all of the 'initialize' methods, rendering it nilable```

[asterite]

Exactly. Now to fix that initialize where hostname is not properly initialized ;-)

[paulkass]

@asterite I dug through the docs (again) and I found the uninitialized keyword. It's marked as unsafe, so I'm not sure if this is what you're hinting at. :)

[jhass]

It's just to keep a reference, so just make it nilable

[asterite]

Oh, the hostname isn't always assigned? If so, an empty string is good.

[asterite]

Or, yeah, it can be nilable. I'll review this in a few hours...

[RX14]

Crystal supports LLVM 6 just fine, it just needs a single bugfix patch applied which arch linux has kindly done. Someone accidentally unexported a symbol in a refactor commit. I was expecting LLVM to release 6.0.1 but they haven't for some reason despite an obvious regression. Someone should do the same for homebrew.

$ crystal -v
Crystal 0.25.1 (2018-06-29)

LLVM: 6.0.0
Default target: x86_64-pc-linux-gnu

[asterite]

I just checked this, and the current type of @hostname is String | Nil. So it's better to keep it that way.

@paulkass Please use this type annotation:

@hostname : String?

[paulkass]

@asterite alright there we go!

[paulkass]

oops sorry about closing this