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

[plan-build] Set $pkg_target at build time for build programs. #5350

Merged
merged 2 commits into from Jul 18, 2018

Conversation

fnichol
Copy link
Collaborator

@fnichol fnichol commented Jul 17, 2018

There are 2 changes at work here: the first to setup up the injection behavior and the second to use it in relevant Habitat Plans.

[plan-build] Set $pkg_target at build time for build programs.

This change updates the build program (both the Bash and PowerShell
implementations) to inject a static package target value into the source
code at Habitat package build time. This means that a built
core/hab-plan-build* package will have a pre-determined,
non-overridable package target.

The build time injection of the package target is fed by the package's
Plan by setting the build program's pkg_target value to its value of
$pkg_target. Make sense? No? Think of it as a pass-through: my build
program's $pkg_target will be set into my program's pkg_target
value. This means that in the Bash implementation, you would see
something like the following in the built package:

pkg_target='x86_64-linux'

and similarily with the PowerShell implementation:

$script:pkg_target = "x86_64-windows"

Bootstrapping Escape Hatch

While the above strategy works for all normal Habitat build use cases,
there's one place where we might need full control over the value of
pkg_target, namely when bootstrapping new package targets or
re-bootstrapping existing ones.

In order to support the bootstrapping case, a specialized behavior is
available when either build program is run directly out of the source
tree (i.e. not run from a built Habitat package). When run like this,
the value of $pkg_target will still contain the build time replacement
token @@pkg_target@@. There is a check right after build program boot
that checks if the value of $pkg_target is @@pkg_target@@. If so,
there are 2 possible behaviors:

  1. If an environment variable of
    $BUILD_PKG_TARGET/$env:BUILD_PKG_TARGET is present, then its value
    will be set for the value of $pkg_target, with a message printed for
    the user. Bootstrapping new package targets or the initial seeded value
    use cases will use this environment variable.
  2. If the above environment variable isn't present, then the prior
    behavior is used. On Bash, uname -s/uname -m is used and on
    PowerShell a predefined string is used. This preserves prior behavior
    and allows users to run the build programs directly out of the source
    tree without having to remember to set a new variable for little to no
    gain.

It should be noted that these 2 bootstrapping cases are not available if
running either build program from a built Habitat package, which is by
design. The intention being that we don't want an errant environment
variable accidentally infecting the build process once we're past any
bootstrapping phase.

Soft Deprecation of pkg_arch and pkg_sys Build Variables

In the process of working this change, I found 2 variables that were set
in each build program that are not only no longer necessary but could be
dangerous if a Plan author was to rely on these variables. Those are:

  • pkg_arch
  • pkg_sys

As the initial author of package target support on the build side, I
will admit to being overly ambitious and adding these variables without
thinking them through. Looking back, they should never have been set up
in the form of public build variables. If there is good news here, I
have since determined that:

  • pkg_arch, pkg_sys, and pkg_target are not currently nor were
    ever documented formally in our docs.
  • pkg_arch and pkg_sys aren't used anywhere internally in the build
    program (other than in service of determining pkg_target)
  • pkg_arch and pkg_sys aren't referenced anywhere in the
    habitat-sh/core-plans repo
  • pkg_arch and pkg_sys aren't referenced in any Plans that I could
    reasonably find searching GitHub

Given all of the above, I've chosen to remove pkg_arch and pkg_sys
and have a note to formally document pkg_target in our reference docs.

Update relevant Habitat Plans to set PLAN_PACKAGE_TARGET.

This change explicitly injects a package target value for Rust software
components that deal with Habitat packages. The PLAN_PACKAGE_TARGET
environment variable is consumed at build time by an
active_package_target function in the habitat_core::package::target
module (which is used once to set a static value at runtime). This is
the mechanism to "bake in" a package target for our tooling.

It should be noted that the PLAN_PACKAGE_TARGET is not a public,
Habitat environment variable. It is an internal Habitat source
code-consuming variable that is not referenced or used anywhere else in
our system (by design).

Additionally, this approach of setting PLAN_PACKAGE_TARGET will not be
used for Builder software components unless we plan to build Builder
packages for platform targets other than x86_64-linux.

@thesentinels
Copy link
Contributor

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

Copy link
Contributor

@baumanj baumanj left a comment

Choose a reason for hiding this comment

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

Looks generally good. I'd just like to see one small change.

