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

Make studio backline dependency explicit #6549

Merged
merged 2 commits into from Jun 12, 2019

Conversation

@smacfarlane
Copy link
Contributor

commented May 13, 2019

Ref: #6509

This is initial set of work to make studio dependencies explicit, starting with backline.

In order for the studio to rely on the RUNTIME_ENVIRONMENT in the package metadata, we need to treat the studio enter command the same way we run hab pkg exec. This is currently a copy/paste from hab pkg exec and will need to be refactored in the future. When I started investigating the possibility of doing that work in this PR, it ended up being a much a much larger slice of work that I felt should be captured in its own PR.

Signed-off-by: Scott Macfarlane smacfarlane@chef.io

@chef-expeditor

This comment has been minimized.

Copy link

commented May 13, 2019

Hello smacfarlane! 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!

@smacfarlane smacfarlane force-pushed the sm/studio-dependencies branch from 5ff917f to 759d4b4 Jun 4, 2019

@smacfarlane smacfarlane changed the title Make studio dependencies explicit Make studio backline dependency explicit Jun 4, 2019

@smacfarlane smacfarlane marked this pull request as ready for review Jun 4, 2019

@@ -86,6 +86,23 @@ mod inner {
let ident = PackageIdent::from_str(&format!("{}/{}",
super::STUDIO_PACKAGE_IDENT,
version[0]))?;
// TODO(SM): This is the minimum change needed to allow the studio to specify
// its dependencies. This is a duplicate of the code in `hab pkg exec` and
// should be refactored.

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 5, 2019

Member

Rather than a TODO, let's file an issue with a link back here so it's discoverable

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 5, 2019

Member

Or just refactor now; it doesn't look like it would be too disruptive. What am I missing that makes it so?

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Jun 7, 2019

Author Contributor

Refactoring this would involve updating code in hab pkg exec and core/os/process or PackageInstall which I feel should be a separate PR. Additionally, when I started looking at making the same change for the Windows studio, I found a lot of duplication and not very easy to grok logic to determine how we enter a studio. Refactoring that will be a larger slice of work that I again feel it should be a separate PR.

Given that this change is to help unblock some changes we're making to our release process and it's updating a component that is both critical and not as well tested as we'd like, I'd like to keep this change as minimal as possible to make discovery of any potential issues easy to debug.

I've filed #6633 and #6634 to track the changes needed.

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 7, 2019

Member

Ok, can we replace the TODO with references to those issues?

_hab install "$pkg"
fi
done
_hab install $HAB_STUDIO_BACKLINE_PKG

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 5, 2019

Member

Can you explain a little why CI_OVERRIDE_CHANNEL is no longer necessary? I'm happy to see it go, I just don't understand why.

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Jun 7, 2019

Author Contributor

Before this change, core/hab-backline was an implicit dependency of the studio used when the studio is invoked. Since it's not declared, the studio would have to determine what backline package to use every time it is run. This would always use the latest stable version of the package that matches core/hab-backline/VERSION. In the context of a release, we don't want the latest stable but we can't rely on HAB_BLDR_CHANNEL as that has a different intent and overloading it to test backline caused issues (Note: I wasn't around at the time so I don't know the specifics) so CI_OVERRIDE_CHANNEL was introduced.

By adding core/hab-backline to pkg_deps, we can leverage the plan-build logic to always resolve that to a fully qualified ident at build time, which allows us to set $HAB_STUDIO_BACKLINE_PKG at the time of build. With the change to studio enter to set up the environment as specified by the studio package, we know exactly what backline package the studio was built with and can correctly install it on the inside of the studio.

This is in alignment with how packages in Habitat are intended to work. Rather than the existing behavior where we've built a back door in order to test changes to a component, we now rebuild the dependent packages against the new version and CI_OVERRIDE_CHANNEL can go away.

This comment has been minimized.

Copy link
@christophermaier

christophermaier Jun 7, 2019

Contributor

This is my favorite thing!

display_args.push(' ');
display_args.push_str(arg.to_string_lossy().as_ref());
}
debug!("Running: {}", display_args);

