Skip to content

build: use Clang 12 for test-asan workflow #40238

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

Closed
targos wants to merge 1 commit into from
Closed

build: use Clang 12 for test-asan workflow #40238

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/test-asan.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ jobs:
if: github.event.pull_request.draft == false
runs-on: ubuntu-latest
env:
CC: clang
CXX: clang++
LINK: clang++
CC: clang-12
Copy link
Member

@gengjiawen gengjiawen Sep 29, 2021

Choose a reason for hiding this comment

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

Also latest clang is 13. Not sure github action bundled it yet.

Copy link
Member Author

@targos targos Sep 29, 2021

Choose a reason for hiding this comment

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

GitHub runners only have clang 10,11 and 12: https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-README.md#language-and-runtime

Copy link
Member

@gengjiawen gengjiawen Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Tried clang-13 locally, same result.

CXX: clang++-12
LINK: clang++-12
Copy link
Member

@gengjiawen gengjiawen Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is LINK here neccessary ?

Context: https://www.gnu.org/software/make/manual/html_node/Implicit-Variables.html

Copy link
Member Author

@targos targos Oct 12, 2021

Choose a reason for hiding this comment

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

Yeah, it doesn't seem useful. /cc @mmarchini

Copy link
Contributor

@mmarchini mmarchini Jan 29, 2022

Choose a reason for hiding this comment

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

I don't remember why LINK was necessary (to force clang linker instead of ld when linking? idk that's the only thing I can think of). We can always remove and see what happens

Copy link
Contributor

@mmarchini mmarchini Jan 29, 2022

Choose a reason for hiding this comment

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

I do think it was more of a GitHub Actions thing and less of a gnu thing

Copy link
Contributor

@mmarchini mmarchini Jan 29, 2022

Choose a reason for hiding this comment

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

Ok I remember now. ld would consume too much memory for ASAN to be possible on Actions, given the size of the instances available (see #32776). I'm almost sure LINK is not standard but is used by gyp. What I'm not entirely sure is if CXX being set to clang++-12 is enough. Again, someone would need to experiment (specifically looking at peak memory usage when LINK is set or not).

CONFIG_FLAGS: --enable-asan
steps:
- uses: actions/checkout@v3
Expand Down