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

fix(pkg,internal): multiple fixes related to cos and driver build. #383

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Jan 19, 2024

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area library

What this PR does / why we need it:

See self review ;)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@@ -58,7 +58,7 @@ func ExtractTarGz(gzipStream io.Reader, destDir string, stripPathComponents int)
switch header.Typeflag {
case tar.TypeDir:
d := filepath.Join(destDir, strippedName)
if err = os.Mkdir(filepath.Clean(d), 0o750); err != nil {
if err = os.MkdirAll(filepath.Clean(d), 0o750); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Avoid failing when creating targz root folder (ie: when strippedName is either empty or /).

@@ -76,8 +76,23 @@ func ExtractTarGz(gzipStream io.Reader, destDir string, stripPathComponents int)
if err = outFile.Close(); err != nil {
return nil, err
}
if err = os.Chmod(filepath.Clean(f), header.FileInfo().Mode()); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Properly set correct mode on created files.

files = append(files, f)

case tar.TypeLink, tar.TypeSymlink:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Manage also soft links and hard links.

@@ -76,15 +77,15 @@ func (c *cos) customizeBuild(ctx context.Context,

currKernelDir := env[kernelDirEnv]

cosKernelDir := currKernelDir + "usr/src/"
cosKernelDir := filepath.Join(currKernelDir, "usr", "src")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small fixes.

if strings.HasSuffix(kernelConfigPath, ".gz") {
src = tar.NewReader(f)
src, err = gzip.NewReader(f)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need a tar reader; we need a gzip reader.

} else {
src = f
}
defer src.Close()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Properly close src.

@@ -87,6 +87,7 @@ func (b *bpf) Build(ctx context.Context,
makeCmdArgs := fmt.Sprintf(`make -C %q`, filepath.Clean(srcPath))
makeCmd := exec.CommandContext(ctx, "bash", "-c", makeCmdArgs) //nolint:gosec // false positive
// Append requested env variables to the command env
makeCmd.Env = os.Environ()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If env gets set in the command, like we were doing, os.Environ was not set.

@FedeDP
Copy link
Contributor Author

FedeDP commented Jan 19, 2024

/cc @LucaGuerra

@poiana poiana requested a review from LucaGuerra January 19, 2024 13:35
@FedeDP
Copy link
Contributor Author

FedeDP commented Jan 19, 2024

/milestone v0.7.1

@poiana poiana added this to the v0.7.1 milestone Jan 19, 2024
internal/utils/extract.go Fixed Show fixed Hide fixed
@LucaGuerra
Copy link
Contributor

Thanks @FedeDP ! I have tested these modifications and they worked for me, downloading the right headers for COS.

internal/utils/extract.go Fixed Show fixed Hide fixed
@poiana poiana added size/L and removed size/XL labels Jan 22, 2024
var files []string

uncompressedStream, err := gzip.NewReader(gzipStream)
func ExtractTarGz(ctx context.Context, gzipStream io.Reader, destDir string, stripPathComponents int) ([]string, error) {
Copy link
Contributor Author

@FedeDP FedeDP Jan 22, 2024

Choose a reason for hiding this comment

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

The new signature takes the context too!

internal/utils/extract.go Fixed Show fixed Hide fixed
@poiana
Copy link
Contributor

poiana commented Jan 23, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, leogr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana merged commit ca1be8a into main Jan 23, 2024
14 checks passed
@poiana poiana deleted the fix/cos_issues branch January 23, 2024 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants