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

Expose the internal @signing_key value in RbNaCl::Signatures::Ed25519 as #keypair #135

Merged
merged 1 commit into from
Apr 29, 2016
Merged

Conversation

grempe
Copy link
Contributor

@grempe grempe commented Apr 26, 2016

As per our Twitter DM discussion today. Let me know if you want me to modify anything. Thanks.

@grempe
Copy link
Contributor Author

grempe commented Apr 26, 2016

Looks like the Travis CI build failed due to a gem dependency (listen) that released a few days ago and now requires on ruby ~> 2.2, and not as a result of this pull request.

guard/listen@2e5c409

They pushed the release out within the last few days.

https://rubygems.org/gems/listen

cc: @guard @e2

@tarcieri
Copy link
Contributor

Adding bundler_args: --without development to .travis.yml should solve the problem

@e2
Copy link

e2 commented Apr 26, 2016

About the guard/listen issue: Bundler 1.12 gracefully selects an earlier version of Listen for the given Ruby version. So a build should even work on 1.9.3 (by finding the last supported version of Listen for that Ruby version). Bundler 1.11 still doesn't handle ruby version requirements well (it just fails to install).

A workaround for that could be: gem install bundler --pre.

Anyway ...

Now the problem is: gemspec in a Gemfile adds development dependencies from the *.gemspec file into the :development group.

You can decide which group you want those dev deps to go to with something like this: https://github.com/guard/listen/blob/6ff4b77/Gemfile#L9

I think specifying development deps in a gemspec file is detrimental, but never got anywhere arguing about it. I think just having bundler as a development dependency in a gemspec file is enough.

Everything else should be in the Gemfile (UPDATED).

I recommend adding rake in the global scope of the Gemfile. A gem's Gemfile is for development anyway.

@e2
Copy link

e2 commented Apr 26, 2016

Oh, and for the sake of discussion, pretty much only Ruby 2.3 and 2.2 (since 2.2.5) are "supported".

So testing earlier Ruby versions is a bit unsensible, because those versions have unfixed bugs anyway...

@e2
Copy link

e2 commented Apr 26, 2016

About the missing rspec: I gave up long ago and just copy and paste config files from other projects, like here: https://github.com/guard/listen/blob/master/Gemfile

It's too easy to forget something useful.

@e2
Copy link

e2 commented Apr 26, 2016

LoadError: cannot load such file -- rubocop/rake_task

Been there, done that: https://github.com/guard/listen/blob/master/Rakefile#L6

@e2
Copy link

e2 commented Apr 26, 2016

I'm half-frustrated at how hard these things are to get right on the first shot here.

@e2
Copy link

e2 commented Apr 26, 2016

While you're at it, I'd move these development dependencies to the Gemfile:

https://github.com/cryptosphere/rbnacl/blob/master/rbnacl.gemspec#L25

gem used to install them with the --dev option presumably. But nowadays, a "dev" environment and a "test" environment are different. E.g., you don't need "pry" on a "test" environment.

It's better to have all the dev deps in the Gemfile for managability and clarity. And you're less likely to screw up a *.gemspec while tinkering with development deps.

@grempe
Copy link
Contributor Author

grempe commented Apr 26, 2016

OK, I deleted all of the Gemfile related commits and now this branch should only contain my single commit that is the subject of the pull request. I think I would just remove guard-rspec from the Gemfile (along with the listen dependency) and be done with it.

https://github.com/cryptosphere/rbnacl/blob/master/Gemfile#L7

@e2, I am curious, did you bump the min version of listen purely since MRI <2.2 is not officially supported anymore, or due to real breakage in your gem for versions less than that? I can't imagine this is the only gem that will run into this issue with your much more stringent requirement for >2.2.

@e2
Copy link

e2 commented Apr 26, 2016

@grempe - long, long, long story short ... the Ruby community is broken and I tried to fix it.

It was the result of weighing the tradeoffs. In terms of SemVer it's correct. It doesn't affect users, only Travis builds. In terms of devops, it's not an error, it's a provisioning failure (which is a good thing!).

Overall, people should upgrade to Ruby 2.2 - regardless whether listen "demands" that version or not. It doesn't make sense to upgrade Listen (minor bump) and not upgrade Ruby (minor bump). There's no reason why Ruby shouldn't be treated like a gem.

It's a long, long, long discussion and there isn't anything wrong other than people's weird reluctance to upgrade to Ruby 2.2. If OS's have outdated Rubies, that's a different problem.

There's also a ton of cult/dogma around why what I did is "breakage" - few people actually put their thinking hats on. Even in terms of SemVer what I did is correct.

Again, the Ruby community is broken. Users affected: 0. Devops freaking out that their Travis builds broke on Ruby 1.9.3 - just a few. People mindlessly upgrading everything except Ruby: 100%. Gem owners mindlessly removing Listen or locking to ~> 3.0.x ... a few. Users actually hurt by this: 0.

People freaking out: 100%.

Summary: it's an enormous subject and ultimately, there isn't a single thing wrong with what I did. (No compelling/practical example proving me otherwise).

@e2
Copy link

e2 commented Apr 26, 2016

@grempe - I'd do this instead:

  1. Rely only on the test group on Travis:
group :test do
  gem "rake"
  gem "rspec"
  gem "coveralls", require: false
  gem "rbnacl-libsodium", ENV["LIBSODIUM_VERSION"]
