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

doc: add ASAN build instructions #32436

Closed
wants to merge 1 commit into from
Closed

Conversation

gengjiawen
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Mar 23, 2020
@mmarchini
Copy link
Contributor

A few suggestions:

  1. Instead of suggesting to build on docker Windows and Mac, we should make sure our config works on those operating systems. Especially on Mac using docker can be really slow since it runs on a VM (well, it also runs on a VM on Windows, but with HyperV it's a bit faster). Not only that, if we use docker we'll never have ASAN checking Windows and Mac specific code paths
  2. Also add instructions to build with clang
  3. --debug is not required. We can suggest debug build in the paragraph and explain why it's useful, but the first example command shouldn't have it because it's a lot slower to build and to run tests
  4. --ninja is not our official build tool and is not required for ASAN, therefore it shouldn't be here as well (folks who prefer Ninja probably already know the option exist). Maybe mention that building with ninja also works
  5. With the suggestions above we can use make test-only instead of executing the tests manually. Calling tools/test.py will miss cctest for example, so it's better to keep it simple and follow our current build workflow

BUILDING.md Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
@gengjiawen
Copy link
Member Author

Instead of suggesting to build on docker Windows and Mac, we should make sure our config works on those operating systems.

On windows ASAN only support cmake IIRC. macOS is also broken for our repo. on Container it should cover most cases. But I agree it would be much better we check on those platforms too. So if anyone want to help with the asan issue on windows and macOS, it's the fastest way (I am using docker even on linux).

BUILDING.md Outdated Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
BUILDING.md Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
BUILDING.md Outdated
related bugs. ASAN builds are currently only supported on linux.
If you want to check it on Windows or macOS or you want a consistent toolchain
on Linux, you can try [Docker](https://www.docker.com/products/docker-desktop)
(using an image like `gengjiawen/node-build:2020-02-14`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing . at the end of the sentence.

@mmarchini
Copy link
Contributor

mmarchini commented Mar 27, 2020

Oh right, I almost forgot: before we land it, should we add instructions on how to build with clang (since ASAN works generally better with clang)?

$ CC=clang CXX=clang++ LINK=clang++ ./configure --debug --enable-asan 
$ CC=clang CXX=clang++ LINK=clang++ make -j4
$ CC=clang CXX=clang++ LINK=clang++ make test-only 

Or, if the user is using the docker container:

$ docker run -e CC=clang -e CXX=clang++ -e LINK=clang++ ...

(not a blocker, but I had better results with clang)

@nodejs-github-bot

This comment has been minimized.

@addaleax
Copy link
Member

This doesn’t need a full CI run, but the commit message should start with doc:.

@gengjiawen
Copy link
Member Author

This doesn’t need a full CI run, but the commit message should start with doc:.

Should I just run this one https://ci.nodejs.org/job/node-test-linter/ ?

@gengjiawen gengjiawen changed the title build: add ASAN build instructions doc: add ASAN build instructions Mar 31, 2020
@mmarchini
Copy link
Contributor

In this case just Actions is fine (since it's a doc-only change)

gengjiawen added a commit that referenced this pull request Apr 1, 2020
PR-URL: #32436
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@gengjiawen
Copy link
Member Author

gengjiawen commented Apr 1, 2020

Landed in 9c00af0

@gengjiawen gengjiawen closed this Apr 1, 2020
@gengjiawen
Copy link
Member Author

Oh right, I almost forgot: before we land it, should we add instructions on how to build with clang (since ASAN works generally better with clang)?

$ CC=clang CXX=clang++ LINK=clang++ ./configure --debug --enable-asan 
$ CC=clang CXX=clang++ LINK=clang++ make -j4
$ CC=clang CXX=clang++ LINK=clang++ make test-only 

Or, if the user is using the docker container:

$ docker run -e CC=clang -e CXX=clang++ -e LINK=clang++ ...

(not a blocker, but I had better results with clang)

Good point, the new docker image now is set to clang by default. Also we should make that use clang in general build and mention in ASAN part clang has better output ?

@gengjiawen gengjiawen deleted the doc/asan branch April 1, 2020 02:57
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
PR-URL: #32436
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
targos pushed a commit that referenced this pull request Apr 12, 2020
PR-URL: #32436
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
targos pushed a commit that referenced this pull request Apr 22, 2020
PR-URL: #32436
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants