[bldr-build] Prefix public callback functions with `do_`. #120

fnichol · 2015-12-18T19:34:53Z

Ths addresses an issue where the author of a Plan wants to call the
install(1) command to copy files, create directories, etc. Here's a
short example that happens in an install() callback:

install() {
  make install
  install -D -m644 LICENSE ${pkg_path}/share/LICENSE
}

The call to install on line 3 won't execute the install(1) command,
but will instead recursively call the defined install() shell
function, which recurses into foreverland.

Additionally, now all user-facing callbacks that are designed to be
overwritten are of the naming form do_*(). The following functions are
now part of the public API for Plan files:

do_begin() (formerly bldr_begin())
do_download() (formerly download())
do_verify() (formerly verify())
do_clean() (formerly clean_cache())
do_unpack() (formerly unpack())
do_prepare_wrapper() (formerly prepare_wrapper())
do_prepare() (formerly prepare())
do_build_wrapper() (formerly build_wrapper())
do_build() (formerly build())
do_install_wrapper() (formerly install_wrapper())
do_install() (formerly install())
do_build_config() (formerly config())
do_service() (formerly service())
do_strip() (formerly strip_binaries())
do_docker_image_wrapper() (formerly dockerfile_wrapper())
do_dockerfile_inline() (formerly docker_inline())
do_docker_image() (formerly dockerfile())
do_end() (formerly bldr_end())

chef-delivery · 2015-12-18T19:36:54Z

This PR has passed 'Verify' and is ready for review and approval!
Use: '@delivery approve' when code review is complete.

fnichol · 2015-12-18T19:36:56Z

reset · 2015-12-18T19:36:59Z

plans/Makefile

-	done
+	$(MAKE) $(addprefix publish-,$(PKGS))
+
+$(PKGS):


I didn't know you could do this, that's awesome

I'm re-remembering a lot of Makefile-isms after reading them to build our softwares 😄

reset · 2015-12-18T19:39:45Z

@fnichol works for me. Do you think we perhaps should prefix all of our callbacks with bldr_ to avoid collisions like this? So install() would become bldr_install()?

fnichol · 2015-12-18T19:46:30Z

@reset It's something I was wondering about, but I'd rather change them all like that to the following:

bldr_begin() -> bldr_begin() (no change)
download() -> bldr_download()
verify() -> bldr_verify()
clean_cache() -> bldr_clean_cache() or bldr_clean()
unpack() -> bldr_unpackl()
prepare() -> bldr_prepare()
build() -> bldr_build()
install;() -> bldr_install()
config() -> bldr_config()
service() -> bldr_service()
strip_binaries() -> bldr_strip_binaries() or bldr_strip()
dockerfile() -> bldr_dockerfile()
bldr_end() -> bldr_end() (no change)

There are few of functions that happen in the build flow (i.e. resolve_deps(), build_environment(), link_libraries(), manifest(), and package) that feel more internal to how bldr-build works that maybe shouldn't be advertised as "public" callback overrides.

Thoughts?

reset · 2015-12-18T19:47:54Z

@fnichol yeah I'm thinking that we should prefix all of our callbacks/public functions with bldr_ and then I would prefix anything that is private with an underscore or two. _link_libraries(), etc.

fnichol · 2015-12-18T19:50:14Z

@reset I wanted to table that idea earlier this week then forgot. You can see that I've started to hint at functions that you probably shouldn't call as a Plan author, but I'd love to make this even more explicit. Otherwise it limits our ability to refactor and extend into the future.

Ths addresses an issue where the author of a Plan wants to call the `install(1)` command to copy files, create directories, etc. Here's a short example that happens in an `install()` callback: ```sh install() { make install install -D -m644 LICENSE ${pkg_path}/share/LICENSE } ``` The call to `install` on line 3 won't execute the `install(1)` command, but will instead recursively call the defined `install()` shell function, which recurses into foreverland. Additionally, now all user-facing callbacks that are designed to be overwritten are of the naming form `do_*()`. The following functions are now part of the public API for Plan files: * `do_begin()` (formerly `bldr_begin()`) * `do_download()` (formerly `download()`) * `do_verify()` (formerly `verify()`) * `do_clean()` (formerly `clean_cache()`) * `do_unpack()` (formerly `unpack()`) * `do_prepare_wrapper()` (formerly `prepare_wrapper()`) * `do_prepare()` (formerly `prepare()`) * `do_build_wrapper()` (formerly `build_wrapper()`) * `do_build()` (formerly `build()`) * `do_install_wrapper()` (formerly `install_wrapper()`) * `do_install()` (formerly `install()`) * `do_build_config()` (formerly `config()`) * `do_service()` (formerly `service()`) * `do_strip()` (formerly `strip_binaries()`) * `do_docker_image_wrapper()` (formerly `dockerfile_wrapper()`) * `do_dockerfile_inline()` (formerly `docker_inline()`) * `do_docker_image()` (formerly `dockerfile()`) * `do_end()` (formerly `bldr_end()`)

This change changes the names of some internally-facing functions in order to signal that external users (i.e. Plan authors) should not call these functions or rely on their current behavior over the long term. A function which is prefix with a single underscore (`_`) is API/private visibility.

