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

chore: Reduce devcontainer size #14280

Merged
merged 1 commit into from
Oct 26, 2022

Conversation

jheidbrink
Copy link
Contributor

Summary

Reduces the devcontainer size from 6 GB to 4 GB.

The Dockerfile meant to remove several downloaded source folders, but those commands assumed a WORKDIR of /. With the WORKDIR set to /usr/local, those rm command had no effect. This was undetected due to rm being called with the -f option. This PR fixes the paths, and also uses the --interactive=never flag instead of the -f flag, so that regressions are detected by failing rm commands.

This PR also removes /root/.cache/go-build folders.

This PR partly removes llvm-10, thus does a small part of #14279.

Test Plan

In the magma folder:

docker build -f .devcontainer/Dockerfile .
# look up the image id
docker run -it --mount type=bind,src=$MAGMA_ROOT,dst=/workspaces/magma <container_image_id> /bin/bash
# inside the container:
bazel build //...

Signed-off-by: Jan Heidbrink <jan.heidbrink@tngtech.com>
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines. label Oct 26, 2022
@github-actions
Copy link
Contributor

Thanks for opening a PR! 💯

A couple initial guidelines

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@github-actions
Copy link
Contributor

dp-workflow

0 tests   0 ✔️  0s ⏱️
0 suites  0 💤
0 files    0

Results for commit c040376.

@@ -52,7 +52,7 @@ RUN echo "Install general purpose packages" && \
gdb \
lcov \
libclang-11-dev \
lldb \
lldb-11 \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lldb means lldb-10. Given that all the other packages here depend on llvm-11 and this one on llvm-10, I assumed without testing that we can use lldb-11 here.

cd / && \
rm -rf grpc
cd ../../.. && \
rm --recursive --interactive=never grpc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In contrast to -f, --interactive=never fails when the directory doesn't exist, so that we detect when this command isn't doing what we expect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would one want to create an alias for it, if it is used multiple times throughout the script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say it's simple enough that it's ok to repeat it. Repeating it is slightly easier to read as you don't have to look up the alias.

cmake -DgRPC_INSTALL=ON -DgRPC_BUILD_TESTS=OFF -DBUILD_SHARED_LIBS=ON ../.. && \
make -j"$(nproc)" && \
make install && \
cd / && \
rm -rf grpc
cd ../../.. && \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This solution has the disadvantage that we have to count how many directories we descended. I discussed with @nstng to use pushd/popd, or running the cd in a subshell so that the parent shell never changes directories. We figured that this version is the most widely understood one and thus kept it despite the disadvantage of having to count.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about introducing a variable for the workdir? I.e.

ARG basedir=/usr/local
cd where/ever/you/want
...
cd ${basedir}

This would avoid the counting and be pretty safe to always return to the desired directory.
Similar to https://docs.docker.com/engine/reference/builder/#workdir.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer the current solution as it is a bit less stateful as it doesn't care what the working directory is set to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another thought: How about the "poor mans pushd/popd": cd -? Might that be a valid option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That wouldn't work for prometheus-cpp below, as that uses multiple cd commands. I'm inclined to not put more thought into this, but accept it as good enough.

@Neudrino Neudrino self-requested a review October 26, 2022 13:57
Copy link
Contributor

@Neudrino Neudrino left a comment

Choose a reason for hiding this comment

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

The Dockerfile meant to remove several downloaded source folders, but those commands assumed a WORKDIR of /.

I wonder if the workdir is set correctly for all parts? Form the code it looks more as if the workdir should only be set to /usr/local for the Go installation but be reset to the original (/) afterwards?

Copy link
Contributor

@Neudrino Neudrino left a comment

Choose a reason for hiding this comment

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

Rebuild the container locally from branch. Builds and starts fine. ✔️

@jheidbrink
Copy link
Contributor Author

The Dockerfile meant to remove several downloaded source folders, but those commands assumed a WORKDIR of /.

I wonder if the workdir is set correctly for all parts? Form the code it looks more as if the workdir should only be set to /usr/local for the Go installation but be reset to the original (/) afterwards?

Hm, I think steps in a dockerfile should avoid relying on a specific WORKDIR (including /), and if they do they should have an explicit WORKDIR declaration close by.

@jheidbrink jheidbrink merged commit ece8b6f into magma:master Oct 26, 2022
@jheidbrink jheidbrink deleted the reduce_devcontainer_size branch October 26, 2022 14:59
@jheidbrink jheidbrink self-assigned this Nov 1, 2022
lucasgonze pushed a commit to lucasgonze/magma that referenced this pull request Feb 29, 2024
Signed-off-by: Jan Heidbrink <jan.heidbrink@tngtech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants