Improve packaging Chef-DK with Habitat #2202

afiune · 2019-07-30T17:13:59Z

Improvements to the way we package Chef-DK with Habitat (for Linux systems)

Description

Restructure wrap_ruby_bin to respect DEBUG env-var
Restructure do_install to execute in a sub-shell
Restructure do_build to execute in a sub-shell
Use SRC_PATH instead of PLAN_CONTEXT/..
Remove unnecessary do_before callback
Use pkg_interpreter_for and remove core/coreutils dependency
Makes pkg_version a func to use update_pkg_version
Adds buildtike job to test habitat package to prevent regressions
Quick general styling

Related Issue

None

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

I have read the CONTRIBUTING document.
I have run the pre-merge tests locally and they pass.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
All commits have been signed-off for the Developer Certificate of Origin.

robbkidd · 2019-07-30T17:16:33Z

@afiune So that I can learn more about these patterns for plans, can you include the Why for each of these changes (in the list of changes on the PR? maybe in the git commit messages themselves?) in addition to the What of the change?

afiune · 2019-07-30T17:20:36Z

@robbkidd I tried to do it by separating the change in different commits but, I can be more explicit with in-line comments. Gimme a few! 💯

afiune · 2019-07-30T17:21:52Z

habitat/plan.sh

@@ -33,17 +34,9 @@ pkg_deps=(
  core/libarchive
 )

-pkg_svc_user=root


It's better to set variables at the top to have better visibility.

afiune · 2019-07-30T17:22:28Z

habitat/plan.sh

-do_before() {
-  do_default_before
-  update_pkg_version
-}


This whole do_before wasn't doing anything, I feel it was a copy/paste from other plans, maybe. So better to have less code to not confuse users that we are using this hook when we are not.

We should be using the do_before to load the package version rather than cat-ing it in the pkg_version declaration.

robbkidd · 2019-07-30T17:27:54Z

habitat/plan.sh

