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.

Sign up for GitHub

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

Jump to bottom

Remove needless dependencies #56

Closed
wants to merge 1 commit into from
Closed

Remove needless dependencies #56

wants to merge 1 commit into from

Conversation

eregon
Copy link
Member

@eregon eregon commented Feb 2, 2022

These are default gems, so there is no need to explicitly depend on them,
and depending on them is actually harmful: https://bugs.ruby-lang.org/issues/18567.

@eregon
Remove needless dependencies
441b012 
These are default gems, so there is no need to explicitly depend on them,
and depending on them is actually harmful: https://bugs.ruby-lang.org/issues/18567.
@eregon eregon mentioned this pull request Feb 2, 2022
Closed
@hsbt
Copy link
Member

hsbt commented Feb 21, 2022

I think we need to remove only net-protocol because digest and strscan may remove completely from stdlib in the future. But net-protocol is not.

This was referenced Feb 21, 2022
Merged
Merged
@eregon
Copy link
Member Author

eregon commented Feb 21, 2022

Ah, I thought net-protocol is an external non-stdlib gem too, but indeed it's a default gem.
net-protocol is no problem anyway.

I don't see strscan or digest being removed from stdlib anytime soon/ever (so many programs depend on them, and adding the explicit dependency is annoying, especially for small scripts without Bundler). But you might know better.

@eregon
Copy link
Member Author

eregon commented Feb 21, 2022

Maybe we should have a Redmine ticket to discuss if strscan and digest might be removed from stdlib one day, or if there is no plan for that and therefore we should only need to be explicit when it will be needed?

@nevans
Copy link
Collaborator

nevans commented Feb 21, 2022

I don't know about general rules. My gut instinct is that we should be moving toward explicitly specifying dependencies even on default gems. But it's worthwhile looking at the specific uses in net-imap:

digest and strscan are only used by CRAM-MD5 and DIGEST-MD5 SASL authenticators, which have been deprecated for a decade or so. No one should be using them. We might keep them for "historical purposes", but I intend to submit a PR for #55, making them optional (they won't automatically be loaded by require "net/imap"). Anyway, if there is a bug in digest, it's unlikely those mechanisms will be any better or worse after upgrading! It would be nice to add support for the SCRAM-* mechanisms, but they aren't widely deployed (as far as I can tell). I doubt anyone will contribute that code any time soon. If and when someone does add support, I hope this whole situation will have improved somewhat? The SASL authenticators which I do intend to contribute in the very near future (OAUTHBEARER and XOAUTH2) do not require digest.

With respect to strscan, I've started constructing some sample IMAP response data to use in benchmarks of the ResponseParser. I've started creating comparison lexers 1) using our current case resp; when /regexp/ lexer and some minor variations, 2) using StringScanner, 3) generated by kpeg, and 4) generated by ragel. If the StringScanner approach wins, then I think I can easily promise it can remain backwards compatible with the current StringScanner... at least until this whole situation has a better resolution.

In addition to @hsbt's remark about net-protocol: as far as I can tell, our only dependency on Net::Protocol is ssl_socket_connect. Wouldn't a much better home for that code be in OpenSSL::SSL::SocketForwarder#connect anyway? 🙂

Speaking of openssl: net-imap doesn't require it because it can connect to insecure servers without it. However, in light of RFC8314, perhaps we should add a dependency to openssl in the future. 😉

@nevans
Copy link
Collaborator

nevans commented Feb 21, 2022

shorter version: I'm okay with removing all three dependencies, at least for now.

nevans added a commit that referenced this pull request Feb 22, 2022
@nevans
Do not load deprecated SASL mechanisms, by default
fb21bc3 
n.b. the mechanisms haven't been removed.  They just aren't loaded by
default.  Closes GH-55.

By making these optional, there's no reason to require the `digest` or
`strscan` gems anymore. Closes GH-56.
@nevans nevans mentioned this pull request Feb 22, 2022
Closed
@nevans
Copy link
Collaborator

nevans commented Feb 22, 2022

@eregon In #58, I convert the digest and strscan dependencies to development dependencies. That should remove the end-user pain, even if it doesn't entirely solve the issue for truffleruby, jruby, etc. If these are going to be gems at all, this problem isn't going to go away. Maybe it would be worthwhile to update these gems to install on truffleruby and jruby with a no-op stub?

@hsbt
Copy link
Member

hsbt commented Feb 22, 2022

