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.

Sign up for GitHub

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

Jump to bottom

Add libgeos-dev for ARM components of multi-arch Beam SDK Python containers #28036

Merged
merged 9 commits into from
Aug 16, 2023
Merged

Add libgeos-dev for ARM components of multi-arch Beam SDK Python containers #28036

Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
88c4253
Add libgeos-dev for ARM
celeste-zeng Aug 16, 2023
97b6cb5
remove double command
celeste-zeng Aug 16, 2023
bd1eb36
Try format fix
celeste-zeng Aug 16, 2023
aed6053
fix format
celeste-zeng Aug 16, 2023
70d0da4
fix ==
celeste-zeng Aug 16, 2023
99d9ab4
revert changes to follow convention
celeste-zeng Aug 16, 2023
fb6b407
formatting
celeste-zeng Aug 16, 2023
fd7cc24
Fix package not found error.
celeste-zeng Aug 16, 2023
356bf3d
install the package regardless
celeste-zeng Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions sdks/python/container/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@ RUN \
# Remove pip cache.
rm -rf /root/.cache/pip

# Required for using Beam Python SDK on ARM machines.
RUN if [ "${TARGETARCH}" = "arm64" ] ; then \
apt-get install libgeos-dev ; \
Copy link
Contributor

@tvalentyn tvalentyn Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

i would suggest to move these statements to line 42 (keeping the statements inside one RUN command). note that apt temp files are cleared in line 44, and this command might require a prior call to apt update.

Also, did you run validatesconainerarm tests to verify this change ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, did you run validatesconainerarm tests to verify this change ?

you could post a link to GHA run on the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

overall, pr intent looks good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Tried to put it around line 42 earlier but I don't think the if condition can be inserted between different package names after calling apt-get install.
  • I can't run on this branch because it is just a fork of the apache/beam master not the original master branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I think we can install unconditionally to avoid extra complexity. I suspect it current version might fail without apt update.

fi

ENTRYPOINT ["/opt/apache/beam/boot"]

####
Expand Down
Loading