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

tiny improvement #1093

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

tiny improvement #1093

wants to merge 2 commits into from

Conversation

joeyjiaojg
Copy link
Contributor

@joeyjiaojg joeyjiaojg commented May 16, 2023 •
edited
Loading

  1. pass JOBS to build.sh
  2. pass COMPILE_FLAGS to cmake config

yuleisui
yuleisui reviewed May 16, 2023
View reviewed changes
build.sh Outdated Show resolved Hide resolved
build.sh Outdated Show resolved Hide resolved
svf/include/Util/Annotator.h Outdated Show resolved Hide resolved
joeyjiaojg
joeyjiaojg commented May 16, 2023
View reviewed changes
Copy link
Contributor Author

@joeyjiaojg joeyjiaojg left a comment

Choose a reason for hiding this comment

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

updated, please check again?

build.sh Outdated Show resolved Hide resolved
build.sh Outdated Show resolved Hide resolved
@joeyjiaojg joeyjiaojg force-pushed the master branch 3 times, most recently from 10fdd04 to 7660ea6 Compare May 16, 2023 06:01
joeyjiaojg
joeyjiaojg commented May 16, 2023
View reviewed changes
Copy link
Contributor Author

@joeyjiaojg joeyjiaojg left a comment

Choose a reason for hiding this comment

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

done, pls review again.

yuleisui
yuleisui reviewed May 16, 2023
View reviewed changes
build.sh Outdated
@@ -6,11 +6,13 @@
# Dependencies include: build-essential libncurses5 libncurses-dev cmake zlib1g-dev
set -e # exit on first error

jobs=4
jobs=$(echo $(nproc) || echo $(sysctl -n hw.physicalcpu))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make it one line rather than a if condition

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be faster when you were trying to build svf using more than 4 threads? I think we can leave it as 4 and people can simply adjust it by themselves

Copy link
Contributor Author

Choose a reason for hiding this comment

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

much faster when I use blade with higher jobs.

build.sh Outdated Show resolved Hide resolved
build.sh Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 16, 2023 •
edited
Loading

Codecov Report

Merging #1093 (f723d9d) into master (984e759) will not change coverage.
The diff coverage is n/a.

❗ Current head f723d9d differs from pull request most recent head 3a2f707. Consider uploading reports for the commit 3a2f707 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1093   +/-   ##
=======================================
  Coverage   64.56%   64.56%           
=======================================
  Files         222      222           
  Lines       23518    23518           
=======================================
  Hits        15184    15184           
  Misses       8334     8334

yuleisui
yuleisui reviewed May 16, 2023
View reviewed changes
build.sh Outdated Show resolved Hide resolved
@joeyjiaojg joeyjiaojg force-pushed the master branch 2 times, most recently from 46333d1 to 23b8c01 Compare May 18, 2023 06:53
@joeyjiaojg
Copy link
Contributor Author

The new patch should be better now.

@joeyjiaojg
Copy link
Contributor Author 

CMake Error at CMakeLists.txt:3 (project):
  No CMAKE_CXX_COMPILER could be found.

  Tell CMake where to find the compiler by setting either the environment
  variable "CXX" or the CMake cache entry CMAKE_CXX_COMPILER to the full path
  to the compiler, or to the compiler name if it is in the PATH.

OK, no better way I know without using any bash condition variable.

@joeyjiaojg
add COMPILE_FLAGS pass to cmake config
d8108a2 
Ex to build with clang.

```
export LLVM_DIR=/usr/src/clang-android
export LLVM_CONFIG=$LLVM_DIR/bin/llvm-config
COMPILE_FLAGS="-DCMAKE_CXX_COMPILER=${LLVM_DIR}/bin/clang++ -DLLVM_DIR=${LLVM_DIR}" source build.sh
```
@joeyjiaojg
JOBS to override default jobs
3a2f707 
JOBS=$(nproc) bash build.sh
@joeyjiaojg
Copy link
Contributor Author

@yuleisui ok, this should be final?

people can choose to build with clang and other JOBS value

export LLVM_DIR=/usr/src/clang-android && export LLVM_CONFIG=$LLVM_DIR/bin/llvm-config && JOBS=$(nproc) COMPILE_FLAGS="-DCMAKE_CXX_COMPILER=${LLVM_DIR}/bin/clang++ -DLLVM_DIR=${LLVM_DIR}" ./build.sh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuleisui yuleisui yuleisui left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@joeyjiaojg @yuleisui