Skip to content

ci: Add missing docker.io prefix for native tasks to CI_IMAGE_NAME_TAG #28330

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
Merged
fanquake merged 1 commit into from
Aug 24, 2023
Merged

ci: Add missing docker.io prefix for native tasks to CI_IMAGE_NAME_TAG #28330

fanquake merged 1 commit into from
Aug 24, 2023

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Aug 23, 2023

Currently, the CI system may pick the wrong (non-native) architecture due to the missing prefix.

For example, assuming the CI_IMAGE_NAME_TAG is debian:bookworm and the user has previously pulled an s390x image:

$ podman run --rm 'docker.io/s390x/debian:bookworm' dpkg --print-architecture
exec /usr/bin/dpkg: exec format error

Now, debian:bookworm will refer to the same image:

$ podman run --rm 'debian:bookworm' dpkg --print-architecture
exec /usr/bin/dpkg: exec format error

However, docker.io/debian:bookworm works fine:

 $ podman run --rm 'docker.io/debian:bookworm' dpkg --print-architecture
arm64

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 23, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #28211 (Bump python minimum version to 3.9 by MarcoFalke)
  • #28210 (build: Bump minimum supported Clang to clang-13 by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot added the Tests label Aug 23, 2023
@hebasto
Copy link
Member

hebasto commented Aug 23, 2023

Does it make sense to be explicit about an architecture: docker.io/amd64/*?

@maflcko
Copy link
Member Author

maflcko commented Aug 23, 2023

Does it make sense to be explicit about an architecture: docker.io/amd64/*?

Why? and How? The architecture is native, not amd64.

@fanquake
Copy link
Member

fanquake commented Aug 23, 2023

Does it make sense to be explicit about an architecture: docker.io/amd64/*?

Wouldn't that just defeat the point of this change?

@hebasto
Copy link
Member

hebasto commented Aug 23, 2023 •
edited
Loading

Before #21652, it was obvious that the fuzzers task is executed on a x86_64 platform. What is the way now to figure out the platform which runs this test?

@maflcko
Copy link
Member Author

maflcko commented Aug 23, 2023

#21652 should be unrelated to any CI changes, because it didn't touch the ./ci/ folder, only the cirrus yaml file.

To find out the architecture, you can look at the logs.

For example, the depends build directory has the HOST included. Or the build directory:

Making install in doc/man
make[1]: Entering directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/doc/man'
make[2]: Entering directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/doc/man'
make[2]: Nothing to be done for 'install-exec-am'.

@hebasto
Copy link
Member

hebasto commented Aug 23, 2023

#21652 should be unrelated to any CI changes, because it didn't touch the ./ci/ folder, only the cirrus yaml file.

It is related because now it is possible to change worker architectures.

To find out the architecture, you can look at the logs.

For example, the depends build directory has the HOST included. Or the build directory:

Making install in doc/man
make[1]: Entering directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/doc/man'
make[2]: Entering directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/doc/man'
make[2]: Nothing to be done for 'install-exec-am'.

Another run might use a worker with arm64 architecture (potentially), no?

@fanquake
Copy link
Member

fanquake commented Aug 23, 2023

It is related because now it is possible to change worker architectures.

The CI system was already being used on x86_64 and aarch64 long before that PR was merged. Hopefully we'll even have a third architecture soon.

@hebasto
Copy link
Member

hebasto commented Aug 23, 2023

It is related because now it is possible to change worker architectures.

The CI system was already being used on x86_64 and aarch64 long before that PR was merged. Hopefully we'll even have a third architecture soon.

I mean, non-intentional change, for example, by an error.

@maflcko
Copy link
Member Author

maflcko commented Aug 23, 2023

Another run might use a worker with arm64 architecture (potentially), no?

Correct, but then that seems unrelated to this pull request here.

This was referenced Aug 24, 2023
Merged
Merged
@maflcko maflcko added this to the 26.0 milestone Aug 24, 2023
@maflcko
Copy link
Member Author

maflcko commented Aug 24, 2023

To clarify, this pull request fixes a bug that exists since CI_IMAGE_NAME_TAG was introduced.

hebasto
hebasto approved these changes Aug 24, 2023
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ACK fab7f5c.

FWIW, on my machines the following strings work:

  • x86_64 hardware: 'docker.io/amd64/debian:bookworm'
  • arm64 hardware: 'docker.io/arm64v8/debian:bookworm'

@fanquake fanquake merged commit 27101d0 into bitcoin:master Aug 24, 2023
@maflcko maflcko deleted the 2308-ci-docker-io- branch August 24, 2023 12:22
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 8, 2023
@fanquake
Merge bitcoin#28330: ci: Add missing docker.io prefix for native task…
1448843 
…s to CI_IMAGE_NAME_TAG
@bitcoin bitcoin locked and limited conversation to collaborators Aug 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

26.0

Development

Successfully merging this pull request may close these issues.

4 participants

@maflcko @DrahtBot @hebasto @fanquake