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(libindex): temp files on non-linux #1140

Merged
merged 1 commit into from
Nov 17, 2023
Merged

Conversation

RTann
Copy link
Contributor

@RTann RTann commented Nov 7, 2023

O_TMPFILE does not exist on non-linux unix-based OSes, so I was unable to build ClairCore on my macOS (Darwin) machine. Similarly, I don't think it exists on Windows, but FILE_FLAG_DELETE_ON_CLOSE does, which sounds to be about the same.

The implemented solution for Darwin runs into the issue O_TMPFILE aimed to fix: ClairCore crashing can still leave files on the system. Since this was previous behavior, I don't see this as a regression, necessarily.

@RTann RTann requested a review from a team as a code owner November 7, 2023 23:19
@RTann RTann requested review from hdonnay and removed request for a team November 7, 2023 23:19
@RTann RTann requested a review from crozzy November 7, 2023 23:19
@RTann RTann force-pushed the ross/tempFile-fetcher branch 3 times, most recently from 9effdf2 to 28bb1a3 Compare November 8, 2023 00:18
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (1e1b17d) 52.11% compared to head (72cdae3) 52.04%.

Files Patch % Lines
libindex/tempfile_linux.go 66.66% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1140      +/-   ##
==========================================
- Coverage   52.11%   52.04%   -0.07%     
==========================================
  Files         218      219       +1     
  Lines       16724    16734      +10     
==========================================
- Hits         8715     8710       -5     
- Misses       7208     7219      +11     
- Partials      801      805       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BradLugo
Copy link
Contributor

Would it be worth adding macOS and Windows builds to CI? e.g., GOOS=darwin go build ...

@hdonnay
Copy link
Member

hdonnay commented Nov 13, 2023

It seems fine as long as they don't slow down the CI.
Might be better off as a nightly CI.
None of the server components are really supported on windows or darwin, they just need to build for Clair's cli.

@RTann
Copy link
Contributor Author

RTann commented Nov 15, 2023

None of the server components are really supported on windows or darwin, they just need to build for Clair's cli.

I develop on a mac and can't compile at the moment 😢

Copy link
Member

@hdonnay hdonnay left a comment

Choose a reason for hiding this comment

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

I think (*tempfile).read would be better named as Reopen or Clone. (*ref).Val should just return a *os.File instead of a *tempFile, because none of the file handles pulled out of the rc should need special handling.

@RTann
Copy link
Contributor Author

RTann commented Nov 17, 2023

(*ref).Val should just return a *os.File instead of a *tempFile, because none of the file handles pulled out of the rc should need special handling.

We need to make sure the file is removed on unix, so we need to call (*tempFile).Close instead of (*os.File).Close

@RTann
Copy link
Contributor Author

RTann commented Nov 17, 2023

(*ref).Val should just return a *os.File instead of a *tempFile, because none of the file handles pulled out of the rc should need special handling.

We need to make sure the file is removed on unix, so we need to call (*tempFile).Close instead of (*os.File).Close

Oh I see now. I'll change it

@RTann RTann force-pushed the ross/tempFile-fetcher branch 3 times, most recently from 1a9cedc to 3118e57 Compare November 17, 2023 01:56
@RTann RTann requested a review from hdonnay November 17, 2023 18:19
Copy link
Member

@hdonnay hdonnay left a comment

Choose a reason for hiding this comment

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

I think one last issue and it's good to go

libindex/tempfile_unix.go Outdated Show resolved Hide resolved
Signed-off-by: RTann <rtannenb@redhat.com>
@RTann
Copy link
Contributor Author

RTann commented Nov 17, 2023

rebased. will merge with CI

@RTann RTann merged commit 558f07c into quay:main Nov 17, 2023
8 checks passed
@RTann RTann deleted the ross/tempFile-fetcher branch November 17, 2023 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants