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

Also build for CentOS 9. #270

Closed
wants to merge 1 commit into from
Closed

Also build for CentOS 9. #270

wants to merge 1 commit into from

Conversation

Romain-Geissler-1A
Copy link
Contributor

@Romain-Geissler-1A Romain-Geissler-1A commented Mar 7, 2022 •
edited

Note: I have removed the btrfs support unconditionally as IMO the old condition "%if 1%{!?el8:1}" was always true, on every distros.

This includes changes inspired from #231.

@Romain-Geissler-1A Romain-Geissler-1A mentioned this pull request Mar 7, 2022
Merged
thaJeztah
thaJeztah reviewed Mar 8, 2022
View reviewed changes
dockerfiles/rpm.dockerfile Outdated
Comment on lines 120 to 121
# Don't try to run runc on RHEL/CentOS as we don't package it anymore.
RUN if [ "${BASE}" != "rhel" ] && [ "${BASE}" != "centos" ]; then runc --version; fi
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather keep this change for a separate PR (sorry haven't been able to spend time to respond on the linked ticket); this definitely needs a wider discussion (and not something we should change overnight).

I'm ok with adding the stream9 to the list in CI though (note that this Jenkinsfile is used for CI, but the actual matrix for which we built is in our internal release pipeline, so it may not be included (yet) on the next build)

thaJeztah
thaJeztah reviewed Mar 8, 2022
View reviewed changes
dockerfiles/rpm.dockerfile Outdated
Comment on lines 120 to 121
# Don't try to run runc on RHEL/CentOS as we don't package it anymore.
RUN if [ "${BASE}" != "rhel" ] && [ "${BASE}" != "centos" ]; then runc --version; fi
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather keep this change for a separate PR (sorry haven't been able to spend time to respond on the linked ticket); this definitely needs a wider discussion (and not something we should change overnight).

I'm ok with adding the stream9 to the list in CI though (note that this Jenkinsfile is used for CI, but the actual matrix for which we built is in our internal release pipeline, so it may not be included (yet) on the next build)

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 have updated this pull request to focus just on CentOS 9. I have split it in two commits, the second commit being now tracked in #272.

@Romain-Geissler-1A Romain-Geissler-1A mentioned this pull request Mar 9, 2022
Open
@Romain-Geissler-1A
Copy link
Contributor Author

Note: it looks like Ci is failing, but external contributor can't see the logs (Access Denied Romain-Geissler-1A is missing the Global/Read permission), so I don't know why it's failing and can't fix it myself. Locally make quay.io/centos/centos:stream9 works for me.

@Romain-Geissler-1A
Copy link
Contributor Author

I saw you have release docker 20.10.13 with Ubuntu Jammy this week, would it be possible to also tackle CentOS 9 ?

@Romain-Geissler-1A
Copy link
Contributor Author

Note for maintainers: CI build is failing, but open source contributor can't see the output of Jenkins, so I would need you to tell me what's wrong in order to fix the build.

@Romain-Geissler-1A
Copy link
Contributor Author

Ping (I don't know why the build fails)

@Romain-Geissler-1A
Copy link
Contributor Author

Ping

1 similar comment
@Romain-Geissler-1A
Copy link
Contributor Author

Ping

@rumpl
Copy link
Member

rumpl commented Apr 29, 2022

@Romain-Geissler-1A I looked at the failure, it's 'go get' is no longer supported outside a module., this should be fixed if you rebase your branch, see this commit for more info

@Romain-Geissler-1A
Also build for CentOS 9.
ee47082 
Signed-off-by: Romain Geissler <romain.geissler@amadeus.com>
@rumpl
Copy link
Member

rumpl commented Apr 29, 2022

On to the next error:

#13 19.02 Fetched 79.5 MB in 8s (9666 kB/s)

#13 19.02 E: Failed to fetch http://archive.raspbian.org/raspbian/pool/main/libh/libhtml-parser-perl/libhtml-parser-perl_3.75-1%2bb1_armhf.deb  Undetermined Error [IP: 93.93.128.191 80]

#13 19.02 E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?

Not sure if it's related, @thaJeztah any ideas?

@thaJeztah
Copy link
Member

I saw failures on docker/docker-ce-packaging#681 (comment) as well; looks like it's now green, so I'm guessing it was some failures with those package servers (or just general networking issues); let me give Jenkins another kick

thaJeztah
thaJeztah reviewed Apr 29, 2022
View reviewed changes
rpm/containerd.spec
Comment on lines -72 to -79
# Should only return true if `el8` (rhel8) is NOT defined
%if 0%{!?el8:1}
%if 0%{?suse_version}
BuildRequires: libbtrfs-devel
%else
BuildRequires: btrfs-progs-devel
%endif
%endif
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is correct; I think the intent here was to

  • include libbtrfs-devel on centos / rhel 7 (which still had btrfs?), and Fedora (?)
  • include btrfs-progs-devel on SUSE flavours (which default to using BTRFS as filesystem)

If I'm reading the current changes correctly, we're now excluding btrfs on all RPM variants (not just RHEL/CentOS > 7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the removal of all this related to btrfs is linked not to these "ifs", but to another one further down:

%if 1%{!?el8:1}
BUILDTAGS="${BUILDTAGS} no_btrfs"
%endif

This "if" expression can only expand to a "true" value, so my understanding is that right now, we always build with flag "no_btrfs", no matter which distro is being used. Assuming that btrfs was never used, I also removed all dependency on btrfs build requires packages.

rpm/containerd.spec
# For some reason on rhel 8 if we "provide" runc then it makes this package unsearchable
%if 0%{!?el8:1}
# For some reason on rhel >= 8 if we "provide" runc then it makes this package unsearchable
%if 0%{?rhel} <= 7
Copy link
Member

Choose a reason for hiding this comment

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

This may also be excluding this rule on Fedora, (open)SUSE, Oracle etc (assuming those don't have rhel defined), so a larger change than only "exclude on RHEL >= 8

@Romain-Geissler-1A
Copy link
Contributor Author

Hi,

Are my replies to all concerns ok or I still need to change something ?

Note that RHEL 9 will be officially released this months, so people will start to need the CentOS 9 package ;)

Cheers,
Romain

@thaJeztah thaJeztah mentioned this pull request May 4, 2022
Merged
@thaJeztah
Copy link
Member

I worked on various permutations of these conditions, and opened #283, based on your commit; let me know if that looks good to you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thaJeztah thaJeztah thaJeztah left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Romain-Geissler-1A @rumpl @thaJeztah