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

add cmake git hooks options #3390

Merged
merged 7 commits into from
Dec 13, 2021
Merged

add cmake git hooks options #3390

merged 7 commits into from
Dec 13, 2021

Conversation

jiayuehua
Copy link
Contributor

@jiayuehua jiayuehua commented Dec 1, 2021

What type of PR is this?

  • [*] bug
  • feature
  • enhancement

What does this PR do?

add option that can toggle wether cmake generate git hooks , default ON, So that platform without sh shell can set it OFF and commit code.

Which issue(s)/PR(s) this PR relates to?

Special notes for your reviewer, ex. impact of this fix, etc:

Additional context:

Checklist:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatible (If it is incompatible, please describe it and add corresponding label.)
  • Need to cherry-pick (If need to cherry-pick to some branches, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to reflect in release notes and how to describe:

                                                            `

@@ -1,19 +1,21 @@
if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/.git/")
Copy link
Contributor

Choose a reason for hiding this comment

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

if (ENABLE_CREATE_GIT_HOOKS AND (EXISTS "xxx"))
 # ...
endif()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix it.

@Sophie-Xie Sophie-Xie added doc affected PR: improvements or additions to documentation ready-for-testing PR: ready for the CI test labels Dec 3, 2021
@jiayuehua
Copy link
Contributor Author

has make if condition more clear.

@jiayuehua jiayuehua assigned jiayuehua and unassigned jiayuehua Dec 9, 2021
@Sophie-Xie Sophie-Xie removed their request for review December 9, 2021 08:31
@jiayuehua jiayuehua removed the request for review from jackwener December 9, 2021 08:45
yixinglu
yixinglu previously approved these changes Dec 9, 2021
Copy link
Contributor

@yixinglu yixinglu left a comment

Choose a reason for hiding this comment

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

LGTM

@yixinglu
Copy link
Contributor

yixinglu commented Dec 9, 2021

@sherman-the-tank Yuehua uses visual studio IDE to develop nebula project locally. I think it's ok to introduce this option, it will not affect current workflow of others and is convenient for him to develop in windows system. WDYT?

@jiayuehua jiayuehua requested review from zhaohaifei and wey-gu and removed request for wey-gu December 10, 2021 06:11
@jiayuehua
Copy link
Contributor Author

Add include what you use to let developer delete no use include files.

@@ -0,0 +1,11 @@
if(ENABLE_IncludeWhatYouUse)
find_program(IncludeWhatYouUse include-what-you-use)
Copy link
Contributor

Choose a reason for hiding this comment

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

IncludeWhatYouUse_FOUND

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not to turn on iwyu when find the program automatically? i think this option is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it run very slow, Just let developer run it .

Copy link
Member

@sherman-the-tank sherman-the-tank left a comment

Choose a reason for hiding this comment

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

Well done!

Just couple styling comments

CMakeLists.txt Outdated
@@ -26,6 +26,8 @@ project("Nebula Graph" C CXX)
option(ENABLE_PACK_ONE "Whether to package into one" ON)
option(ENABLE_VERBOSE_BISON "Enable Bison to report state" OFF)
option(ENABLE_PACKAGE_TAR "Enable package artifacts to tar." OFF)
option(ENABLE_CREATE_GIT_HOOKS "Enable create git hooks." ON)
option(ENABLE_IncludeWhatYouUse "Enable Include_What_You_Use" OFF)
Copy link
Member

Choose a reason for hiding this comment

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

Please use all CAPITAL, like other options, following the convention

@@ -0,0 +1,11 @@
if(ENABLE_IncludeWhatYouUse)
Copy link
Member

Choose a reason for hiding this comment

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

Suggest to place this file in cmake/nebula

Copy link
Contributor Author

@jiayuehua jiayuehua Dec 10, 2021

Choose a reason for hiding this comment

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

Ok, I move it to cmake/nebula

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I change it to CAPITAL

@jiayuehua jiayuehua removed the request for review from zhaohaifei December 10, 2021 08:33
@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2021

Codecov Report

Merging #3390 (d48a879) into master (9a0fbb6) will decrease coverage by 0.05%.
The diff coverage is 94.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3390      +/-   ##
==========================================
- Coverage   85.40%   85.35%   -0.06%     
==========================================
  Files        1289     1293       +4     
  Lines      119798   120044     +246     
==========================================
+ Hits       102317   102461     +144     
- Misses      17481    17583     +102     
Impacted Files Coverage Δ
src/common/time/TimezoneInfo.cpp 50.00% <ø> (+4.54%) ⬆️
src/daemons/GraphDaemon.cpp 65.51% <50.00%> (-0.56%) ⬇️
src/daemons/MetaDaemon.cpp 62.62% <50.00%> (-0.27%) ⬇️
src/daemons/StorageDaemon.cpp 62.22% <60.00%> (-0.57%) ⬇️
src/common/datatypes/test/ValueToJsonTest.cpp 88.19% <64.28%> (-0.56%) ⬇️
src/common/time/TimeUtils.h 84.00% <80.00%> (-4.58%) ⬇️
src/storage/StorageServer.cpp 83.26% <92.85%> (+0.59%) ⬆️
src/common/time/parser/DatetimeReader.cpp 97.22% <97.22%> (ø)
src/common/time/parser/test/DateTimeParserTest.cpp 99.19% <99.19%> (ø)
src/common/datatypes/Date.cpp 100.00% <100.00%> (ø)
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35ddd01...d48a879. Read the comment docs.

@yixinglu yixinglu merged commit 58914a7 into vesoft-inc:master Dec 13, 2021
@foesa-yang foesa-yang removed the doc affected PR: improvements or additions to documentation label Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants