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

Enable race detector for CI #1441

Merged
merged 13 commits into from Aug 26, 2021
Merged

Enable race detector for CI #1441

merged 13 commits into from Aug 26, 2021

Conversation

typeless
Copy link
Contributor

@typeless typeless commented Apr 4, 2017 •
edited by 6543

close #1430

@tboerger
Copy link
Member

tboerger commented Apr 4, 2017

I thought more about a flag like ENABLE_RACE or something like that

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 4, 2017
@typeless
Copy link
Contributor Author

typeless commented Apr 4, 2017

@tboerger I am okay with either way. We can have the final decision discussed in #1430. I'll change the PRs accordingly.

@strk
Copy link
Member

strk commented Apr 4, 2017

LGTM

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 4, 2017
@typeless
Copy link
Contributor Author

typeless commented Apr 4, 2017 •
edited

I have checked 'Allow edits from maintainers.'. You know 😄
Otherwise, we need a follow-up PR to update the signature.

Edit: maybe you guys with the maintainer key could upload a new PR with the updated signature. This change is only a one-liner, that would probably easier for you.

@lunny lunny added this to the 1.2.0 milestone Apr 4, 2017
@appleboy
Copy link
Member

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 11, 2017
@strk
Copy link
Member

strk commented Apr 18, 2017

@appleboy can you help with the signature ? @tboerger mentioned you as his preferred successor as he's stepping down (mentioned on Gitter)

@strk
Copy link
Member

strk commented Apr 19, 2017

It is the coverage step that fails, due to:

Unable to authenticate user. Error fetching user. GET https://api.github.com/user: 401 Bad credentials []

Is that what the signature is for ?
Was there a plan to drop the signature requirement @appleboy ?

@appleboy
Copy link
Member

@strk I don't have permission to update the sig file. @lunny can help this.

@lunny lunny force-pushed the enable-race-detector-in-ci branch from ab4224e to 57a07cd Compare April 19, 2017 07:54
@strk
Copy link
Member

strk commented Apr 30, 2017

The mysql check fails on drone with:

undefined reference to `__libc_malloc'
race_linux_amd64.sy

Some linker flags missing @typeless ?

@lunny lunny force-pushed the enable-race-detector-in-ci branch from 57a07cd to e9915f9 Compare April 30, 2017 06:24
@appleboy
Copy link
Member

@lunny Need to resign drone config.

@lunny lunny force-pushed the enable-race-detector-in-ci branch from e9915f9 to 34a00ff Compare April 30, 2017 06:33
@lunny
Copy link
Member

lunny commented Apr 30, 2017

build failed. what's this collect2: error: ld returned 1 exit status

@bkcsoft bkcsoft added status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR and removed status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR labels May 3, 2017
@bkcsoft
Copy link
Member

bkcsoft commented May 3, 2017

Is the drone.sig updated? otherwise put status/blocked on this :sli

@lunny
Copy link
Member

lunny commented May 3, 2017

@bkcsoft yes, updated

@lunny
Copy link
Member

lunny commented May 25, 2017

@typeless any news?

@typeless
Copy link
Contributor Author

typeless commented May 25, 2017 •
edited

@lunny It looks that glibc is absent. Alpine Linux uses musl libc which is probably incompatible (or it only supports static linking?)

Also I found this golang/go#9918.

@lunny
Copy link
Member

lunny commented May 25, 2017

So let's move it to v1.x.x

@lunny lunny added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label May 25, 2017
@lunny lunny added this to the 1.x.x milestone May 25, 2017
@lunny lunny force-pushed the enable-race-detector-in-ci branch from d8bc88b to 8a969a1 Compare August 25, 2021 15:59
@lunny
Copy link
Member

lunny commented Aug 26, 2021

CI failed seems not related.

@lunny
Copy link
Member

lunny commented Aug 26, 2021

make L-G-T-M work

@lunny lunny merged commit b60e814 into go-gitea:main Aug 26, 2021
zeripath added a commit to zeripath/gitea that referenced this pull request Aug 26, 2021
@zeripath
Prevent "Race" detected in TestAdmin*User
c1f796f 
These tests are missing the defer prefix.

Related go-gitea#1441

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this pull request Aug 26, 2021
Merged
6543 pushed a commit that referenced this pull request Aug 27, 2021
@zeripath
Prevent "Race" detected in TestAdmin*User (#16830)
cd8db3a 
These tests are missing the defer prefix.

Related #1441

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath mentioned this pull request Aug 27, 2021
Merged
@6543 6543 removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Aug 28, 2021
@6543 6543 mentioned this pull request Aug 28, 2021
Merged
zeripath added a commit to zeripath/gitea that referenced this pull request Sep 13, 2021
@zeripath
Update go-chi/session (fixes "race" in tests)
975b3c8 
Update to latest go-chi/session where the NewManager causes a new
Provider instantiation instead of reconfiguring an old one.
(https://gitea.com/go-chi/session/pulls/1)

The NewManager call is now concurrency safe and would allow live
reconfiguration in future but for now this PR simply fixes an
intermittent "data-race" detected in our tests.  (See
https://drone.gitea.io/go-gitea/gitea/43900/2/14)

Related go-gitea#17027
Related go-gitea#1441

Signed-off-by: Andrew Thornton <art27@cantab.net>
This was referenced Sep 13, 2021
Merged
Merged
Merged
Merged
zeripath added a commit that referenced this pull request Sep 13, 2021
@zeripath
Update go-chi/session (fixes "race" in tests) (#17031)
8af7a21 
Update to latest go-chi/session where the NewManager causes a new
Provider instantiation instead of reconfiguring an old one.
(https://gitea.com/go-chi/session/pulls/1)

The NewManager call is now concurrency safe and would allow live
reconfiguration in future but for now this PR simply fixes an
intermittent "data-race" detected in our tests.  (See
https://drone.gitea.io/go-gitea/gitea/43900/2/14)

Related #17027
Related #1441

Signed-off-by: Andrew Thornton <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zeripath zeripath zeripath left review comments

@appleboy appleboy appleboy approved these changes

@lunny lunny lunny approved these changes

@techknowlogick techknowlogick techknowlogick approved these changes

@6543 6543 6543 approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/testing type/upstream This is an issue in one of Gitea's dependencies and should be reported there
Projects
None yet
Milestone
1.16.0
Development

Successfully merging this pull request may close these issues.

Make use of the race detector