Support local buildpacks and meta-buildpacks in libcnb-test (outdated) #590

colincasey · 2023-07-07T19:31:38Z

The following changes have made to support local testing of buildpacks and meta-buildpacks:

refactored shared functionality between libcnb-cargo and libcnb-test into libcnb-package
modified the test runner to support a BuildpackReference::Local variant which is similar to BuildpackReference::Crate because both represent buildpacks on the file-system which need to be compiled
after local and crate references are compiled, they are packaged into Docker images to get around an issue with pack not handling meta-buildpacks correctly (Make pack build --buildpack <path> apply configuration in package.toml buildpacks/pack#1320)
all the images created during the test are then cleaned up afterwards

These changes also affects #583 since the functionality for getting a target buildpack output directory path was moved into BuildpackOutputDirectoryLocator.

If you want to try this out locally in a buildpack project:

check out this branch
use a path references for the libcnb-test dependency

 libcnb-test = { path = "/Users/ccasey/Projects/heroku/libcnb.rs/libcnb-test" }

use the BuildpackReference::Local variant to specify a path to a buildpack or meta-buildpack

TestRunner::default().build(
  BuildConfig::new("heroku/builder:22", "/path/to/app")
    .buildpacks(vec![
      BuildpackReference::Local(PathBuf::from("../../meta-buildpacks/nodejs"))
  ]), 
  |ctx| {
    // test body
  }
)

GUS-W-13871688.

The following changes have made to support local testing of buildpacks and meta-buildpack: - refactored shared functionality between `libcnb-cargo` and `libcnb-test` into `libcnb-package` - modified the test runner to support a `BuildpackReference::Local` variant which is similar to `BuildpackReference::Crate` because both represent buildpacks on the file-system which need to be compiled - after local and crate references are compiled, they are packaged into Docker images to get around an issue with pack not handling meta-buildpacks correctly (buildpacks/pack#1320) - all the images created during the test are then cleaned up afterwards

schneems

Ed and Manuel: I have some questions and comments related to libcnb-package in here. You can respond out of band or I can open an issue where it would be better to have a discussion. Raising them here since it's top of mind, and one or both of you will need to review before we can merge.

Colin: Thanks again for undertaking this. I also appreciate the walkthrough. I marked some things as change requested, we mostly covered those on the call. We also talked about adding an example usage to the existing TestRunner examples to show that it's an option.

I left a bunch of comments, they're marked as such. Writing helps me process. Also I marked other changes as optional.

In addition I also want to mention that we talked about the possibility of a performance optimization. After discussion I don't think we should do it now, but it's worth exploring later. This section is purely for future reference about what we talked about

Compile once

The current implementation will compile the required local buildpack for every test invocation. We talked about moving to a compile once. This is the model used in Cutlass. To do it I used:

A class similar to the Local variant https://github.com/heroku/cutlass/blob/ea2e67a3f3ccd1a3f14bb3e67cfa37b36537297c/lib/cutlass/local_buildpack.rb
Teardown/destroy was provided by the test execution framework: https://github.com/heroku/cutlass#initial-config-localbuildpack-for-packagetoml

One challenge of the current implementation is that the buildpacks are compiled and then put into containers. This container packaging is done to support multibuildpacks. If we wanted to optimize to only compile once, then we would end up with a resource that could possibly leak. Also Rust makes sharing across threads/tests significantly harder.

Originally I proposed using something like lazy_static to create and retain a single temp dir where all buildpacks could be compiled and referenced by directory namespace. The OCI part is harder. Also if it's worth optimizing, it's probably worth profiling a bit to gauge how expensive the container creation and stuffing is versus the compilation (my hunch is that it's much quicker, but I would like to verify).

I think it's worth validating the existing logic and implementation and getting this working (I want to use it soon) and then work to refactor for performance later.

schneems · 2023-07-21T20:11:20Z

CHANGELOG.md

+
+### Changed
+
+- `libcnb-package`


[Question for maintainers, no action] More of a question for Manuel and Ed. This package is mostly exposed for re-use in our tooling. I would assume that we don't expect others to use these interfaces directly or do we?

Are we treating libcnb-package as a "true" cargo project with version guarantees, or is it more like this internal serde crate https://crates.io/crates/serde_derive_internals?

Are we treating libcnb-package as a "true" cargo project with version guarantees, or is it more like this internal serde crate

Good question. I'd thought the former, but I'm open to either really, as long as we're clear with potential external consumers of the create. Also at the moment with libcnb being pre-v1, we're bumping the major version most releases, so this mostly won't be an issue - and later once we go v1 I suspect libcnb-package will have settled down enough that we don't expect too much breakage.

One thing to watch out for though - if we were to switch to being more relaxed about the major version for libcnb-package - we'd really have to switch to using hard version pins instead of ranges for intra-crate dependencies. ie: libcnb-cargo would need to depend on libcnb-package version ==1.2.3 not ^1.2.3. (We should perhaps make that switch regardless given our unified release versions approach.)

@schneems Just to follow up here - we've now (a) switched to hard version pins in #644, (b) marked libcnb-package (and the new libcnb-common) as internal in their READMEs in #662.

schneems · 2023-07-21T20:14:03Z

libcnb-test/src/build_config.rs

@@ -251,6 +251,8 @@ pub enum BuildpackReference {
    Crate,
    /// References another buildpack by id, local directory or tarball.
    Other(String),
+
+    Local(PathBuf),


[Change Request]: Talked about on the call. I would like to see this be more descriptive about the desired functionality, something like CompileLocal. Also add a docstring to the variant.

@schneems we also discussed that it could be a buildpack id within your project space.

@Malax should we just rename Other to Uri and do detection in the runner for if it's a local buildpack id or path? that way we could have it align with how values in package.toml are treated during packaging.

we also discussed that it could be a buildpack id within your project space.

Right. I like the ergonomics of that more, if it's not too difficult to implement. One thought is that it might get confusing to see:

Other(String::from("heroku/yarn-engine")), CompileLocal(String::from("heroku/nodejs-engine"))

Which is related to your question to Manuel.

IDK what he thinks, but my preference is generally to try to separate out refactorings from new features unless they're in conflict. It makes it easier to follow the changes and review, though it's more work for implementers. I'm suggesting we push refactoring/changing Other() until after this PR is merged. Also, I'm willing to do some of that work if needed.

To the end of should it be changed. I think the interface is already confusing. It was unclear to me that specifying heroku/jvm-internal in the java buildpack test would pull in the one from the builder and not the local one. To that end I think it's worth considering what it does already it handles:

Path to already compiled buildpack (zipped or not)

Buildpack Id

Something else?

Maybe we separate these out into their own variants. For the Buildpack Id case it will work if a buildpack is specified via the builder or via the CLI flags i.e. --buildpack <path/to/heroku/node> I'm not sure what to name or call that. Maybe "ExternalBuildpack" or "ExternalBuildpackID"?

libcnb-test/src/pack.rs

schneems · 2023-07-21T20:27:01Z

libcnb-package/src/lib.rs

-        );
-    }
+#[derive(Debug)]
+pub enum FindCargoWorkspaceError {


[Question, no action] Question to maintainers:
I noticed this doesn't have a Display implementation. If we say that libcnb-package is an "actual" crate then I think we would want that and some docs for the variants (or if we thiserror the display would function as a doc of sorts.

Looking at other examples in libcnb-package I'm seeing that:

No pub enum comes with a display implementation, at least one doesn't even have Debug (CrossCompileAssistance) so scratch that comment

Documentation on pub enum is inconsistent. Some have enum level docs, some have variant level docs. I would defer to Manuel and Ed for some guidance on when to do which.

For libcnb-package we should possibly consider labeling it as a stronger "unstable" rather than simply "you should consider using a different package instead". I still believe in documenting private interfaces (in shared code) we should still have some consistency.

schneems · 2023-07-21T20:33:39Z

libcnb-package/src/lib.rs

+        .args(["locate-project", "--workspace", "--message-format", "plain"])
+        .current_dir(dir_in_workspace)
+        .output()
+        .map_err(FindCargoWorkspaceError::SpawnCommand)?;


[Comment, no action] - Invoking a command to get this information jumped out as being unusual. At first I thought you were shelling to cargo libcnb but then realized this is calling cargo locate-project and parsing the output. I found the code https://github.com/rust-lang/cargo/blob/b40be8bdcf2eff9ed81702594d44bf96c27973a6/src/bin/cargo/commands/locate_project.rs it does seem like it's not exposed anywhere, which would have been convenient.

schneems · 2023-07-21T20:36:14Z

libcnb-package/src/lib.rs

+///
+/// ## Errors
+///
+/// Will return an `Err` if the root workspace directory cannot be located.


[Optional] It wasn't clear to me the first read through that this could be intentional (i.e. everything worked correctly and cargo couldn't find a worspace) or accidental (running a binary with this function on a system that doesn't have cargo installed, somehow or the OS throws an error like OOM, etc.)

We could expand the errors section here.

schneems · 2023-07-21T20:44:09Z

libcnb-package/src/output.rs

+pub fn assemble_meta_buildpack_directory(
+    destination_path: impl AsRef<Path>,
+    buildpack_source_dir: impl AsRef<Path>,
+    buildpack_path: impl AsRef<Path>,


[Question, possible action requested] What's the difference between buildpack_path and buildpack_source_dir ? I looked at the call sites:

&buildpack_package.path, &buildpack_package.buildpack_data.buildpack_descriptor_path,

and

assemble_meta_buildpack_directory( target_dir, &buildpack_package.path, &buildpack_package.buildpack_data.buildpack_descriptor_path, buildpack_package .buildpackage_data .as_ref() .map(|data| &data.buildpackage_descriptor), buildpack_output_directory_locator, ) .map_err(PackageBuildpackError::AssembleBuildpackDirectory)

It's still unclear. It would be good to get a comment or a more descriptive variable name.

buildpack_path is the buildpack.toml file

buildpack_source_dir which is the directory that contains the buildpack.toml file.

the buildpack_source_dir could be derived from buildpack_path though since it's the parent directory. alternatively, the buildpack_path could be replaced with BuildpackData which might add some clarity to the signature as well.

the buildpack_source_dir could be derived from buildpack_path though since it's the parent directory. alternatively, the buildpack_path could be replaced with BuildpackData which might add some clarity to the signature as well.

Thanks for the clarification. I think a buildpack_toml with impl AsRef<Path> would be clearer if you want to change it and a larger refactor doesn't work out.

Regarding changing it from two inputs to one: I'm unsure if there's ever a valid reason for a buildpack path to be different than the toml path.

Regarding changing it to pass in the data directly: In Ruby I generally like passing in derived values i.e. instead of passing in a filename if we need its contents, we could pass in the contents directly. Or passing in a boolean of whether a hash has a key instead of passing in the whole hash. In rust this seems less needed but still a nice pattern when it can be used. To that end I like passing in BuildpackData directly, with the caveat that I don't want you to have to jump through hoops, especially if it means duplication.

libcnb-test/src/pack.rs

libcnb-test/src/test_runner.rs

libcnb-test/tests/integration_test.rs

Co-authored-by: Richard Schneeman <richard.schneeman+no-recruiters@gmail.com>

schneems · 2023-07-27T16:21:55Z

Linking some in-progress use cases that we can come back to after merge:

@colincasey I see some activity since our conversation are you ready for a review from Manuel/Ed or still working on updates?

colincasey · 2023-07-31T18:04:09Z

this PR needs to sync up with recent changes to libcnb-test but i'm not quite ready for that yet @schneems. in the meantime, i wouldn't mind if @Malax or @edmorley have time to provide feedback on the refactoring between libcnb-cargo and libcnb-package that were done to share logic between these three crates.

edmorley · 2023-08-07T09:02:35Z

after local and crate references are compiled, they are packaged into Docker images to get around an issue with pack not handling meta-buildpacks correctly (Make pack build --buildpack <path> apply configuration in package.toml buildpacks/pack#1320)

Thanks to Colin's PR buildpacks/pack#1841, that issue is now fixed in Pack CLI v0.30.0-rc1:
https://github.com/buildpacks/pack/releases/tag/v0.30.0-rc1

As such, I think the workaround should be removed from this PR, since:
(a) the final Pack CLI 0.3.0 release will be out shortly (and we can force the release candidate in CI in the meantime via the pack-version option to the setup-pack Action)
(b) we don't really need to worry about users with old Pack CLI, since this is a libcnb-test only issue, so only something buildpack maintainers will run into (vs an issue that prevents end-users performing a normal pack build with our published buildpacks)

edmorley · 2023-08-08T08:35:45Z

libcnb-test/src/pack.rs

+}
+
+#[derive(Clone, Debug)]
+pub(crate) struct PackBuildpackPackageCommand {


This change can be dropped now that the Pack CLI bug has been fixed

edmorley · 2023-08-08T08:37:02Z

libcnb-test/src/pack.rs

+    }
+}
+
+pub(crate) fn run_buildpack_package_command<C: Into<Command>>(command: C) {


(there's the new run_command util after #619 btw; though moot point given this can be removed due to the Pack CLI bug being fixed)

edmorley · 2023-08-08T08:37:42Z

libcnb-test/src/test_context.rs

-        let _image_delete_result =
-            self.runner
+        let mut images = vec![&self.image_name];
+        images.extend(&self.buildpack_images);


This change can be dropped now that the Pack CLI bug has been fixed

edmorley · 2023-08-08T08:38:14Z

libcnb-test/src/test_runner.rs

@@ -101,6 +101,15 @@ impl TestRunner {
    ) {
        let config = config.borrow();

+        let mut test_context = TestContext {


This change can be dropped now that the Pack CLI bug has been fixed

edmorley · 2023-08-08T08:39:26Z

libcnb-test/tests/integration_test.rs

@@ -132,14 +133,29 @@ fn starting_containers() {

 #[test]
 #[ignore = "integration test"]
-#[should_panic(
-    expected = "Could not package current crate as buildpack: BuildBinariesError(ConfigError(NoBinTargetsFound))"


This test refactoring might be best as a separate PR that lands before this one

edmorley

Thank you for working on this!

I've had a quick look through the PR, but am finding it quite hard to review quickly given the size of the PR.

Now that the Pack CLI bug has been fixed, it seems a fair bit of the complexity/size of this PR might go away, since the solution no longer needs to create an intermediate Docker image for the meta-buildpacks (and keep track of cleaning them up etc).

Another thing that might make the PR easier to review is to split out any changes that can be made separately. For example, unrelated refactorings/changes, or even changes that are related to this work, but are preparatory in nature, and could be made in a prior PR. For example, for the bollard migration work I ended up opening half a dozen PRs, only the last of which was actually the migration itself - which made it much easier for the PR reviewers.

colincasey · 2023-08-08T20:11:15Z

fair point, @edmorley. i've split out the bulk of the refactoring work into #628.

colincasey · 2023-08-30T19:30:30Z

Closing this for #666

colincasey added 2 commits July 7, 2023 16:06

Ignore the target workspace dir

d2cd784

colincasey added libcnb-test libcnb-package labels Jul 19, 2023

colincasey added 3 commits July 19, 2023 16:28

Clean up error handling and docs

67a9af2

Updated CHANGELOG.md

df125ca

Updated integration tests

df58c3c

colincasey marked this pull request as ready for review July 20, 2023 12:48

colincasey requested a review from a team as a code owner July 20, 2023 12:48

schneems requested changes Jul 21, 2023

View reviewed changes

Update libcnb-test/src/pack.rs

2042df4

Co-authored-by: Richard Schneeman <richard.schneeman+no-recruiters@gmail.com>

edmorley assigned colincasey Aug 2, 2023

edmorley added enhancement New feature or request breaking change labels Aug 2, 2023

edmorley reviewed Aug 8, 2023

View reviewed changes

edmorley requested changes Aug 8, 2023

View reviewed changes

edmorley mentioned this pull request Aug 10, 2023

Refactor libcnb-package and libcnb-cargo in preparation for test support #628

Closed

colincasey changed the title ~~Support local buildpacks and meta-buildpacks in libcnb-test~~ Support local buildpacks and meta-buildpacks in libcnb-test (outdated) Aug 30, 2023

colincasey mentioned this pull request Aug 30, 2023

Support local buildpacks and meta-buildpacks in libcnb-test #666

Merged

colincasey closed this Aug 30, 2023

colincasey deleted the libcnb_test_local_and_meta_buildpack_support branch August 30, 2023 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support local buildpacks and meta-buildpacks in libcnb-test (outdated) #590

Support local buildpacks and meta-buildpacks in libcnb-test (outdated) #590

colincasey commented Jul 7, 2023 •

edited by edmorley

Loading

schneems left a comment

schneems Jul 21, 2023

edmorley Aug 7, 2023 •

edited

Loading

edmorley Aug 30, 2023

schneems Jul 21, 2023

colincasey Jul 24, 2023

schneems Jul 25, 2023

schneems Jul 21, 2023

schneems Jul 21, 2023

schneems Jul 21, 2023

schneems Jul 21, 2023

colincasey Jul 24, 2023

schneems Jul 25, 2023

schneems commented Jul 27, 2023

colincasey commented Jul 31, 2023

edmorley commented Aug 7, 2023

edmorley Aug 8, 2023

edmorley Aug 8, 2023

edmorley Aug 8, 2023

edmorley Aug 8, 2023

edmorley Aug 8, 2023

edmorley left a comment

colincasey commented Aug 8, 2023

colincasey commented Aug 30, 2023

Support local buildpacks and meta-buildpacks in libcnb-test (outdated) #590

Support local buildpacks and meta-buildpacks in libcnb-test (outdated) #590

Conversation

colincasey commented Jul 7, 2023 • edited by edmorley Loading

schneems left a comment

Choose a reason for hiding this comment

Compile once

Choose a reason for hiding this comment

edmorley Aug 7, 2023 • 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

Choose a reason for hiding this comment

schneems commented Jul 27, 2023

colincasey commented Jul 31, 2023

edmorley commented Aug 7, 2023

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

edmorley left a comment

Choose a reason for hiding this comment

colincasey commented Aug 8, 2023

colincasey commented Aug 30, 2023

colincasey commented Jul 7, 2023 •

edited by edmorley

Loading

edmorley Aug 7, 2023 •

edited

Loading