-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix RPMs using a too-new version of glibc #11008
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ package main | |
|
||
import ( | ||
"fmt" | ||
"strings" | ||
) | ||
|
||
const ( | ||
|
@@ -175,7 +176,11 @@ func tagPipelines() []pipeline { | |
|
||
// RPM/DEB package builds | ||
for _, packageType := range []string{rpmPackage, debPackage} { | ||
ps = append(ps, tagPackagePipeline(packageType, buildType{os: "linux", arch: arch, fips: fips})) | ||
bt := buildType{os: "linux", arch: arch, fips: fips} | ||
if packageType == "rpm" && arch == "amd64" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a comment here explaining why we're doing this. Similar to the one in build-package.sh. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should I add one here or at #R291 ? This simply sets the field value (which could be used anywhere) while R291-R295 does something with it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think here is appropriate since we want to explain why we're setting the centos7 flag when building x8664 rpms. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Come to think of it I don't know that we should be checking the architecture here. The original fix only covered amd64, but I don't see any reason why the issue wouldn't affect i386 and ARM as well. That being said, I don't have an easy way to test this. Do you have any thoughts here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. I imagine those are less commonly used than amd64 but we'd probably fix them too. Let's do amd64 first so we can fix the most common use case and then follow up with ARM/32-bit (don't know if anyone's actually using it TBH). Can you use AWS account to spin up proper CentOS 7 boxes for testing? They should have ARM boxes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, looking at our downloads page, it doesn't look like we actually provide CentOS 7 compatible ARM binaries at all currently. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can test 64 bit ARM on AWS pretty easily, but as far as I am aware there are not any 32 bit CentOS 7 or RHEL AMIs available on AWS. This makes it non-trivial to test for 32 bit issues. That being said, if the issue persists with 64 bit ARM it probably affects i386 and ARMv7 as well. |
||
bt.centos7 = true | ||
} | ||
ps = append(ps, tagPackagePipeline(packageType, bt)) | ||
} | ||
} | ||
} | ||
|
@@ -283,6 +288,11 @@ func tagDownloadArtifactCommands(b buildType) []string { | |
} | ||
artifactOSS := true | ||
artifactType := fmt.Sprintf("%s-%s", b.os, b.arch) | ||
|
||
if b.centos7 { | ||
artifactType += "-centos7" | ||
} | ||
|
||
if b.fips { | ||
artifactType += "-fips" | ||
artifactOSS = false | ||
|
@@ -362,8 +372,19 @@ func tagPackagePipeline(packageType string, b buildType) pipeline { | |
} | ||
|
||
dependentPipeline := fmt.Sprintf("build-%s-%s", b.os, b.arch) | ||
|
||
if b.centos7 { | ||
dependentPipeline += "-centos7" | ||
} | ||
|
||
apkPackages := []string{"bash", "curl", "gzip", "make", "tar"} | ||
if packageType == rpmPackage { | ||
// Required by `make rpm` | ||
apkPackages = append(apkPackages, "go") | ||
} | ||
|
||
packageBuildCommands := []string{ | ||
`apk add --no-cache bash curl gzip make tar`, | ||
fmt.Sprintf("apk add --no-cache %s", strings.Join(apkPackages, " ")), | ||
`cd /go/src/github.com/gravitational/teleport`, | ||
`export VERSION=$(cat /go/.version.txt)`, | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.