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

Clean up dependency on proxygen libs #488

Merged
merged 3 commits into from
Jun 1, 2019
Merged

Clean up dependency on proxygen libs #488

merged 3 commits into from
Jun 1, 2019

Conversation

dangleptr
Copy link
Contributor

I found the problem when i write a UT with no dependency of proxygen lib.
It is introduced by #210 and #395

Please pay more attention to your dependency. @steppenwolfyuetong @darionyaphet

@dangleptr dangleptr added the ready-for-testing PR: ready for the CI test label May 31, 2019
@dangleptr
Copy link
Contributor Author

Jenkins go

ayyt
ayyt reviewed May 31, 2019
View reviewed changes
Copy link
Contributor

@ayyt ayyt left a comment

Choose a reason for hiding this comment

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

Awesome work! 👍 I added some inline comments.

src/storage/test/StorageHttpHandlerTest.cpp
dataPath.c_str(),
localIp,
localDataPort);

{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can explain that the mockserver is not started, how can the latter test run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We just need the http server.

src/graph/CMakeLists.txt
add_library(
graph_http_obj OBJECT
GraphHttpHandler.cpp
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no dependency here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just depend on base_obj. Good catch

Copy link
Contributor

@dutor dutor May 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Here is adding a library, OBJECT library, which shall have no dependencies.

ayyt
ayyt reviewed May 31, 2019
View reviewed changes
src/meta/CMakeLists.txt Outdated
)
add_dependencies(
meta_http_handler
base_obj)
Copy link
Contributor

Choose a reason for hiding this comment

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

right bracket format

@dutor
Copy link
Contributor

dutor commented May 31, 2019

Executables and shared libraries depend on other libraries, while static or OBJECT libraries depend on other headers, not libraries.

Now add_dependencies brings us too many chaos, we have to address this recently.

Please refer to this issue #406 .

@dutor
Copy link
Contributor

dutor commented May 31, 2019 •
edited
Loading

Besides, by moving third-party out from our repo, add_dependencies is no longer needed anymore.

@dangleptr
Copy link
Contributor Author

Executables and shared libraries depend on other libraries, while static or OBJECT libraries depend on other headers, not libraries.

Now add_dependencies brings us too many chaos, we have to address this recently.

Please refer to this issue #406 .

Make sense. OBJECT should just depend on the headers.
"add_dependencies" only work for Executables

@dangleptr
Copy link
Contributor Author

Besides, by moving third-party out from our repo, add_dependencies is no longer needed anymore.

Very good.

ayyt
ayyt approved these changes May 31, 2019
View reviewed changes
Copy link
Contributor

@ayyt ayyt left a comment

Choose a reason for hiding this comment

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

👍，I will study hard about cmake.

@nebula-community-bot
Copy link
Member

Unit testing passed.

1 similar comment
@nebula-community-bot
Copy link
Member

Unit testing passed.

@nebula-community-bot
Copy link
Member

Unit testing passed.

@dangleptr dangleptr merged commit c50426e into vesoft-inc:master Jun 1, 2019
@dangleptr dangleptr deleted the remove branch June 1, 2019 06:10
yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Feb 16, 2020
@dangleptr
Clean up dependency on proxygen libs (vesoft-inc#488)
806528b 
* Clean up dependency on proxygen libs

* Address comments
tong-hao pushed a commit to tong-hao/nebula that referenced this pull request Jun 1, 2021
@dangleptr
Clean up dependency on proxygen libs (vesoft-inc#488)
c9c8db8 
* Clean up dependency on proxygen libs

* Address comments
yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Mar 21, 2022
@nebula-bots @cangfengzhs
Issue3373 storage exit crash (vesoft-inc#3553) (vesoft-inc#488)
720f8e0 
* use rcu replace thread local

fix storage exit crash

format

address some comment

* fix bug

* fix bug

fix bug

fix bug

Co-authored-by: hs.zhang <22708345+cangfengzhs@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dutor dutor dutor approved these changes

@ayyt ayyt ayyt approved these changes

@laura-ding laura-ding laura-ding approved these changes

@darionyaphet darionyaphet Awaiting requested review from darionyaphet

@critical27 critical27 Awaiting requested review from critical27

Assignees
No one assigned
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dangleptr @dutor @nebula-community-bot @ayyt @laura-ding