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

docker: Use pre-built curl-impersonate library from github releases #3984

Merged
merged 1 commit into from
Feb 20, 2024
Merged

docker: Use pre-built curl-impersonate library from github releases #3984

merged 1 commit into from
Feb 20, 2024

Conversation

xduugu
Copy link
Contributor

@xduugu xduugu commented Feb 15, 2024

The docker image is only available for amd64 architecture and therefore cannot be used for arm images.

Fixes #3983

Dockerfile Outdated
&& mkdir -p /usr/local/lib/curl-impersonate \
&& tar xaf "$archive" -C /usr/local/lib/curl-impersonate --wildcards 'libcurl-impersonate-ff.so*' \
&& rm "$archive" \
&& apt-get purge --assume-yes curl
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
&& apt-get purge --assume-yes curl
&& apt-get purge --assume-yes curl \
&& rm -rf /var/lib/apt/lists/*

Clean Apt cache

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe consider moving all this section up to line 25 to avoid having to do an apt update again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, makes sense. I'll update the PR.

Dockerfile Outdated
&& sha512sum="d04b1eabe71f3af06aa1ce99b39a49c5e1d33b636acedcd9fad163bc58156af5c3eb3f75aa706f335515791f7b9c7a6c40ffdfa47430796483ecef929abd905d" \
; } \
|| { \
[ $(arch) = 'armv7l' ] \
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure: would this binary also work on arm64 (e.g. in the case of Raspberry Pi)?

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'm not sure what you mean. armv7l is arm32 / aarch32 and arm64 is aarch64. Actually, I run rss-bridge on a raspberry pi 4 and use the aarch64 library of curl-impersonate.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, aarch64 is already in the list. My question was whether some of those strings were too specific and could also work for other values. For instance (I am not sure): armv8l vs. aarch64

Copy link
Contributor

Choose a reason for hiding this comment

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

Might not be important for now, though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rss-bridge image is available for architectures:

  • linux/amd64 = x86_64
  • linux/arm/v7 = armv7l
  • linux/arm64 = aarch64

For new architectures, the build step will fail and an existing condition must be adjusted (if applicable) or a new condition must be added.

The docker image is only available for `amd64` architecture and therefore
cannot be used for arm images.

Fixes #3983
Comment on lines +56 to +57
ENV LD_PRELOAD /usr/local/lib/curl-impersonate/libcurl-impersonate-ff.so
ENV CURL_IMPERSONATE ff91esr
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe there should the be a fall-back case to not using CURL_IMPERSONATE if we have not found any pre-compiled version of it higher up for the current architecture.
And maybe together with an echo "No libcurl-impersonate found for current architecture $(arch)"

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 not make the Dockerfile more complex than needed. For now, all architectures for which the rss-bridge image is available (see #3984 (comment)), are covered.

For unsupported architectures, the docker build step will fail and no image is created. In this case, the Dockerfile needs to be adjusted to support the new architecture.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough

@dvikan
Copy link
Contributor

dvikan commented Feb 19, 2024

i really dont want to acquire more docker skills.

shall i merge this now?

@Alkarex
Copy link
Contributor

Alkarex commented Feb 19, 2024

shall i merge this now?

Checked to work on x86_64 (same than before this PR), so fine for me

Copy link
Contributor

@Alkarex Alkarex left a comment

Choose a reason for hiding this comment

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

Tested to work with x86_64

@dvikan dvikan merged commit 35f6e62 into RSS-Bridge:master Feb 20, 2024
7 checks passed
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.

docker: curl-impersonate library included in arm images is compiled for the wrong architecture (amd64/x86-64)
3 participants