@@ -35,6 +35,7 @@ do_build() {
sed \
-e "s,#!/bin/bash$,#!$(pkg_path_for bash)/bin/bash," \
-e "s,^HAB_PLAN_BUILD=.*$,HAB_PLAN_BUILD=$pkg_version/$pkg_release," \
-e "s,^pkg_target=.*$,pkg_target='$pkg_target'," \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this only for the case where we want to replace pkg_target='@@pkg_target@@'? If so, I'd suggest making that explicit:

    -e "s,^pkg_target='@@pkg_target@@'\$,pkg_target='$pkg_target',"

since that makes this code much easier to discover. And if it's not, I'd still suggest escaping the $ in the regex to make it clear that it's behaving as the end-of-line anchor and not a special shell parameter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I waffled on this question as well, but in the interests of local context (i.e. the approach in the previous 2 lines), I went with this one.

So, in order to get the best of both worlds, I addressed this and updated the previous 2 lines for better internal consistency.

Good catch!

This change updates the build program (both the Bash and PowerShell
implementations) to inject a static package target value into the source
code at Habitat package build time. This means that a built
`core/hab-plan-build*` package will have a pre-determined,
non-overridable package target.

The build time injection of the package target is fed by the package's
Plan by setting the build program's `pkg_target` value to its value of
`$pkg_target`. Make sense? No? Think of it as a pass-through: my build
program's `$pkg_target` will be set into my program's `pkg_target`
value. This means that in the Bash implementation, you would see
something like the following in the built package:

```sh
pkg_target='x86_64-linux'
```

and similarily with the PowerShell implementation:

```powershell
$script:pkg_target = "x86_64-windows"
```

Bootstrapping Escape Hatch
--------------------------

While the above strategy works for all normal Habitat build use cases,
there's one place where we might need full control over the value of
`pkg_target`, namely when bootstrapping new package targets or
re-bootstrapping existing ones.

In order to support the bootstrapping case, a specialized behavior is
available when either build program is run directly out of the source
tree (i.e. not run from a built Habitat package). When run like this,
the value of `$pkg_target` will still contain the build time replacement
token `@@pkg_target@@`. There is a check right after build program boot
that checks if the value of `$pkg_target` is `@@pkg_target@@`. If so,
there are 2 possible behaviors:

1. If an environment variable of
`$BUILD_PKG_TARGET`/`$env:BUILD_PKG_TARGET` is present, then its value
will be set for the value of `$pkg_target`, with a message printed for
the user. Bootstrapping new package targets or the initial seeded value
use cases will use this environment variable.
2. If the above environment variable isn't present, then the prior
behavior is used. On Bash, `uname -s`/`uname -m` is used and on
PowerShell a predefined string is used. This preserves prior behavior
and allows users to run the build programs directly out of the source
tree without having to remember to set a new variable for little to no
gain.

It should be noted that these 2 bootstrapping cases are not available if
running either build program from a built Habitat package, which is by
design. The intention being that we don't want an errant environment
variable accidentally infecting the build process once we're past any
bootstrapping phase.

Soft Deprecation of `pkg_arch` and `pkg_sys` Build Variables
------------------------------------------------------------

In the process of working this change, I found 2 variables that were set
in each build program that are not only no longer necessary but could be
dangerous if a Plan author was to rely on these variables. Those are:

* `pkg_arch`
* `pkg_sys`

As the initial author of package target support on the build side, I
will admit to being overly ambitious and adding these variables without
thinking them through. Looking back, they should never have been set up
in the form of public build variables. If there is good news here, I
have since determined that:

* `pkg_arch`, `pkg_sys`, and `pkg_target` are not currently nor were
ever documented formally in our docs.
* `pkg_arch` and `pkg_sys` aren't used anywhere internally in the build
program (other than in service of determining `pkg_target`)
* `pkg_arch` and `pkg_sys` aren't referenced anywhere in the
`habitat-sh/core-plans` repo
* `pkg_arch` and `pkg_sys` aren't referenced in any Plans that I could
reasonably find searching GitHub

Given all of the above, I've chosen to remove `pkg_arch` and `pkg_sys`
and have a note to formally document `pkg_target` in our reference docs.

Signed-off-by: Fletcher Nichol <fnichol@nichol.ca>
This change explicitly injects a package target value for Rust software
components that deal with Habitat packages. The `PLAN_PACKAGE_TARGET`
environment variable is consumed *at build time* by an
`active_package_target` function in the `habitat_core::package::target`
module (which is used once to set a static value at runtime). This is
the mechanism to "bake in" a package target for our tooling.

It should be noted that the `PLAN_PACKAGE_TARGET` is **not** a public,
Habitat environment variable. It is an internal Habitat source
code-consuming variable that is not referenced or used anywhere else in
our system (by design).

Additionally, this approach of setting `PLAN_PACKAGE_TARGET` will not be
used for Builder software components **unless** we plan to build Builder
packages for platform targets other than `x86_64-linux`.

Signed-off-by: Fletcher Nichol <fnichol@nichol.ca>
@fnichol fnichol force-pushed the fnichol/plan-build-target-setting-and-injection branch from dc1b4ba to 7e4aebf Compare July 17, 2018 19:51
@fnichol
Copy link
Collaborator Author

fnichol commented Jul 17, 2018

Nice, addressed!

Copy link
Contributor

@baumanj baumanj left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

Copy link
Contributor

@christophermaier christophermaier left a comment

Choose a reason for hiding this comment

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

tenor-243685115

@fnichol fnichol merged commit 0e0157c into master Jul 18, 2018
@fnichol fnichol deleted the fnichol/plan-build-target-setting-and-injection branch July 18, 2018 17:44
@christophermaier christophermaier added Type:Feature PRs that add a new feature Focus:Supervisor Related to the Habitat Supervisor (core/hab-sup) component Focus: CLI Related to the Habitat CLI (core/hab) component Platform: Linux Deals with Linux-specific behavior Platform: Windows Deals with Windows-specific behavior Type: Feature Issues that describe a new desired feature and removed X-feature labels Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Focus: CLI Related to the Habitat CLI (core/hab) component Focus:Exporter Focus :Plan Build Focus:Supervisor Related to the Habitat Supervisor (core/hab-sup) component Platform: Linux Deals with Linux-specific behavior Platform: Windows Deals with Windows-specific behavior Type:Feature PRs that add a new feature Type: Feature Issues that describe a new desired feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants