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

Refactor libcnb-cargo #657

Merged
merged 5 commits into from
Aug 29, 2023
Merged

Refactor libcnb-cargo #657

merged 5 commits into from
Aug 29, 2023

Conversation

Malax
Copy link
Member

@Malax Malax commented Aug 28, 2023

I would've loved to split this PR up a little more. However, one of the main objectives of this PR was to remove
complexity (in the original sense of the word: consisting of many interconnecting parts or elements) that was quite
the challenge. I managed to break out some PRs regardless: #659, #658, #656, #654, #652, #653 (all merged).

Some further refactoring will be done in subsequent PRs to keep this one smaller. See the issues for these:

To ease review, I'd suggest taking a look at the new files in libcnb-package first. They are self-contained and can
be reviewed individually. Then, review libcnb-cargo/src/package/command.rs top to bottom. The diff for that file
isn't very helpful. As the last step, go over the diff of the PR for the filler code.

High Level Overview of Changes

Errors

Split errors into dedicated types where it made sense. Most functions in libcnb-package now have their own error type that implements Display, Debug and Error. Splitting them allows us to build an error hierarchy when needed. For example, packaging a directory as a buildpack can either fail with an error specific to meta-buildpack packaging or
and error when packaging a libcnb.rs buildpack.

This slimmed down the main Error type for libcnb-cargo's package command. Combined with the previous PR that aimed to simplify errors in libncb-cargo (#652), error.rs went from ~250 lines to just 28.

Since Display, Debug and Error moved to the error types, we can re-use these representations outside of
libcnb-cargo. For example, support for testing meta-buildpacks to libcnb-test can re-use the error types and string
representations - less effort implementation wise and error messages are consistent.

If we ever want to add more elaborate error messages to libcnb-cargo, for example adding hints about CLI flags that
are out of scope for the generic string representation, we can still match against the concrete error value we're
interested in and print a different message.

Moving build and packaging logic to libcnb-package

Similarly to #633, this PR moves all packaging and build logic to libcnb-package so that it can be re-used outside
of libcnb-cargo. Functions have been refactored to be less intertwined with others and make fewer assumptions. One
big change is the explicit mapping from buildpack IDs to packaged paths. Earlier, libcnb: URIs where replaced by
"guessing" where the built output should be located. Functions that deal with libcnb: URIs not take an explicit
mapping.

Dependency Graph as Single Source of Truth

The dependency graph of CNBs was previously only used to determine the build order. With this PR, it becomes the single source of truth. As before, at the start of the program, the graph is being built. Afterwards, we query the graph when we need information about the buildpacks in the cargo workspace.

For example, to determine if we only need want to build the buildpack in $CWD, we simply query the graph for a node
that represents a buildpack located at $CWD - no additional validation of the contents of $CWD required.

The dependency graph also now only contains nodes and edges that are relevant to the program. CNBs in the workspace that do not need to (or cannot) be packaged with libcnb-cargo are omitted. Also, dependencies that are not libcnb.rs CNBs in source form are not represented in the graph anymore. This simplifies the code that comes after the building of the dependency graph since no filtering is required after that point.

A module to build this dependency graph has been added to libcnb-package so it can be re-used.

Support for non-libcnb.rs CNBs

Previously, libcnb-cargo had some special cases to support non-libcnb.rs buildpacks. I removed those since they're no longer necessary. libcnb-cargo will now simply leave non-libcnb.rs CNBs untouched but keep references (in
package.toml files) to them. This allows meta-buildpacks with non-libcnb.rs CNBs to continue working as expected
without any special logic inlibcnb-cargo.

The only change for end-users is that those buildpacks will no longer be printed as "being built" by libcnb-cargo.

Removal of BuildpackData<_> and PackageDescriptorData<_>

These wrapper types were basically just PathBuf + BuildpackDescriptor and PackageDescriptor respectively. In many cases the path wasn't used. Functions now only take BuildpackDescriptor and PackageDescriptor and a path if needed. This allows us to remove these wrappers that had their own machinery to read them from disk. Since we already have a generic way of reading TOML files (read_toml_file/write_toml_file) we can now use those instead.

