-
Notifications
You must be signed in to change notification settings - Fork 41
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
Makefile: refactors brupop-image
target
#418
Conversation
@@ -36,12 +36,13 @@ BUILDER_IMAGE = public.ecr.aws/bottlerocket/bottlerocket-sdk-$(BOTTLEROCKET_SDK_ | |||
|
|||
export CARGO_HOME = $(TOP)/.cargo | |||
|
|||
image: build brupop-image | |||
image: check-licenses brupop-image |
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.
Removes the duplicate build step here. No reason to build binaries locally when building the image does the same thing. Calls into the check-license
path to ensure licenses are good. This is the default target.
# Builds, Lints and Tests the Rust workspace | ||
build: fetch check-licenses | ||
# Builds, Lints, and Tests the Rust workspace locally | ||
build: check-licenses |
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.
check-licenses
already does fetch. So removed this duplicate work.
cargo test --locked | ||
cargo build --locked |
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.
Moved building to run after tests have passed.
|
||
brupop-image: fetch check-licenses | ||
# Builds only the brupop image. Useful target for CI/CD, releasing, etc. | ||
brupop-image: |
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.
brupop-image
no longer has a dependency on fetch or check-licenses since both depend on the cargo / rust toolchain being available. This ensures that we can build the brupop image in an environment that may not have those tools.
docker build $(DOCKER_BUILD_FLAGS) \ | ||
--build-arg UNAME_ARCH="$(UNAME_ARCH)" \ | ||
--build-arg BUILDER_IMAGE="$(BUILDER_IMAGE)" \ | ||
--tag "$(IMAGE_NAME)" \ | ||
--network none \ | ||
-f Dockerfile . | ||
|
||
dist: brupop-image | ||
dist: check-licenses brupop-image |
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.
This target should utilize the check-license / fetch path since it's creating a distribution (and we should ensure we have checked the licenses locally)
Ensures that building the brupop image works without needing to have the rust and cargo toolchain installed. This is useful for release and CI/CD where an environment may not have these tools installed. Signed-off-by: John McBride <jpmmcb@amazon.com>
996d1c5
to
fd8403f
Compare
ADD ./ /src/ | ||
|
||
# Ensure cargo dependencies are fetched and available in the Docker context | ||
RUN cargo fetch --locked --manifest-path /src/Cargo.toml |
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.
Fetches sources in docker image to ensure we have necessary upstream code to build locally. This is still dependent on the Cargo.lock, so nothing new should be pulled in here. Note: this requires network access
docker build $(DOCKER_BUILD_FLAGS) \ | ||
--build-arg UNAME_ARCH="$(UNAME_ARCH)" \ | ||
--build-arg BUILDER_IMAGE="$(BUILDER_IMAGE)" \ | ||
--tag "$(IMAGE_NAME)" \ | ||
--network none \ |
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.
Note: in order to fetch cargo sources from crates.io upstream in the image, we need to allow network access. But since this using the lockfile, this should be ok since typically, on the local machine, the sources would be fetched and mounted into the container.
Ensures that building the brupop image works without needing to have the rust and cargo toolchain installed. This is useful for release and CI/CD where an environment may not have these tools installed.
Issue number:
N/a - related to useful changes for releasing brupop
Description of changes:
See comments below for further explanation
Testing done:
Able to run:
make brupop-image
without cargo/rust toolchain installedmake
, the default target, which builds the image but also checks licenses, fetches sources locally, etcmake build
which fetches sources, checks licenses, but builds binaries locally (not in the docker environment or as part of an image)Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.