-
Notifications
You must be signed in to change notification settings - Fork 6
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
Compiling all project types with libcnb package #575
Conversation
* Compile all Buildpacks in a project when run from the root of a Cargo Workspace * Compile a single buildpack or meta-buildpack when run from within a directory containing a `buildpack.toml` Notable changes to how the command operates: * Buildpacks are compiled to a folder structure similar to `target/buildpack/{buildpack_id}` * The target directory of each compiled Buildpack is written to `stdout` * Meta-buildpacks will detect any local buildpack dependencies that are referenced via file paths in `package.toml` (e.g.; `uri = “libcnb:buildpack_id”`) and will ensure that those buildpacks are compiled first
* main: Update fastrand requirement from 1.8.0 to 2.0.0 (#573) Fix lint failures with Rust 1.70 (#574) Bump Swatinem/rust-cache from 2.3.0 to 2.4.0 (#572) Bump Swatinem/rust-cache from 2.2.1 to 2.3.0 (#571) Bump buildpacks/github-actions from 5.1.0 to 5.2.0 (#569) Prepare 0.12.0 release (#568) Support Buildpack API `0.9` (#567) Add Clone for Scope (#566) Improve Env::get, add Env::get_string_lossy (#565) Bump buildpacks/github-actions from 5.0.1 to 5.1.0 (#564)
libcnb package
command to:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this has come a long way since the first review we did some time ago, good job! :) There are still some kinks to iron out, but nothing that requires re-architecting the code-base. LMK when I didn't to a great job explaining what I mean in my comments so that I have a chance to fix them :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do another polish pass over this before reviewing since:
- Code doesn't pass linting checks
- Many identifiers still have their old names from when the graph stuff was buildpack specific which isn't accurate anymore.
- We said we wanted to change the naming for some things we introduced during our pairing session,
TopoSort#deps
for example. I leaning towardsTopoSort
itself not being a great name. - IntelliJ reported spelling mistakes (I fixed them via 1c2f423 already)
- At least one function (
lookup_buildpack_package_node_index
) is no longer useful after we removed the buildpack specifics since it doesn't to much anymore - the cost of keeping it around is higher than coping the body to the locations it's being used.
From my previous review and the changes we made in our paring session, I think merging is fine after we did another polish run.
51e9a5f
to
70f89c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of my comments from the previous review weren't resolved, especially
Many identifiers still have their old names from when the graph stuff was buildpack specific which isn't accurate anymore.
I went ahead and did it myself to speed things up. See: 70f89c1.
I think we can merge this in this state now, after @edmorley's review comments have been addressed.
This PR modifies the
libcnb package
command to:buildpack.toml
Notable changes to how the command operates:
target/buildpack/{debug|release}/{buildpack_id}
stdout
package.toml
(e.g.;uri = "libcnb:dependent_buildpack_id"
) and will ensure that those buildpacks are compiled firstbin/detect
andbin/compile
script will be left untouched and their source directory will be outputCloses #215, GUS-W-12731729
Example output
STDERR
STDOUT
STDERR
STDOUT
STDERR
STDOUT