Apart from removing code, the cognitive burden is lessened. I found it hard to wrap my head around these wrappers
since they are special constructs that are only used in the libcnb-cargo context.

Fixes: #638, #599

@Malax Malax force-pushed the malax/refactor-libcnb-cargo-2 branch 3 times, most recently from b8d42e4 to 99a3c4f Compare August 28, 2023 12:45
@Malax Malax force-pushed the malax/refactor-libcnb-cargo-2 branch 2 times, most recently from 2c90b90 to 33eb62e Compare August 28, 2023 13:17
@Malax Malax force-pushed the malax/refactor-libcnb-cargo-2 branch 6 times, most recently from 5991c20 to fa456ed Compare August 28, 2023 17:36
@Malax Malax force-pushed the malax/refactor-libcnb-cargo-2 branch 2 times, most recently from 199adb6 to 8d32d7d Compare August 29, 2023 08:45
@Malax Malax changed the base branch from main to malax/libcnb-common August 29, 2023 08:46
@Malax Malax force-pushed the malax/refactor-libcnb-cargo-2 branch 2 times, most recently from ddcab7f to 5b0152c Compare August 29, 2023 09:06
@Malax
Copy link
Member Author

Malax commented Aug 29, 2023

I'm not sure what to do with the CHANGELOG here, especially when we talk about libcnb-package. It would essentially read: "everything changed, take a look". Ideally, libcnb-package wouldn't be public, but it is. :/

Base automatically changed from malax/libcnb-common to main August 29, 2023 10:18
@Malax Malax force-pushed the malax/refactor-libcnb-cargo-2 branch from 5b0152c to 4e45ab4 Compare August 29, 2023 10:20
@Malax Malax marked this pull request as ready for review August 29, 2023 10:22
@Malax Malax requested a review from a team as a code owner August 29, 2023 10:22
@Malax Malax force-pushed the malax/refactor-libcnb-cargo-2 branch from 4e45ab4 to 36915d5 Compare August 29, 2023 10:23
Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Thank you!

libcnb-package/src/lib.rs Show resolved Hide resolved
libcnb-package/src/package.rs Outdated Show resolved Hide resolved
libcnb-package/src/package.rs Outdated Show resolved Hide resolved
libcnb-package/src/package_descriptor.rs Outdated Show resolved Hide resolved
libcnb-package/src/util.rs Show resolved Hide resolved
libcnb-package/src/util.rs Outdated Show resolved Hide resolved
libcnb-cargo/src/package/command.rs Show resolved Hide resolved
libcnb-package/src/package_descriptor.rs Show resolved Hide resolved
@edmorley
Copy link
Member

I'm not sure what to do with the CHANGELOG here, especially when we talk about libcnb-package. It would essentially read: "everything changed, take a look". Ideally, libcnb-package wouldn't be public, but it is. :/

One option would be for us to update libcnb-package's readme to say something like "this is an internal libcnb implementation detail, API stability guarantees do not apply" - then we can just skip describing changes in the changelog, and only discuss them in terms of whether end-user behaviour of libcnb-{cargo,test} changes?

We could also add something similar to the readme of libcnb-common, and have anything in there that we think end-users will use be re-exported via libcnb instead.

@Malax Malax force-pushed the malax/refactor-libcnb-cargo-2 branch from c4adf79 to 6a276b8 Compare August 29, 2023 13:53
@Malax Malax force-pushed the malax/refactor-libcnb-cargo-2 branch from 6a276b8 to 1c1ebea Compare August 29, 2023 15:00
@Malax Malax enabled auto-merge (squash) August 29, 2023 15:00
@Malax Malax merged commit 7e80021 into main Aug 29, 2023
4 checks passed
@Malax Malax deleted the malax/refactor-libcnb-cargo-2 branch August 29, 2023 15:04
@Malax Malax mentioned this pull request Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants