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

Remove unused kconv require #533

Merged
merged 1 commit into from
Apr 1, 2021

Conversation

deepj
Copy link
Contributor

@deepj deepj commented Nov 2, 2018

It seems to be require 'kconv' (Kanji converter in the standard Ruby libraries) is not used anywhere at all.

This found in truffle-ruby which hasn't been kconv yet in this issue oracle/truffleruby#1439

This thing was introduced in this commit 5c8e802

@eregon
Copy link

eregon commented Nov 2, 2018

The CI seems to reveal that mechanize doesn't seem to use the kconv library itself, but ntlm-http 0.1.1 does:

  1) Error:
TestMechanizeHttpAgent#test_response_authenticate_ntlm:
NameError: uninitialized constant Module::Kconv
    /home/travis/.rvm/gems/ruby-2.2.7/gems/ntlm-http-0.1.1/lib/net/ntlm.rb:103:in `encode_utf16le'

That seems a pretty old gem, and I can't find the source repository easily.
That gem should of course take care of require-ing its own dependencies, but somehow does not.

The source from the unpacked gem looks like:

      def encode_utf16le(str)
        swap16(Kconv.kconv(str, Kconv::UTF16, Kconv::ASCII))
      end
      
      def swap16(str)
        str.unpack("v*").pack("n*")
      end

That might maybe be simplified by using String#encode nowadays, but I'm not sure of the semantics of kconv.

On the mechanize side, it looks like the ntlm-http dependency could be lazily loaded in

type_2 = Net::NTLM::Message.decode64 challenge.params

Which would avoid requiring both ntlm-http and kconv eagerly.

What do the mechanize authors think?

But at this point I of course realize Ruby implementations should just support kconv since it's a standard library still distributed in latest MRI.

@deepj
Copy link
Contributor Author

deepj commented Nov 3, 2018

@eregon https://rubygems.org/gems/ntlm-http the last version (and only one) was released almost 10 years ago, and it seems it won't be fixed. Alternatively, mechanize can use a different library for NTLM support though.

@flavorjones flavorjones added this to the v2.8.0 milestone Feb 1, 2021
@flavorjones
Copy link
Member

Related: #565

@flavorjones
Copy link
Member

I've rebased this off master to ensure CI runs.

Base automatically changed from master to main February 7, 2021 19:22
@flavorjones
Copy link
Member

Possibly related: #574 removed the dependency on ntlm-http and replaced it with rubyntlm. Rebasing now to see what CI thinks.

@flavorjones flavorjones force-pushed the remove-kconv-require branch from 4dd56a3 to 1b1b227 Compare April 1, 2021 15:38
@deepj
Copy link
Contributor Author

deepj commented Apr 1, 2021

@flavorjones it looks good! :)

@flavorjones flavorjones merged commit 35b72e8 into sparklemotion:main Apr 1, 2021
@flavorjones
Copy link
Member

Will be in the next release, v2.8.0

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.

3 participants