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 libcnb_runtime_build and libcnb_runtime_detect public #313

Closed
wants to merge 2 commits into from
Closed

Make libcnb_runtime_build and libcnb_runtime_detect public #313

wants to merge 2 commits into from

Conversation

nokome
Copy link
Contributor

@nokome nokome commented Feb 9, 2022

This exposes libcnb_runtime_build and libcnb_runtime_detect (and associated types) so that they can be used by other crates.

For my use case, instead of building separate executable for each buildpack, I would like to combine several buildpacks into an existing CLI executable and delegate to their detect and build methods. This change allows me to do that without rewriting alot of the code contained in these two functions e.g.

/// A local trait for buildpacks that extends `libcnb::Buildpack`
///
/// Why? To provide some additional introspection and the ability
/// to compile several buildpacks into a single binary and call
/// their `detect` and `build` methods.
pub trait BuildpackTrait: libcnb::Buildpack {
    ...

    /// Run the buildpack's `detect` method
    fn detect_with(&self, platform_dir: &Path, build_plan: &Path) -> Result<i32>
    where
        Self: Sized,
    {
        env::set_var("CNB_STACK_ID", CNB_STACK_ID);

        let buildpack_dir = self.ensure_dir()?;
        env::set_var("CNB_BUILDPACK_DIR", buildpack_dir);

        match libcnb_runtime_detect(
            self,
            DetectArgs {
                platform_dir_path: PathBuf::from(platform_dir),
                build_plan_path: PathBuf::from(build_plan),
            },
        ) {
            Ok(code) => Ok(code),
            Err(error) => bail!(
                "While running `detect` for buildpack `{}`: {}",
                Self::id(),
                error
            ),
        }
    }

This exposes `libcnb_runtime_build` and `libcnb_runtime_detect` (and associated types)
so that they can be used by other crates.
@Malax
Copy link
Member

Malax commented Feb 9, 2022

Hey @nokome 👋🏻

Thanks for your PR! In general, I can see us exposing libcnb_runtime_build and libcnb_runtime_detect.

Before we continue with this, I'd love to hear more about your use-case. Which benefits do you get by having multiple buildpacks in an existing binary and how are you packaging it? I never thought of doing that and maybe there is even more we can do to support your use-case if it's applicable to a broader audience?

@Malax Malax added the libcnb label Feb 9, 2022
@nokome
Copy link
Contributor Author

nokome commented Feb 10, 2022

Thanks for your interest @Malax. I'm putting together a PR for our project, https://github.com/stencila/stencila, that will introduce libcnb and outline the rationale for doing so. That is probably the best place to answer your questions about our use case. I can ping you when that's ready (hopefully in a day or two).

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 for the PR and the links to your project - it's very exciting to see other usages of libcnb! I'm curious, will your project interact with Pack CLI, or is it effectively an alternative implementation of the CNB lifecycle?

Re this PR, at first glance these changes seem great, I just wonder if it might be best to split them into a few PRs, since there are a few distinct changes here:

  1. Moving args handling + the process::exits up a level (to libcnb_runtime)
  2. Making libcnb_runtime_build, libcnb_runtime_detect, DetectArgs and BuildArgs public
  3. Improving support for non-Unix platforms in LayerEnv and libcnb_runtime

Previously, there were several places in the code where `cfg` attributes
where causing compile errors on Windows (when bypassing `libcnb_runtime`
and using `libcnb_runtime_detect` and `libcnb_runtime_build` directly).
This fixes those compile errors (checked using cross-compile on Linux).
@nokome
Copy link
Contributor Author

nokome commented Feb 28, 2022

Hi @edmorley. Sorry for taking so long to reply: I've had my head down with my other PR and somehow missed the notification.

I'm curious, will your project interact with Pack CLI, or is it effectively an alternative implementation of the CNB lifecycle?

Both. Our users are data scientists and researchers (who often don't have software dev experience) and we want to provide a transparent and seamless-as-possible journey from a local project to a Docker image. So the use case is perhaps a little different to the usual use case for CNB's.

We're using cargo libcnb package and creating a CNB builder to use with pack but we also have a "lifecycle-lite" so that users can use the stencila CLI to preview which buildpacks match against a project (and perhaps guide them to making it more reproducible) e.g.

> stencila buildpacks plan fixtures/projects/polyglot/small

Buildpack 'stencila/apt'

- apt_packages        Install apt packages for Ubuntu 'impish' (Aptfile)

Buildpack 'stencila/node'

- node                Install Node.js lts
- node_modules        Install Node.js packages into node_modules (package-lock.json)

Buildpack 'stencila/python'

- python              Install Python ^3.10 (pyproject.toml)
- poetry              Install Poetry (pyproject.toml)
- venv                Install Python packages into virtual environment using Poetry (pyproject.toml)

Buildpack 'stencila/r'

- r                   Install R 4.1.2 (renv.lock)
- renv                Install R packages into renv by restoring from lockfile (renv.lock)

Buildpack 'stencila/stencila'

- stencila            Install Stencila >=1.1.0

I just wonder if it might be best to split them into a few PRs, since there are a few distinct changes here:

I feel that 1 & 2 are closely connected so to avoid unnecessary extra work would prefer to combine those. Would it be OK to submit two PRs for each of my two commits:

@nokome
Copy link
Contributor Author

nokome commented Mar 2, 2022

I have split this into 3 separate PRS: #368, #369, and #370. Will close this one.

@nokome nokome closed this Mar 2, 2022
Malax referenced this pull request Mar 7, 2022
…b_runtime` (#369)

* Centralize arg handling, writing to stderr and process exit in `libcnb_runtime`

This change takes a "Functional Core - Imperitive Shell" type of approach by centralizing
argument handling, writing of error messages to stderr and calls to `process::exit` in
the main `libcnb_runtime` function.

This allows for other functions such as `libcnb_runtime_detect` and `libcnb_runtime_build`
to be re-used in other runtimes.

Note: This commit has been extracted out from https://github.com/Malax/libcnb.rs/pull/313 (which will be superseded by this an other PRs).

* Add DetectArgsParseError, BuildArgsParseError

Co-authored-by: Manuel Fuchs <[email protected]>
Malax referenced this pull request Mar 7, 2022
Note: This commit is made on top of https://github.com/Malax/libcnb.rs/pull/369

Note: This commit has been extracted out from https://github.com/Malax/libcnb.rs/pull/313 (which will be superseded by this and other PRs).
Malax referenced this pull request Mar 7, 2022
* Make `libcnb_runtime_build` etc public

Note: This commit is made on top of https://github.com/Malax/libcnb.rs/pull/369

Note: This commit has been extracted out from https://github.com/Malax/libcnb.rs/pull/313 (which will be superseded by this and other PRs).

* Hide newly exposed runtime types from docs

They're only exposed for advanced use-cases and should not be used by buildpack authors directly.

* Update CHANGELOG

* Expose all public runtime types

Co-authored-by: Nokome Bentley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants