Skip to content
This repository has been archived by the owner on Jul 14, 2021. It is now read-only.

Use compiled ruby on windows #726

Merged
merged 4 commits into from
Mar 18, 2016
Merged

Use compiled ruby on windows #726

merged 4 commits into from
Mar 18, 2016

Conversation

jkeiser
Copy link
Contributor

@jkeiser jkeiser commented Mar 4, 2016

No description provided.

@@ -32,6 +32,18 @@
install_dir "#{default_root}/#{name}"
end

if windows?
override :ruby, version: "2.1.6"
Copy link

Choose a reason for hiding this comment

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

Don't need if/else branches now.

Just

override :ruby, version: "2.1.6"

The default devkit is the latest one and it correctly select the platform bit-ness for you.

@ksubrama
Copy link

ksubrama commented Mar 4, 2016

👍


dependency "rubygems"
dependency "bundler"
dependency "appbundler"
# windows does not have native readline support with compiled ruby
dependency "rb-readline" if windows?
Copy link

Choose a reason for hiding this comment

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

Minor nit: I would put this in the project definition instead of here because chefdk (the software) shouldn't technically care about the exactly it gets ruby or even if it gets readline. chefdk the project on the other hand probably cares about the platforms its packaging for.

This way, if we every get to refactoring the ruby software definition, we can move all the ruby related pre and post install steps into one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually did this due to the fact that we don't compile with readline at all--this is necessary for the software def to work at present.

Copy link
Contributor

Choose a reason for hiding this comment

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

and I think based on a recent commit in chef, we can omit if windows?

Choose a reason for hiding this comment

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

reiterating this - it'll also make #748 redundant and keep things consistent. Add it as a dependency right below "preparation" and that way, ruby gets installed and cached as one of the first things in this project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -39,6 +44,7 @@
override :'kitchen-inspec', version: "v0.12.3"

override :berkshelf, version: "v4.3.0"
override :'dep-selector-libgecode', version: "1.2.0"

Choose a reason for hiding this comment

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

Can you bump this in omnibus-software? (As follow up work if you want to...)

@mwrock
Copy link
Contributor

mwrock commented Mar 18, 2016

👍

@jkeiser jkeiser merged commit 6f56423 into master Mar 18, 2016
@ksubrama ksubrama deleted the jk/compiled-ruby branch March 18, 2016 20:12
@thommay thommay added Type: Enhancement Adds new functionality. and removed Enhancement labels Feb 1, 2017
@chef-boneyard chef-boneyard locked and limited conversation to collaborators Feb 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Enhancement Adds new functionality.
Development

Successfully merging this pull request may close these issues.

5 participants