@nevans #58 seems good strategy for me. I agreed to reduce the dependencies in net-imap.

@eregon
Copy link
Member Author

eregon commented Feb 24, 2022

Yes, that sounds helpful :)
I agree longer term TruffleRuby (& JRuby) need to support all those default gems.
But for TruffleRuby we'd like some time to consider what's the best way for that, and this helps in the meantime + unbreaks existing TruffleRuby releases for users for net-* gems.

@headius
Copy link

headius commented Feb 24, 2022

Maybe it would be worthwhile to update these gems to install on truffleruby and jruby with a no-op stub?

digest already supports JRuby, after some work last fall to move our implementation into the gem.

strscan has an open PR with full JRuby support.

We have been working on these default gems for some time now and intend to support all of the ones that have a JRuby equivalent.

@bjorntrondsen bjorntrondsen mentioned this pull request Jun 17, 2022
Closed
nevans added a commit to nevans/net-imap that referenced this pull request Jul 16, 2022
@nevans @singpolyma
Warn when using deprecated SASL mechanisms
e8be7c3 
Mark obolete SASL mechanisms as deprecated (fixes rubyGH-55):
* Warn every time a deprecated mechanism is used.
* Warnings can be disabled with `warn_deprecation: false`
* delay loading stdgem dependencies until `#initialize`.  Fixes rubyGH-56.
* This is a backwards-compatible alternative to the approach in rubyGH-58
  (don't require and add the deprecated authenticators automatically).
  We can use that incompatible approach in a later version.

Additionally:
* Adds basic tests for every authenticator (to avoid another rubyGH-52!)
* Fixes a frozen string bug in DigestMD5Authenticator.
* By making these optional, there's no reason to require the `digest` or
  `strscan` gems anymore; fixes rubyGH-56.

The DIGEST-MD5 bug was originally reported, tested, and fixed by
@singpolyma here: nevans/net-sasl#3.

Co-authored-by: Stephen Paul Weber <[email protected]>
nevans added a commit that referenced this pull request Jul 16, 2022
@nevans @singpolyma
Warn when using deprecated SASL mechanisms
76ad566 
Mark obolete SASL mechanisms as deprecated (fixes GH-55):
* This is a backwards-compatible alternative to the approach in GH-58
  (don't require and add the deprecated authenticators automatically).
  We can use that incompatible approach in a later version.
* Warn every time a deprecated mechanism is used.
* Warnings can be disabled with `warn_deprecation: false`
* delay loading stdgem dependencies until `#initialize`.  Fixes GH-56.

Additionally:
* Adds basic tests for every authenticator (to avoid another GH-52!)
* Fixes a frozen string bug in DigestMD5Authenticator.
* By making these optional, there's no reason to require the `digest` or
  `strscan` gems anymore; fixes GH-56.

The DIGEST-MD5 bug was originally reported, tested, and fixed by
@singpolyma here: nevans/net-sasl#3.

Co-authored-by: Stephen Paul Weber <[email protected]>
nevans added a commit that referenced this pull request Jul 16, 2022
@nevans @singpolyma
Warn when using deprecated SASL mechanisms
d466c2e 
Mark obolete SASL mechanisms as deprecated (fixes GH-55):
* This is a backwards-compatible alternative to the approach in GH-58
  (don't require and add the deprecated authenticators automatically).
  We can use that incompatible approach in a later version.
* Warn every time a deprecated mechanism is used.
* Warnings can be disabled with `warn_deprecation: false`
* Fixes GH-56: delay loading standard gem dependencies until
  `#initialize`, and convert the gems to development dependencies.

Additionally:
* Adds basic tests for every authenticator (to avoid another GH-52!)
* Fixes a frozen string bug in DigestMD5Authenticator.
* Fixes constant resolution for exceptions in DigestMD5Authenticator.
* Can register an authenticator type that responds to #call (instead of
  #new).  I was originally going to register deprecated authenticators
  with a Proc that required the file and issued a warning, but I decided
  to put everything into the initializer instead.  `#authenticator`
  needed to be updated to safely delegate all args, and I left this in.

The DIGEST-MD5 bug was originally reported, tested, and fixed by
@singpolyma here: nevans/net-sasl#3.

Co-authored-by: Stephen Paul Weber <[email protected]>
@nevans nevans mentioned this pull request Jul 16, 2022
Merged
@hsbt hsbt closed this in #62 Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eregon @hsbt @nevans @headius