The `resolve_dep()` function was using `exit_with()`, but since it was being called from a subshell and the return code was not being checked, the program would continue after a bad depdency solve.

It turns out that using a shell-style for loop in a Makefile does not give you the same guarentees as running that same for loop in a shell script with `set -e` set. The for loop is considered a single line statement to `make(1)` so any failures inside the loop will not trigger an abort. This change uses Makefile targets to accomplish the same end goal.

fnichol · 2015-12-19T00:59:43Z

Based on feedback from @reset and @adamhjk, I went with a do_*() format for public, user-facing callbacks.

chef-delivery · 2015-12-19T01:01:36Z

This PR has passed 'Verify' and is ready for review and approval!
Use: '@delivery approve' when code review is complete.

fnichol · 2015-12-19T01:01:38Z

Updated the PR body as well with some context and ready for a review!

adamhjk · 2015-12-19T01:03:56Z

adamhjk · 2015-12-19T01:04:05Z

@delivery approve

Merged change 19030a67-f5c3-4371-806a-c301b1b4765b From review branch rename-install-callback into master Signed-off-by: adam <[email protected]>

chef-delivery · 2015-12-19T01:04:08Z

Change: 19030a67-f5c3-4371-806a-c301b1b4765b approved by: @adamhjk

reset · 2015-12-19T03:19:33Z

@fnichol @adamhjk perfect!

This was a missed oversight with changes made in #120.

This was missed when updating variables in #120.

[AtomicWriter] fsync() parent directory after rename

chef-delivery added the Delivery Change Created label Dec 18, 2015

reset reviewed Dec 18, 2015
View reviewed changes

fnichol added 5 commits December 18, 2015 17:58

[bldr-build] Ensure functions that run in subshells use return.

6ce1a8f

The `resolve_dep()` function was using `exit_with()`, but since it was being called from a subshell and the return code was not being checked, the program would continue after a bad depdency solve.

[bldr-build] Fix issue where LD_RUN_PATH contains a leading colon.

e237186

fnichol force-pushed the rename-install-callback branch from 31570c5 to b5a4978 Compare December 19, 2015 00:59

fnichol changed the title ~~[bldr-build] Rename install() callback to install_files().~~ [bldr-build] Prefix public callback functions with do_. Dec 19, 2015

chef-delivery added a commit that referenced this pull request Dec 19, 2015

Merge pull request #120 from chef/rename-install-callback

f96a713

Merged change 19030a67-f5c3-4371-806a-c301b1b4765b From review branch rename-install-callback into master Signed-off-by: adam <[email protected]>

chef-delivery merged commit f96a713 into master Dec 19, 2015

chef-delivery deleted the rename-install-callback branch December 19, 2015 01:04

fnichol added a commit that referenced this pull request Dec 21, 2015

Update plan.sh callback function names in functional tests.

8caf7a2

This was a missed oversight with changes made in #120.

fnichol added a commit that referenced this pull request Dec 21, 2015

[hana] Update $BLDR_CONTEXT to $PLAN_CONTEXT.

37448fd

This was missed when updating variables in #120.

fnichol mentioned this pull request Dec 21, 2015

Function callback updates #121

Merged

raskchanky pushed a commit that referenced this pull request Apr 16, 2019

Merge pull request #120 from stevendanna/ssd/fsync-dir

a549a96

[AtomicWriter] fsync() parent directory after rename

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bldr-build] Prefix public callback functions with `do_`. #120

[bldr-build] Prefix public callback functions with `do_`. #120

fnichol commented Dec 18, 2015

chef-delivery commented Dec 18, 2015

fnichol commented Dec 18, 2015

reset Dec 18, 2015

fnichol Dec 18, 2015

reset commented Dec 18, 2015

fnichol commented Dec 18, 2015

reset commented Dec 18, 2015

fnichol commented Dec 18, 2015

fnichol commented Dec 19, 2015

chef-delivery commented Dec 19, 2015

fnichol commented Dec 19, 2015

adamhjk commented Dec 19, 2015

adamhjk commented Dec 19, 2015

chef-delivery commented Dec 19, 2015

reset commented Dec 19, 2015

[bldr-build] Prefix public callback functions with do_. #120

[bldr-build] Prefix public callback functions with do_. #120

Conversation

fnichol commented Dec 18, 2015

chef-delivery commented Dec 18, 2015

fnichol commented Dec 18, 2015

reset Dec 18, 2015

Choose a reason for hiding this comment

fnichol Dec 18, 2015

Choose a reason for hiding this comment

reset commented Dec 18, 2015

fnichol commented Dec 18, 2015

reset commented Dec 18, 2015

fnichol commented Dec 18, 2015

fnichol commented Dec 19, 2015

chef-delivery commented Dec 19, 2015

fnichol commented Dec 19, 2015

adamhjk commented Dec 19, 2015

adamhjk commented Dec 19, 2015

chef-delivery commented Dec 19, 2015

reset commented Dec 19, 2015

[bldr-build] Prefix public callback functions with `do_`. #120

[bldr-build] Prefix public callback functions with `do_`. #120