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

Knife: Add gems for ECC algorithm support to omnibus. #5736

Merged
merged 5 commits into from
Jan 19, 2017

Conversation

rhass
Copy link
Contributor

@rhass rhass commented Jan 19, 2017

Description

These definitions have been added to resolve build issues with ECC algorithm support on Solaris and Cisco IOS-XR.

The bcyrpt_pbkdf gem currently link against shared libs which are not installed on Cisco IOS-XR.

The rbnacl gems currently fail to compile on Solaris for both x86-64 and
SPARC.

Adding these definitions will allow us to selectively define the supported platforms as well as provide a simple means to iterate on patches as needed.

Note: This was pinned to a dev branch for testing:
https://github.com/chef/chef/compare/rhass/COOL-657_ecc-algorithm-support?expand=1#diff-ba79ba7a139ca66864e6fb2f9cbbc904R11

I am targeting master, so this pinning will need to be reverted and the patch cherry-picked until we kill those branches in a couple weeks.

Issues Resolved

  • COOL-657 (Internal Tracking ID)

Check List

These definitions have been added to resolve build issues with ECC
algorithm support on Solaris and Cisco IOS-XR.

The bcyrpt_pbkdf gem currently link against shared libs which are not
installed on Cisco IOS-XR.

The rbnacl gems currently fail to compile on Solaris for both x86-64 and
SPARC.

Adding these definitions will allow us to selectively define the
supported platforms as well as provide a simple means to itterate on
patches as needed.

Signed-off-by: Ryan Hass <rhass@users.noreply.github.com>
@@ -26,9 +26,7 @@ Gem::Specification.new do |s|

s.add_dependency "ffi-yajl", "~> 2.2"
s.add_dependency "net-ssh", ">= 2.9", "< 5.0"
s.add_dependency "rbnacl-libsodium", "~> 1.0.0"
s.add_dependency "rbnacl", "~> 4.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

you probably want to remove this as well.

@@ -44,6 +44,9 @@
dependency "chef-gem-byebug"
dependency "chef-gem-debug_inspector"
dependency "chef-gem-binding_of_caller"
dependency "chef-gem-rbnacl-libsodium" unless ios_xr?
dependency "chef-gem-bcrypt_pbkdf-ruby" unless solaris?
Copy link
Contributor

Choose a reason for hiding this comment

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

and likely you just want to make this an unless ios_xr? || solaris? around both of them (or all three)

@lamont-granquist
Copy link
Contributor

couple comments, although i don't feel strongly about them...

Copy link
Contributor

@thommay thommay left a comment

Choose a reason for hiding this comment

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

ChefStyle is angry, and same comments as Lamont. Otherwise, 👍

Signed-off-by: Tom Duffield <tom@chef.io>
Based on feedback.

Signed-off-by: Tom Duffield <tom@chef.io>
@tduffield
Copy link
Contributor

Test build (chef 748) was successful

@@ -44,6 +44,10 @@
dependency "chef-gem-byebug"
dependency "chef-gem-debug_inspector"
dependency "chef-gem-binding_of_caller"
unless ios_xr? || solaris?
Copy link

Choose a reason for hiding this comment

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

Do we need both deps for any Cisco or Solaris OS? 🤔 I ask this because in the desc @rhass mentioned that the bcyrpt_pbkdf gem currently link against shared libs which are not installed on Cisco IOS-XR and that the rbnacl gems currently fail to compile on Solaris for both x86-64 and SPARC.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is how lamont and thom recommended we do it, and the builds succeeded, so unless there's a specific reason we should I'm inclined to keep it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this feature on either Cisco or Solaris, its mostly for workstations, and Cisco's are definitely not workstations, and Solaris workstations should be reasonably miniscule as a use case. The bigger problem here is Linux, Windows, Mac, and maybe FreeBSD. It introduces a slight discrepancy in our omnibus philosophy, but we've had that before with ruby-shadow.

Copy link

Choose a reason for hiding this comment

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

I trust y'all 👍 - Just wanted to point it out. 😄

@rhass rhass force-pushed the rhass/COOL-657_ecc-algorithm-support branch from 7536422 to 0287dea Compare January 19, 2017 20:00
We need to pin rainbow until the next version of rubygems is released as
it is causing build failures in travis.

Signed-off-by: Ryan Hass <rhass@users.noreply.github.com>
@rhass rhass force-pushed the rhass/COOL-657_ecc-algorithm-support branch from 0287dea to 9705a2b Compare January 19, 2017 20:19
Signed-off-by: Tom Duffield <tom@chef.io>
@rhass rhass merged commit 41a8dc2 into master Jan 19, 2017
@rhass rhass deleted the rhass/COOL-657_ecc-algorithm-support branch January 19, 2017 21:33
@thommay thommay added Type: Bug Does not work as expected. and removed Bug labels Jan 25, 2017
@lamont-granquist lamont-granquist changed the title Add gems for ECC algorithm support to omnibus. Knife: Add gems for ECC algorithm support to omnibus. Feb 17, 2017
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Bug Does not work as expected.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants