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

Add Cargo command for cross-compiling and packaging buildpacks #199

Merged
merged 41 commits into from
Dec 3, 2021

Conversation

Malax
Copy link
Member

@Malax Malax commented Nov 24, 2021

This PR adds a Cargo command binary crate to build and package libcnb.rs based buildpacks.

Previously, every buildpack author had to come up with a way to package their buildpack that conformed both to the CNB spec as well as the assumptions libcnb.rs makes about how it's packaged. Some used cargo-make, others bash scripts. Writing a proper build script and setting up the project is hard since it's common that a buildpack must be cross-compiled to another platform. Our own Ruby example shipped with a Makefile.toml for cargo-make, but did not document how to use it. All of the above made getting started with libcnb.rs very hard, especially since there were no docs around this either.

With this new Cargo command, libcnb.rs users can use a centrally managed packaging tool for their buildpacks and can fully concentrate on writing their buildpack instead of build logic. It tries to make cross-compiling easier by providing useful hints.

The README has been updated to transform the existing "Hello World Buildpack" section into an end-to-end quickstart guide that includes packaging and running the buildpack.

Installation

Users will install it via cargo install libcnb-cargo, to try it out before the release on crates.io run:

$ cargo install --path /path/to/libcnb.rs/libcnb-cargo

Usage

USAGE:
    cargo libcnb package [FLAGS] [OPTIONS]

FLAGS:
    -h, --help       Prints help information
        --release    Build in release mode, with optimizations
    -V, --version    Prints version information

OPTIONS:
    -o, --output <output-path>    Write buildpack tarball to this path instead of Cargo's target directory
        --target <target>         Build for the target triple [default: x86_64-unknown-linux-musl]

Example Output

$ cargo libcnb package
INFO - Reading buildpack metadata...
INFO - Found valid buildpack with id "libcnb-examples/my-buildpack" @ 0.1.0!
INFO - Building buildpack binary (x86_64-unknown-linux-musl)...
   Compiling syn v1.0.82
   Compiling bit-vec v0.6.3
   Compiling regex-syntax v0.6.25
   # Omitting further compilation output...
    Finished dev [unoptimized + debuginfo] target(s) in 19.71s
INFO - Writing buildpack tarball...
INFO - Successfully wrote buildpack tarball target/libcnb-examples_my-buildpack_0.1.0_dev.tar.gz (6.5M)
INFO - Packaging successfully finished!
INFO - Hint: To test your buildpack locally with pack, run: pack build my-image -b target/libcnb-examples_my-buildpack_0.1.0_dev.tar.gz --path /path/to/application

What's next

I consider this an MVP, there are many improvements that could be made and it could have more features. We'll implement these features and fixes along the way.

This new libcnb-cargo crate is, besides a binary crate, also a library crate that exposes the building blocks for the packaging process. The intent is to use these building blocks in an integration test solution, ensuring parity between command line packaging and integration testing.

GUS-W-10229113.

@Malax Malax force-pushed the malax/cargo branch 6 times, most recently from e2b0bd2 to 5ae27b4 Compare November 24, 2021 16:46
@Malax Malax marked this pull request as ready for review November 25, 2021 12:39
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 so much for working on this! Also love the new guide in README.

Here's an interim review (mainly on docs) whilst I work on the rest :-)

Cargo.toml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
libcnb-cargo/Cargo.toml Show resolved Hide resolved
@edmorley edmorley self-requested a review November 25, 2021 16:36
Malax and others added 3 commits November 25, 2021 17:45
Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com>
Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com>
Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com>
Malax and others added 2 commits November 26, 2021 11:31
Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com>
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 so much for this! I've reviewed most things so far, apart from assemble_buildpack_tarball() (and also how that might interact with meta-buildpacks). I'll finish the rest by Monday.

Some general comments:

  • there are no tests, what do you think should be tested now vs in the future?
  • the cargo-make and bash implementations run strip on the release binary on Linux (can't run on OS X since only gnu strip can read the cross-compiled binary), whereas cargo libcnb package does not. This is likely fine to skip for now, given that Cargo is about to stabilise support for stripping via Cargo.toml (Stabilize the strip profile option, now that rustc has stable -C strip rust-lang/cargo#10088; due on stable 2022-01-13), so likely not worth us implementing in the meantime, but mentioning here for completeness.
  • I presume we may want to publish compiled binaries for at least Linux to speed up CI - not something to worry about for this PR, but could you file a GitHub issue for this?

For the comments left below, feel free to defer (via filing a GitHub issue) or wontfix as you feel appropriate. I just wanted to mention everything I spotted up-front rather than try and guess what should be a GitHub issue vs not.

Marking requested changes since (a) I still have assemble_buildpack_tarball to review, (b) I'd like a glance at the rest post changes (though I don't imagine any issues).

README.md Outdated Show resolved Hide resolved
libcnb-cargo/src/main.rs Outdated Show resolved Hide resolved
libcnb-cargo/src/main.rs Outdated Show resolved Hide resolved
libcnb-cargo/src/main.rs Outdated Show resolved Hide resolved
libcnb-cargo/src/main.rs Outdated Show resolved Hide resolved
libcnb-cargo/src/cross_compile.rs Outdated Show resolved Hide resolved
libcnb-cargo/src/cross_compile.rs Outdated Show resolved Hide resolved
libcnb-cargo/src/cross_compile.rs Outdated Show resolved Hide resolved
libcnb-cargo/Cargo.toml Outdated Show resolved Hide resolved
libcnb-cargo/src/cross_compile.rs Outdated Show resolved Hide resolved
Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com>
@schneems
Copy link
Contributor

