Skip to content

Remove needless dependencies - #56

Closed
eregon wants to merge 1 commit into
ruby:masterfrom
eregon:remove-needless-dependencies
Closed

Remove needless dependencies#56
eregon wants to merge 1 commit into
ruby:masterfrom
eregon:remove-needless-dependencies

Conversation

@eregon

@eregoneregon commented Feb 2, 2022

Copy link
Copy Markdown
Member

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.
@eregoneregon mentioned this pull request Feb 2, 2022
Closed
@hsbt

hsbt commented Feb 21, 2022

Copy link
Copy Markdown
Member

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

eregon commented Feb 21, 2022

Copy link
Copy Markdown
MemberAuthor

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

eregon commented Feb 21, 2022

Copy link
Copy Markdown
MemberAuthor

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

nevans commented Feb 21, 2022

Copy link
Copy Markdown
Collaborator

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

nevans commented Feb 21, 2022

Copy link
Copy Markdown
Collaborator

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. ClosesGH-55.
By making these optional, there's no reason to require the `digest` or
`strscan` gems anymore. ClosesGH-56.
@nevansnevans mentioned this pull request Feb 22, 2022
Closed
@nevans

nevans commented Feb 22, 2022

Copy link
Copy Markdown
Collaborator

@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

hsbt commented Feb 22, 2022

Copy link
Copy Markdown
Member

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

@eregon

eregon commented Feb 24, 2022

Copy link
Copy Markdown
MemberAuthor

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

headius commented Feb 24, 2022

Copy link
Copy Markdown

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.

@bjorntrondsenbjorntrondsen 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 (fixesrubyGH-55):
* Warn every time a deprecated mechanism is used.
* Warnings can be disabled with `warn_deprecation: false`
* delay loading stdgem dependencies until `#initialize`. FixesrubyGH-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; fixesrubyGH-56.
The DIGEST-MD5 bug was originally reported, tested, and fixed by
@singpolyma here: nevans/net-sasl#3.
Co-authored-by: Stephen Paul Weber <singpolyma@singpolyma.net>
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 (fixesGH-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`. FixesGH-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; fixesGH-56.
The DIGEST-MD5 bug was originally reported, tested, and fixed by
@singpolyma here: nevans/net-sasl#3.
Co-authored-by: Stephen Paul Weber <singpolyma@singpolyma.net>
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 (fixesGH-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`
* FixesGH-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 <singpolyma@singpolyma.net>
@nevansnevans mentioned this pull request Jul 16, 2022
Merged
@hsbthsbt closed this in #62 Sep 28, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@eregon@hsbt@nevans@headius