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

Updated omnibus postinst script to symlink to appbundle created binstubs #2732

Merged
merged 2 commits into from
Feb 28, 2018

Conversation

miah
Copy link
Contributor

@miah miah commented Feb 23, 2018

The binstubs created by rubygems, and linked to by our omnibus postinst script error when they encounter GEM_HOME or GEM_PATH set to anything outside of the /opt/inspec/embedded directory.

On my laptop with chruby the environment variables GEM_HOME and GEM_PATH are set when I create a new shell. When I invoke InSpec via /usr/local/bin/inspec or /opt/inspec/embedded/bin/inspec I get the following error:

$ /usr/local/bin/inspec version
/opt/inspec/embedded/lib/ruby/2.4.0/rubygems.rb:271:in `find_spec_for_exe': can't find gem inspec (>= 0.a) (Gem::GemNotFoundException)
	from /opt/inspec/embedded/lib/ruby/2.4.0/rubygems.rb:299:in `activate_bin_path'
	from /usr/local/bin/inspec:23:in `<main>'

This error persists until I unset GEM_HOME and unset GEM_PATH.

$ unset GEM_PATH GEM_HOME
$ /usr/local/bin/inspec version
2.0.16

The inspec binstub created by appbundler correctly deals with GEM_HOME and GEM_PATH.

$ /opt/inspec/bin/inspec version
2.0.16

Because our omnibus package already uses appbundler, the fix is to change our symlink to use it rather than the rubygems binstub.

This problem exists in older 1.x packaged releases as well.

Fixes #2727

Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

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

This looks like a fine change, and matches what we do in other projects (see https://github.com/chef/chef/blob/master/omnibus/package-scripts/chef/postinst#L14 for example).

For consistency with other projects, I'd like to see the var name stay the same. Also, before we approve and proceed, have we done an ad-hoc Jenkins build from this branch and tested the resulting package on macOS and one other OS (such as Ubuntu) just to make sure this succeeds?

Another thing to think about (outside of this PR) is a change to the Jenkins "test" job to do a GEM_HOME=/nonexistent/path GEM_PATH=/nonexistent/path /usr/local/bin/inspec version or whatever the symlink path is to ensure the package works properly.

@@ -5,7 +5,7 @@
#

PROGNAME=`basename $0`
INSTALLER_DIR=/opt/inspec/embedded
Copy link
Contributor

Choose a reason for hiding this comment

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

To keep this consistent with other omnibus projects, I would leave this as INSTALLER_DIR and just change the value to /opt/inspec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @adamleff =)

rather than rubygems binstubs in embedded directory.

The rubygems binstubs do not account for GEM_HOME or GEM_PATHS existing
in the invoking shell. This creates problems on systems with multiple
ruby versions. If GEM_HOME or GEM_PATH is set the rubygems embedded
binstubs will attempt to use this external gem source which will
generally result in errors for the user.

Signed-off-by: Miah Johnson <[email protected]>
@miah
Copy link
Contributor Author

miah commented Feb 23, 2018

Excellent comment about Jenkins test as well, I'll add one!

Jenkins to verify the appbundler shim is working correctly.

Signed-off-by: Miah Johnson <[email protected]>
@miah miah requested a review from a team as a code owner February 27, 2018 23:16
@miah
Copy link
Contributor Author

miah commented Feb 27, 2018

I added two scripts to this PR which will be called by Jenkins during the verify stage.

Copy link
Contributor

@jerryaldrichiii jerryaldrichiii left a comment

Choose a reason for hiding this comment

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

Looks good to me. Great work @miah!

@miah
Copy link
Contributor Author

miah commented Feb 28, 2018

@adamleff What do you think friend? =)

Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

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

I dig it!

@miah miah dismissed adamleff’s stale review February 28, 2018 18:44

Because he digs it! 🕳

Copy link
Contributor

@jquick jquick left a comment

Choose a reason for hiding this comment

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

Thanks @miah !

@jquick jquick merged commit 112f12d into master Feb 28, 2018
@jquick jquick deleted the mj/2727 branch February 28, 2018 18:47
miah added a commit that referenced this pull request May 10, 2018
@miah miah mentioned this pull request May 10, 2018
jquick pushed a commit that referenced this pull request May 10, 2018
…ify (#3038)

that inspec is functional.

#2732

Signed-off-by: Miah Johnson <[email protected]>
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.

InSpec 2.0.17 for macOS 10.13 doesn't work (Gem::GemNotFoundException)
5 participants