+  # versions of dependencies already resolved
+  build_line "AppBundling chef-dk gems: ${gems_to_appbundle[*]}"
+  ( cd "$CACHE_PATH" || exit_with "unable to enter hab-cache directory" 1
+    bundle exec appbundler "$HAB_CACHE_SRC_PATH/$pkg_dirname" "$ruby_bin_dir" ${gems_to_appbundle[*]} >/dev/null


w( °ｏ°)w ... appbundler will accept multiple gems on the command line?

afiune · 2019-07-30T17:27:57Z

habitat/plan.sh

 do_download() {
-  # Instead of downloading, build a gem based on the source in src/
-  cd $PLAN_CONTEXT/..


As a best practice, we /shouldn't/ change the directory inside the callbacks since that position will be persistent throughout the build process of the package and latter other callbacks or habitat internals might conflict with that change of state, instead we should 1) execute commands that depend on a specific path inside a sub-sells or 2) use pushd and popd to move to a directory and get back where we were before.

Number 1) is more desired but you could use them both.

Here we actually don't even need to change the directory, so we should try to avoid it.

afiune · 2019-07-30T17:29:03Z

habitat/plan.sh

@@ -53,7 +46,8 @@ do_verify() {
 do_unpack() {
  # Unpack the gem we built to the source cache path. Building then unpacking
  # the gem reuses the file inclusion/exclusion rules defined in the gemspec.
-  gem unpack $PLAN_CONTEXT/../$pkg_name-$pkg_version.gem --target=$HAB_CACHE_SRC_PATH


We were using $PLAN_CONTEXT/.. to go to /src, habitat has an environment to go there directly, let us use it! SRC_PATH 💯

afiune · 2019-07-30T17:31:05Z

habitat/plan.sh

 }

 do_build() {
-  cd $CACHE_PATH


Same, avoid changing directories like this. Use sub-shells instead. See line 87

afiune · 2019-07-30T17:31:43Z

habitat/plan.sh

@@ -64,40 +58,46 @@ do_prepare() {
  build_line "Ruby ABI version appears to be ${RUBY_ABI_VERSION}"

  build_line "Setting link for /usr/bin/env to 'coreutils'"
-  [[ ! -f /usr/bin/env ]] && ln -s $(pkg_path_for coreutils)/bin/env /usr/bin/env
-
-  return 0


if we always return 0 at the end of this function, we would be saying that this function always succeeds.

afiune · 2019-07-30T17:32:26Z

habitat/plan.sh

+  local _zlib_dir
+  export NOKOGIRI_CONFIG
+  export GEM_HOME
+  export GEM_PATH


It is safer to first initialize the variables and then set the values, especially if you don't know what the content of the variable could be.

That's a pattern that has come from when the value is from a command interpolation, right? Because the exit code gets eaten and testing success would be on local? So, just get in the habit of initializing locals first, then assigning values?

afiune · 2019-07-30T17:33:06Z

habitat/plan.sh

+    bundle config --local silence_root_warning 1
+    bundle install --without dep_selector --no-deployment --jobs 2 --retry 5 --path "$pkg_prefix"
+    gem build ${pkg_name}.gemspec
+  )


All these bundle commands depend to run inside the cache path, here is the way you create sub-shells to run those commands.

afiune · 2019-07-30T17:33:23Z

habitat/plan.sh

 }

 do_install() {
-  cd $CACHE_PATH


Same, try to avoid changing paths 😅

afiune · 2019-07-30T17:35:06Z

habitat/plan.sh

+  # versions of dependencies already resolved
+  build_line "AppBundling chef-dk gems: ${gems_to_appbundle[*]}"
+  ( cd "$CACHE_PATH" || exit_with "unable to enter hab-cache directory" 1
+    bundle exec appbundler "$HAB_CACHE_SRC_PATH/$pkg_dirname" "$ruby_bin_dir" ${gems_to_appbundle[*]} >/dev/null


when both paths are the same, appbundler allows you to pass a list of gems to bundle, let us use that instead of doing a for here since we need to execute it inside a sub-shell.

afiune · 2019-07-30T17:35:58Z

habitat/plan.sh

-  for exe in $pkg_prefix/ruby-bin/*; do
-    wrap_ruby_bin $(basename ${exe})
+  build_line "Installing generated gem. (${CACHE_PATH}/${pkg_name}-${pkg_version}.gem)"
+  gem install --no-doc "${CACHE_PATH}/${pkg_name}-${pkg_version}.gem"


this gem install command was inside the do_build callback, were as now it is inside the do_install since we are installing it 😉

afiune · 2019-07-30T17:36:44Z

habitat/plan.sh

+
+  build_line "Link the appbundled binstubs into the package's bin directory"
+  for exe in "$ruby_bin_dir"/*; do
+    wrap_ruby_bin "$(basename "${exe}")"


The wrap_ruby_bin helper method doesn't need to run on a specific path so, we avoid using unnecessary sub-shells.

robbkidd · 2019-07-30T17:41:30Z

habitat/plan.sh


-  # Appbundling gems speeds up runtime by creating binstubs for Ruby executables with
-  # versions of dependencies already resolved
+  # TODO(afiune) Should we define this inside the repo and not here inside the plan?


Agree that this list should be maintained in one place for all the buildy things to reference. Question is: where to store the list and in what format?

afiune · 2019-07-30T17:42:40Z

habitat/plan.sh

  cat <<EOF > "$wrapper"
 #!$(pkg_path_for busybox-static)/bin/sh
 set -e
-if test -n "$DEBUG"; then set -x; fi
+if test -n "\$DEBUG"; then set -x; fi


Before we were actually printing the content of the variable, now we are writing the variable name to check it at execution of these binaries, you can now do this:

[86][default:/src:1]# DEBUG=true chef + export 'GEM_HOME=/hab/pkgs/afiune/chef-dk/4.3.3/20190730094053/ruby/2.6.0/' + export 'GEM_PATH=/hab/pkgs/core/ruby26/2.6.3/20190515141800/lib/ruby/gems/2.6.0:/hab/pkgs/core/bundler/1.17.3/20190416230243:/hab/pkgs/afiune/chef-dk/4.3.3/20190730094053/ruby/2.6.0/:/hab/pkgs/afiune/chef-dk/4.3.3/20190730094053' + export 'SSL_CERT_FILE=/hab/pkgs/core/cacerts/2018.12.05/20190115014206/ssl/cert.pem' + export 'APPBUNDLER_ALLOW_RVM=true' + unset RUBYOPT GEMRC + exec /hab/pkgs/core/ruby26/2.6.3/20190515141800/bin/ruby /hab/pkgs/afiune/chef-dk/4.3.3/20190730094053/ruby-bin/chef Usage: chef -h/--help chef -v/--version chef command [arguments...] [options...] Available Commands: exec Runs the command in context of the embedded ruby env Prints environment variables used by ChefDK gem Runs the `gem` command in context of the embedded Ruby generate Generate a new repository, cookbook, or other component shell-init Initialize your shell to use ChefDK as your primary Ruby install Install cookbooks from a Policyfile and generate a locked cookbook set update Updates a Policyfile.lock.json with latest run_list and cookbooks push Push a local policy lock to a policy group on the Chef Infra Server push-archive Push a policy archive to a policy group on the Chef Infra Server show-policy Show policyfile objects on the Chef Infra Server diff Generate an itemized diff of two Policyfile lock documents export Export a policy lock as a Chef Infra Zero code repo clean-policy-revisions Delete unused policy revisions on the Chef Infra Server clean-policy-cookbooks Delete unused policyfile cookbooks on the Chef Infra Server delete-policy-group Delete a policy group on the Chef Infra Server delete-policy Delete all revisions of a policy on the Chef Infra Server undelete Undo a delete command describe-cookbook Prints cookbook checksum information used for cookbook identifier

afiune · 2019-07-30T17:44:01Z

habitat/plan.sh

 export GEM_HOME="$pkg_prefix/ruby/${RUBY_ABI_VERSION}/"
 export GEM_PATH="$(pkg_path_for ${ruby_pkg})/lib/ruby/gems/${RUBY_ABI_VERSION}:$(hab pkg path core/bundler):$pkg_prefix/ruby/${RUBY_ABI_VERSION}/:$GEM_HOME"
 export SSL_CERT_FILE=$(pkg_path_for core/cacerts)/ssl/cert.pem
 export APPBUNDLER_ALLOW_RVM=true
 unset RUBYOPT GEMRC
-exec $(pkg_path_for ${ruby_pkg})/bin/ruby ${real_cmd} \$@
+exec $(pkg_path_for ${ruby_pkg})/bin/ruby ${real_cmd} "\$@"


As a best practice, we should double-quote array expansions to avoid re-splitting elements.

tduffield · 2019-08-01T14:31:02Z

habitat/plan.sh

-do_before() {
-  do_default_before
-  update_pkg_version
-}


We should be using the do_before to load the package version rather than cat-ing it in the pkg_version declaration.

afiune · 2019-08-01T14:35:05Z

@tduffield I can put back update_pkg_version but then I will make the pkg_version a function, would that be ok?

out-of-curiosity: what is the motivation to use this pattern Vs cat-ing?

tduffield · 2019-08-01T14:40:04Z

@afiune it's what the Habitat docs recommend: https://www.habitat.sh/docs/reference/#plan-helper-functions. I don't recall the exact reasoning, but I think it has to do with whether or not the path is correct based on where we source the plan file.

afiune · 2019-08-01T14:53:59Z

@tduffield makes sense, I have put it back and converted the pkg_version into a function. Thanks for your feedback!

The whole do_before wasn't doing anything, I feel it was a copy/paste from other plans, maybe. So better to have less code to not confuse users that we are using this hook when we are not. Signed-off-by: Salim Afiune <afiune@chef.io>

We were using `$PLAN_CONTEXT/..` to go to `/src` but habitat has an environment to go there directly, let us use it! `SRC_PATH` Signed-off-by: Salim Afiune <afiune@chef.io>

It's better to set variables at the top to have better visibility. If we always return 0 at the end of this function, we would be saying that this function always succeeds. Signed-off-by: Salim Afiune <afiune@chef.io>

As a best practice, we /shouldn't/ change the directory inside the callbacks since that position will be persistent throughout the build process of the package and latter other callbacks or habitat internals might conflict with that change of state, instead we should 1) execute commands that depend on a specific path inside a sub-sells or 2) use pushd and popd to move to a directory and get back where we were before. Number 1) is more desired but you could use them both. It is safer to first initialize the variables and then set the values, especially if you don't know what the content of the variable could be. All the bundle commands depend on running inside the cache path, I changed it to use sub-shells to run those commands. Signed-off-by: Salim Afiune <afiune@chef.io>

As a best practice, we /shouldn't/ change the directory inside the callbacks since that position will be persistent throughout the build process of the package and latter other callbacks or habitat internals might conflict with that change of state, instead we should 1) execute commands that depend on a specific path inside a sub-sells or 2) use pushd and popd to move to a directory and get back where we were before. Number 1) is more desired but you could use them both. TODO we should define the list of binaries inside the repo and not inside the plan Signed-off-by: Salim Afiune <afiune@chef.io>

As a best practice, we should double-quote array expansions like `"\$@"` to avoid re-splitting elements. Before we were actually printing the content of the variable, now we are writing the variable name to check it at execution of these binaries, you can now do this: ``` [86][default:/src:1]# DEBUG=true chef + export 'GEM_HOME=/hab/pkgs/afiune/chef-dk/4.3.3/20190730094053/ruby/2.6.0/' + export 'GEM_PATH=/hab/pkgs/core/ruby26/2.6.3/20190515141800/lib/ruby/gems/2.6.0:/hab/pkgs/core/bundler/1.17.3/20190416230243:/hab/pkgs/afiune/chef-dk/4.3.3/20190730094053/ruby/2.6.0/:/hab/pkgs/afiune/chef-dk/4.3.3/20190730094053' + export 'SSL_CERT_FILE=/hab/pkgs/core/cacerts/2018.12.05/20190115014206/ssl/cert.pem' + export 'APPBUNDLER_ALLOW_RVM=true' + unset RUBYOPT GEMRC + exec /hab/pkgs/core/ruby26/2.6.3/20190515141800/bin/ruby /hab/pkgs/afiune/chef-dk/4.3.3/20190730094053/ruby-bin/chef Usage: chef -h/--help chef -v/--version chef command [arguments...] [options...] Available Commands: exec Runs the command in context of the embedded ruby env Prints environment variables used by ChefDK gem Runs the `gem` command in context of the embedded Ruby generate Generate a new repository, cookbook, or other component shell-init Initialize your shell to use ChefDK as your primary Ruby install Install cookbooks from a Policyfile and generate a locked cookbook set update Updates a Policyfile.lock.json with latest run_list and cookbooks push Push a local policy lock to a policy group on the Chef Infra Server push-archive Push a policy archive to a policy group on the Chef Infra Server show-policy Show policyfile objects on the Chef Infra Server diff Generate an itemized diff of two Policyfile lock documents export Export a policy lock as a Chef Infra Zero code repo clean-policy-revisions Delete unused policy revisions on the Chef Infra Server clean-policy-cookbooks Delete unused policyfile cookbooks on the Chef Infra Server delete-policy-group Delete a policy group on the Chef Infra Server delete-policy Delete all revisions of a policy on the Chef Infra Server undelete Undo a delete command describe-cookbook Prints cookbook checksum information used for cookbook identifier ``` Signed-off-by: Salim Afiune <afiune@chef.io>

Why? Because if we have code inside the repo that is not being tested we will definitely loose this work while doing other modifications, the best thing to do is always add tests to constantly check if any functionality is lost. TODO Improve this tests a lot since right now they are just printing the version of the commands that work, but at least we will know if break one for the habitat packages. Signed-off-by: Salim Afiune <afiune@chef.io>

As a best practice we should use the `update_pkg_version` as habitat documents it here: https://www.habitat.sh/docs/reference/#plan-helper-functions Signed-off-by: Salim Afiune <afiune@chef.io>

We should use proper habitat helper methods to get this information like a package interpreter: https://www.habitat.sh/docs/reference/#plan-helper-functions Signed-off-by: Salim Afiune <afiune@chef.io>

afiune · 2019-08-01T18:37:16Z

Apologies for the force-pushed but there was a request to have all the in-line comments inside every commit message so I had to rebase them all to do so. Now, this PR should be ready to review and merge!

robbkidd · 2019-08-02T20:30:39Z

habitat/plan.sh

-  cd $PLAN_CONTEXT/..
-  gem build $pkg_name.gemspec
+  build_line "Building gem from source. (${SRC_PATH}/${pkg_name}.gemspec)"
+  gem build "${SRC_PATH}/${pkg_name}.gemspec"


To confirm, ${SRC_PATH} is the directory the studio was entered from, right? Which should be fine for standard builds of this code base. Noting that this could surprise someone in their development environment if they enter a studio from higher up in a directory structure if they are trying to work with multiple plans in a single studio.

you can't enter from higher up, you have to enter from the source root path so that the source is available inside the studio (docker container)

.studiorc

robbkidd · 2019-08-02T20:40:44Z

habitat/tests.sh

+echo -n " - chef-shell "
+chef-shell -v | awk {'print $NF'}
+# TODO(afiune) do we still use this? because it doesn't work
+echo " - chef-zero (not-working)"


I think we do still need this. Maybe chef-zero got munched in the shuffling of what gem provides executables post-license change?

It is there, it just doesn't work. I'll fix it on further PRs

robbkidd

I asked more questions, but I don't think they block merging.

Thank you for your commit message commentary!

lock · 2019-10-04T10:20:55Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

afiune requested review from robbkidd, tas50, marcparadise and tyler-ball July 30, 2019 17:13

afiune requested review from a team as code owners July 30, 2019 17:13

afiune self-assigned this Jul 30, 2019

afiune added the Aspect: Packaging Distribution of the projects 'compiled' artifacts. label Jul 30, 2019

afiune commented Jul 30, 2019

View reviewed changes

robbkidd reviewed Jul 30, 2019

View reviewed changes

afiune commented Jul 30, 2019

View reviewed changes

habitat/plan.sh

}

do_install() {

cd $CACHE_PATH

Copy link

Contributor Author

afiune Jul 30, 2019 •

edited

Loading

Choose a reason for hiding this comment

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

Same, try to avoid changing paths 😅

afiune commented Jul 30, 2019

View reviewed changes

robbkidd reviewed Jul 30, 2019

View reviewed changes

afiune commented Jul 30, 2019

View reviewed changes

afiune requested a review from a team as a code owner August 1, 2019 12:34

afiune force-pushed the afiune/hab branch 3 times, most recently from f38f1da to 1ee692e Compare August 1, 2019 13:56

tduffield suggested changes Aug 1, 2019

View reviewed changes

afiune force-pushed the afiune/hab branch from 1ee692e to 5cc2899 Compare August 1, 2019 14:36

tduffield approved these changes Aug 1, 2019

View reviewed changes

Salim Afiune added 9 commits August 1, 2019 20:16

Remove unnecessary do_before callback

9e053ff

The whole do_before wasn't doing anything, I feel it was a copy/paste from other plans, maybe. So better to have less code to not confuse users that we are using this hook when we are not. Signed-off-by: Salim Afiune <afiune@chef.io>

Use SRC_PATH instead of PLAN_CONTEXT/..

0c1e12f

We were using `$PLAN_CONTEXT/..` to go to `/src` but habitat has an environment to go there directly, let us use it! `SRC_PATH` Signed-off-by: Salim Afiune <afiune@chef.io>

quick styling

c0ffad6

It's better to set variables at the top to have better visibility. If we always return 0 at the end of this function, we would be saying that this function always succeeds. Signed-off-by: Salim Afiune <afiune@chef.io>

Makes pkg_version a func to use update_pkg_version

f53c640

As a best practice we should use the `update_pkg_version` as habitat documents it here: https://www.habitat.sh/docs/reference/#plan-helper-functions Signed-off-by: Salim Afiune <afiune@chef.io>

Use pkg_interpreter_for and remove core/coreutils

f427dc9

We should use proper habitat helper methods to get this information like a package interpreter: https://www.habitat.sh/docs/reference/#plan-helper-functions Signed-off-by: Salim Afiune <afiune@chef.io>

afiune force-pushed the afiune/hab branch from dfdcc58 to f427dc9 Compare August 1, 2019 18:31

afiune added the Type: Chore non-critical maintenance of a project. label Aug 2, 2019

afiune removed request for tas50, marcparadise and tyler-ball August 2, 2019 17:20

robbkidd reviewed Aug 2, 2019

View reviewed changes

.studiorc Show resolved Hide resolved

robbkidd reviewed Aug 2, 2019

View reviewed changes

robbkidd approved these changes Aug 2, 2019

View reviewed changes

afiune merged commit cc332fe into master Aug 5, 2019

chef-ci deleted the afiune/hab branch August 5, 2019 09:24

lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve packaging Chef-DK with Habitat #2202

Improve packaging Chef-DK with Habitat #2202

afiune commented Jul 30, 2019 •

edited

Loading

robbkidd commented Jul 30, 2019

afiune commented Jul 30, 2019

afiune Jul 30, 2019

afiune Jul 30, 2019 •

edited

Loading

tduffield Aug 1, 2019

robbkidd Jul 30, 2019

afiune Jul 30, 2019

afiune Jul 30, 2019

afiune Jul 30, 2019

afiune Jul 30, 2019

afiune Jul 30, 2019

robbkidd Jul 30, 2019

afiune Jul 30, 2019

afiune Jul 30, 2019 •

edited

Loading

afiune Jul 30, 2019

afiune Jul 30, 2019

afiune Jul 30, 2019

robbkidd Jul 30, 2019

afiune Jul 30, 2019 •

edited

Loading

afiune Jul 30, 2019

tduffield Aug 1, 2019

afiune commented Aug 1, 2019

tduffield commented Aug 1, 2019 •

edited

Loading

afiune commented Aug 1, 2019

afiune commented Aug 1, 2019

robbkidd Aug 2, 2019

afiune Aug 2, 2019

robbkidd Aug 2, 2019

afiune Aug 2, 2019

robbkidd left a comment

lock bot commented Oct 4, 2019

Improve packaging Chef-DK with Habitat #2202

Improve packaging Chef-DK with Habitat #2202

Conversation

afiune commented Jul 30, 2019 • edited Loading

Description

Related Issue

Types of changes

Checklist:

robbkidd commented Jul 30, 2019

afiune commented Jul 30, 2019

Choose a reason for hiding this comment

afiune Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afiune Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afiune Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afiune commented Aug 1, 2019

tduffield commented Aug 1, 2019 • edited Loading

afiune commented Aug 1, 2019

afiune commented Aug 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbkidd left a comment

Choose a reason for hiding this comment

lock bot commented Oct 4, 2019

afiune commented Jul 30, 2019 •

edited

Loading

afiune Jul 30, 2019 •

edited

Loading

afiune Jul 30, 2019 •

edited

Loading

afiune Jul 30, 2019 •

edited

Loading

tduffield commented Aug 1, 2019 •

edited

Loading