schneems commented Dec 1, 2021

We can surely add this to the README. If you ask me, it's not worth adding though. We're talking about one of the most basic Cargo commands to run a binary and end-users are supposed to install the binary with cargo install anyway.

I was referring to having it for local dev debugging/execution. I trust my tests and all, but sometimes I just want to exercise things manually.

That command you've provided doesn't work in the libcnb dev directory:

⛄️ 3.0.3 🚀 /Users/rschneeman/Documents/projects/work/libcnb.rs (malax/cargo)
$ cargo run -- libcnb --help
error: `cargo run` could not determine which binary to run. Use the `--bin` option to specify a binary, or the `default-run` manifest key.
available binaries: cargo-libcnb, example-01-basics, example-02-ruby-sample
⛄️ 3.0.3 🚀 /Users/rschneeman/Documents/projects/work/libcnb.rs (malax/cargo)
$ cargo build
    Finished dev [unoptimized + debuginfo] target(s) in 0.44s
⛄️ 3.0.3 🚀 /Users/rschneeman/Documents/projects/work/libcnb.rs (malax/cargo)
$ cargo run -- libcnb --help
error: `cargo run` could not determine which binary to run. Use the `--bin` option to specify a binary, or the `default-run` manifest key.
available binaries: cargo-libcnb, example-01-basics, example-02-ruby-sample

@edmorley
Copy link
Member

edmorley commented Dec 1, 2021

@schneems That's because this repository is using the Cargo workspaces feature (there are multiple crates here).

The command to use is still cargo run (you should never need to manually build and run the binary), but it needs to be told which crate to run. The error message you pasted mentioned how to do this, and listed the binary names available.

Using the method from the error message, gives:

cargo run --bin cargo-libcnb

In addition, these alternatives work too:
cargo run --package libcnb-cargo

cd cargo-libcnb && cargo run (I'm guessing this is the approach Manuel showed)

These alternatives also work for any other cargo command, for example when you want to run cargo test against a single package (to save running all tests):
cargo test -p libcnb-data (-p is equivalent to --package)
cd libcnb-data && cargo test

There is more on this here:
https://doc.rust-lang.org/cargo/reference/workspaces.html
https://doc.rust-lang.org/cargo/commands/cargo-run.html#package-selection

The first chapter of the Rust book also covers using cargo run instead of cargo build:
https://doc.rust-lang.org/book/ch01-03-hello-cargo.html#building-and-running-a-cargo-project

@Malax Malax force-pushed the malax/cargo branch 2 times, most recently from 1085ea5 to 31ddae8 Compare December 2, 2021 09:34
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! :-)

@Malax
Copy link
Member Author

Malax commented Dec 2, 2021

@schneems @hone: @edmorley has approved, but since you had input before I'll not merge yet.

@hone
Copy link
Member

hone commented Dec 2, 2021

@Malax this is really a net new project/crate. With my other PR and some other suggestions I feel more blocked as it stands now without it being merged. Let's get this in and open PRs against this existing code base IMO.

@Malax
Copy link
Member Author

Malax commented Dec 2, 2021

Let's get this in and open PRs against this existing code base IMO.

Exactly the approach I was thinking too:

I consider this an MVP, there are many improvements that could be made and it could have more features. We'll implement these features and fixes along the way.

Thanks for the approve!

@schneems
Copy link
Contributor

schneems commented Dec 2, 2021

Using the method from the error message, gives:

I tried running as:

$ cargo run --bin cargo-libcnb --help

It needs:

$ cargo run --bin cargo-libcnb -- --help

Which makes sense. But it's still nice to not make people guess/hunt. My comment is less about me being able to figure it out, and more about being in the mind space that whoever is dev-ing will be tired/forgetful/new/whatever to one or more of those concepts.

Where I'm coming from: I appreciate that this might be common expected knowledge from rust devs. In Ruby where every project ever has a Gemfile and bundler, every gem library still explains to people how to bundle install. For 99% of people, it's duplicate info. For that 1%, it's exactly what they need.

I'm in the mind space where if I have to look something up once, it's almost always worth asking "why", then retracing my steps to finding somewhere to make the next time easier (if not eliminating the need altogether). I wanted to run the CLI, couldn't immediately figure it out...so I wanted to document it. Which has the added bonus (for me) of a knowledge share showing more canonical/less-hacky ways to get to it.

Copy link
Contributor

@schneems schneems left a comment

Choose a reason for hiding this comment

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

🚀⛴🛳🚢🛸

@Malax Malax merged commit 47f00cc into main Dec 3, 2021
@Malax Malax deleted the malax/cargo branch December 3, 2021 08:50
edmorley added a commit that referenced this pull request Dec 6, 2021
Since:
- The `target/` entry was causing leftover nested target dire ctories
  to be hidden and not cleaned up.
- The `cargo-target/` entry is redundant as of #199.
edmorley added a commit that referenced this pull request Dec 7, 2021
Since:
- The `target/` entry was causing leftover nested target dire ctories
  to be hidden and not cleaned up.
- The `cargo-target/` entry is redundant as of #199.
@edmorley edmorley added this to the 0.4.0 milestone Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants