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

Makefile: only use race detection when it's available #3518

Merged
merged 1 commit into from
Sep 17, 2021
Merged

Makefile: only use race detection when it's available #3518

merged 1 commit into from
Sep 17, 2021

Conversation

nalind
Copy link
Member

@nalind nalind commented Sep 14, 2021 •
edited

What type of PR is this?

/kind other

What this PR does / why we need it:

Check if go test supports the -race flag on the build platform, and if so, and only use it for unit tests if it's supported.

Fix a race in run_linux.go.

How to verify it

make test-unit should work correctly on platforms where the -race flag is not supported for builds. go help build will list which platforms your installed version of Go supports the flag for.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Marking this as not needing new tests, though it may end up requiring additional changes if the tests we already have start noticing things that need to be fixed.

Does this PR introduce a user-facing change?

None

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 14, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nalind

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

@rhatdan
Copy link
Member

rhatdan commented Sep 14, 2021

LGTM

flouthoc
flouthoc reviewed Sep 14, 2021
View reviewed changes
Copy link
Collaborator

@flouthoc flouthoc left a comment •
edited

Choose a reason for hiding this comment

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

Stdout spits logs for downloading go.mods maybe redirect stdout to /dev/null and error to /stdout

Edit: its line no 13 of Makefile.

@nalind
Copy link
Member Author

nalind commented Sep 14, 2021

Ah, right, the order matters. Swapped them.

@TomSweeneyRedHat
Copy link
Member

LGTM

@nalind nalind closed this Sep 15, 2021
@nalind nalind reopened this Sep 15, 2021
@nalind
Copy link
Member Author

nalind commented Sep 15, 2021

Taking the -race flag off of the main buildah binary. The performance hit of building it with race detection enabled is, as promised, rather noticeable.

@nalind
Copy link
Member Author

nalind commented Sep 15, 2021

And likewise, -race makes our conformance tests take quite a bit longer, so dropping that for now rather than attempting to increase the timeout.

@rhatdan
Copy link
Member

rhatdan commented Sep 15, 2021

What about the copy command?

@nalind
Copy link
Member Author

nalind commented Sep 15, 2021

I'm currently hoping it doesn't slow the integration tests down enough to cause CI jobs to time out, but we'll see.

@nalind
Makefile: turn on race detection whenever it's available
a314d2a 
Check if `go test` supports the -race flag on the build platform, and if
so, use it for unit tests instead of just assuming that it's always
available.

Use sync/atomic to safely use a uint32 instead of a bool to keep track
of whether or not the process we started for RUN has stopped.

[NO NEW TESTS NEEDED]

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
@nalind nalind changed the title Makefile: turn on race detection whenever it's available Makefile: only use race detection when it's available Sep 16, 2021
@nalind
Copy link
Member Author

nalind commented Sep 16, 2021

Switched to using sync/atomic instead of a mutex for fixing the stopped race.

@rhatdan
Copy link
Member

rhatdan commented Sep 16, 2021

/lgtm
/hold

@openshift-ci openshift-ci bot added the lgtm label Sep 16, 2021
@TomSweeneyRedHat
Copy link
Member

LGTM

@rhatdan
Copy link
Member

rhatdan commented Sep 17, 2021

/hold cancel

@openshift-merge-robot openshift-merge-robot merged commit 7faf1e0 into containers:main Sep 17, 2021
@nalind nalind deleted the always-race branch September 17, 2021 12:43
@nalind nalind mentioned this pull request Sep 23, 2021
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@flouthoc flouthoc flouthoc left review comments

Assignees

@rhatdan rhatdan

Labels
approved kind/other lgtm locked - please file new issue/PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nalind @rhatdan @TomSweeneyRedHat @flouthoc @openshift-merge-robot