end
  1. Use the Rakefile patch I mentioned above for RuboCop (unless you want to run RuboCop on Travis, which makes little sense I think).
  2. Move the development deps out of the gemspec - they're confusing as heck. You can leave only Bundler at e.g. '~> 1.11'. Because bundler is the only development dep RubyGems should even know about. All other dev deps can be installed using bundler and the Gemfile.
  3. Best to turn off coverage in CI, see: https://github.com/guard/listen/blob/master/spec/spec_helper.rb#L12

And I think that's it.

@e2
Copy link

e2 commented Apr 27, 2016

A few hints:

  • the add_development_dependency is just useless metadata that shows up on the RubyGems home page.
  • Rails has a test and development environment. They're separate. That's a good practice for gems too.
  • politically, you're either in the "bundler camp" (meaning: using a Gemfile for deps) or in the RubyGems camp (meaning: using something like hoe to manage the gem). I'd go for Bundler+Gemfile, so the rbnacl.gemspec shouldn't have any gem dev dependencies.

As for the Listen, the "true story" is that only Ruby 2.2.2 was tested on Travis on Listen 3.x. The ruby requirement in the gemspec was >= 1.9.3.

So today, I created this gem to solve this: https://github.com/e2/ruby_dep

Basically, it sets the required ruby constraint based on what's being tested by Travis.

So if a gem is supposed to work on Ruby 2.3, 2.2, 2.1, 2.0 - then all those versions should be tested on Travis.

The problem with Listen is: it's integration test heavy, tests are brittle (depend on Travis load sometimes), they have lots of sleeps (timing issues), and the build matrix was just too huge for the PRs ahead.

I didn't predict that bundle install would fail to automatically install an earlier version of Listen for an earlier version of Ruby.

The biggest misunderstanding is: that since Listen 3.x worked on 1.9.3 before ... that I "broke" something. Actually, 1.9.3 was unsupported, even if it "happened to work".

And bumping a major version in Listen wouldn't make sense, because there was absolutely no change in the API. There's a slight practical difference in SemVer for interpreted code vs compiled code.

Anyway, end of rant. The subject is enormously complex and goes way deeper than "OMG! Build doesn't work! Must be a SemVer violation!".

@tarcieri
Copy link
Contributor

tarcieri commented Apr 27, 2016

I think the fix looks like this:

  • .travis.ci: bundler_args: --without development
  • Gemfile:
group :development, :test do
 +  gem "rake"
end

(I will commit this tonight)

@e2 the solutions to these problems can really be rather simple and don't need to involve accusations of the Ruby community being broken or all the gem boilerplate being wrong. You may disagree about the specifics but those are really just matters of opinion. While this project doesn't have a code of conduct yet, I really don't appreciate comments like "the Ruby community is broken and I tried to fix it" on PRs. Let's keep it technical please.

@e2
Copy link

e2 commented Apr 27, 2016

I think the fix looks like this:

You need to add rspec because --without development will ignore the dev deps in the rbnacl.gemspec file. And coveralls, unless you guard it with the CI env checking.

@e2
Copy link

e2 commented Apr 27, 2016

@e2 the solutions to these problems can really be rather simple

E.g. the "simple" solution here to "the problem I caused" is either:

  1. Using pre-release version of Bundler
  2. Updating to Ruby 2.2 or 2.3

And many people are unwilling to even entertain the idea of any of those two solutions.

So simple solutions become complex when people overreact to the symptoms and make "assumptions" without digging in deeper. And this is what happened in this case. (I don't mean this PR here - just in response to the change in Listen).

and don't need to involve accusations of the Ruby community being broken

It's not an accusation. I didn't mean that in derogatory terms. Maybe I'm just exhausted with the subject already and I'm less careful with humor.

If the discussions aren't technical and start off as accusatory (as was due to my commit) - that to me is a "broken community" that "needs patching". It's meant as humor. And as a way to redirect discussion to actual technical things instead of despairing that "something broken".

I do wish people responded to the "breakage" with purely technical questions before jumping to conclusions. So I appreciate you bringing that back down to earth.

I'm assuming I'm not needed here anymore. Mention me if I am. Or open an issue in Listen otherwise.

@e2
Copy link

e2 commented Apr 27, 2016

Just based on the current build failures: I'd suggest using gem install bundler --pre on Travis to work around this.

@e2
Copy link

e2 commented Apr 27, 2016

In .travis.yml:

before_install:
  - gem install bundler --pre
  - gem update bundler

(May fix a RVM/Travis issue with JRuby as well - about Bundler missing).

@e2
Copy link

e2 commented Apr 27, 2016

By "missing Bundler issue" I mean exactly this here: https://travis-ci.org/cryptosphere/rbnacl/jobs/125958825 (jruby-head)

@tarcieri
Copy link
Contributor

#136 fixed the build problems. @grempe can you rebase and try again?

@grempe
Copy link
Contributor Author

grempe commented Apr 29, 2016

Rebased. All green. 💯

#
# @return [String] The signature key bytes. Left half is 32-byte
# curve25519 private scalar, right half is 32-byte group element
def keypair
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keypair_bytes maybe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also maybe put this right after to_bytes in the file...

@grempe
Copy link
Contributor Author

grempe commented Apr 29, 2016

Updated PR branch to rename method to #keypair_bytes and moved its location in the file.

@tarcieri tarcieri merged commit dc1e8d1 into RubyCrypto:master Apr 29, 2016
@tarcieri
Copy link
Contributor

Looks good, thanks!

@grempe
Copy link
Contributor Author

grempe commented Apr 30, 2016

Thanks for pulling this in. 👍

@grempe grempe deleted the signing_key_bytes branch April 30, 2016 17:46
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.

None yet

3 participants