This comment has been minimized.

Copy link
@christophermaier

christophermaier Jun 7, 2019

Contributor

This could be a note for the future refactorings, rather than a change for this PR, but this is a lot of code for just printing out what's going to be run. It could be done more concisely (e.g., with some help from SliceConcatExt::join).

(As an aside, it's currently doing all that concatenation even if debug logging is disabled, making it wasted effort.)

Reading more of the surrounding code, it also seems out of place here, given that this block of code is just trying to figure out which Studio binary to run; the arguments only really get used later on.

Similarly, jamming the environment variables directly into the running environment here, rather than somehow conveying it out to the point where the command is actually run, strikes me as a bad idea. Sure, it ends up working out to the same thing in this case, since we exec to become the studio process, but messing with global state at a point so distant from the actual execution just seems like begging for bugs.

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Jun 7, 2019

Author Contributor

(As an aside, it's currently doing all that concatenation even if debug logging is disabled, making it wasted effort.)

Reading more of the surrounding code, it also seems out of place here, given that this block of code is just trying to figure out which Studio binary to run; the arguments only really get used later on.

Yup 😭 I couldn't see a low-impact way of doing this only when necessary, studio/enter.rs is need of a lot of cleanup. Ideally we'd do that work up front, but this will unblock a lot of the improvements to our release process. I'm open to suggestions, but as I noted above I feel that work should be it's own PR and keep this isolated to changes necessary to make backline a runtime dependency.

This comment has been minimized.

Copy link
@christophermaier

christophermaier Jun 7, 2019

Contributor

@smacfarlane Yup, I totally get the constraints you're operating under. Feel free to take my suggestions as todo items on that future PR.

I would like for us to be able to tackle that PR in the very near future, however, just so we can not let bad code lie any longer than absolutely necessary.

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 7, 2019

Member

I had to abandon #6347, but I think that direction would take us towards something cleaner and more maintainable.

@@ -13,6 +15,10 @@ pkg_build_deps=(core/coreutils
core/hab)
pkg_bin_dirs=(bin)

do_prepare() {
set_runtime_env "HAB_STUDIO_BACKLINE_PKG" "$(< $(pkg_path_for core/hab-backline)/IDENT)"

This comment has been minimized.

Copy link
@christophermaier

christophermaier Jun 7, 2019

Contributor

Is this going to be a permanent thing, or is this basically some support code to bridge us to the glorious future when all this code is sane? 😅

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Jun 7, 2019

Author Contributor

I suspect semi-permanent as RUNTIME_ENVIRONMENT is our interface for influencing the runtime (but not the internals!) of the studio. Though... I just had a thought that may allow us to remove this. I'll run some experiments, but I think for now this should stay as-is.

_hab install "$pkg"
fi
done
_hab install $HAB_STUDIO_BACKLINE_PKG

This comment has been minimized.

Copy link
@christophermaier

christophermaier Jun 7, 2019

Contributor

This is my favorite thing!

@smacfarlane smacfarlane force-pushed the sm/studio-dependencies branch from 350484b to d9131e2 Jun 11, 2019

smacfarlane added some commits May 3, 2019

Initial pass at making backline dependency explicit
Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
[PR Feedback] Address feedback and CI
Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

@smacfarlane smacfarlane force-pushed the sm/studio-dependencies branch from d9131e2 to fbaac3f Jun 11, 2019

@smacfarlane smacfarlane merged commit 26f12d1 into master Jun 12, 2019

5 checks passed

DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #2279 passed (1 hour, 23 minutes, 54 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #2580 passed (4 minutes, 10 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

@smacfarlane smacfarlane deleted the sm/studio-dependencies branch Jun 12, 2019

chef-ci added a commit that referenced this pull request Jun 12, 2019

Update CHANGELOG.md with details from pull request #6549
Obvious fix; these changes are the result of automation not creative thinking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.