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

feat(function): support md5 #5846

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

Conversation

fansehep
Copy link
Contributor

@fansehep fansehep commented Apr 2, 2024

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Description:

How do you solve it?

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

Checklist:

Tests:

  • Unit test(positive and negative cases)
  • Function test
  • Performance test
  • N/A

Affects:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatibility (If it breaks the compatibility, please describe it and add the label.)
  • If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....

@CLAassistant
Copy link

CLAassistant commented Apr 2, 2024

CLA assistant check
All committers have signed the CLA.

@QingZ11 QingZ11 requested a review from Salieri-004 April 7, 2024 06:19
@Salieri-004 Salieri-004 added the ready-for-testing PR: ready for the CI test label Apr 7, 2024
@QingZ11
Copy link
Contributor

QingZ11 commented Apr 7, 2024

Thank you for your contribution. The automated lint test has failed. You can click on this link or check the automated test results below to see where the issue is and fix it. Once fixed, you can commit the changes.

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, but need to fix the lint error

CMakeLists.txt Outdated
@@ -30,6 +30,8 @@ option(ENABLE_PACKAGE_TAR "Enable package artifacts to tar." OFF)
option(ENABLE_CREATE_GIT_HOOKS "Enable create git hooks." ON)
option(ENABLE_INCLUDE_WHAT_YOU_USE "Enable include-what-you-use find nouse include files" OFF)

SET(CMAKE_EXPORT_COMPILE_COMMANDS TRUE)
Copy link
Contributor

Choose a reason for hiding this comment

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

This CMAKE_EXPORT_COMPILE_COMMANDS option could be specified in cmake configuration step and need not to always turn it on. please cleanup it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please let the CI run.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fansehep hi, according to yixinglu's response, you can simply remove the added CMAKE_EXPORT_COMPILE_COMMANDS configuration statement. Currently, it is set to true, but as per yixinglu's suggestion, it is already configurable elsewhere, so there is no need to hardcode it 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.

thanks for your reminders, but from the CI compile output, the mold linker link error :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Nebula CI sometimes needs to run a few more times to pass. I will rerun it a few more times. Wait for my good news 🥺.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for your reminders, but from the CI compile output, the mold linker link error :(

Perhaps you should add some link dependencies, as commented in this issue. #5840

Copy link
Contributor

Choose a reason for hiding this comment

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

@fansehep To fix the failure of CI, u'd better to check where the target $<TARGET_OBJECTS:function_manager_obj> is used in the whole code base, and add the the ${PROXYGEN_LIBRARIES} dependency for there.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fansehep CI has been fixed, shall we go on?

Signed-off-by: alexsehep <[email protected]>
Signed-off-by: alexsehep <[email protected]>
Signed-off-by: alexsehep <[email protected]>
E2ern1ty added a commit to E2ern1ty/nebula that referenced this pull request Sep 26, 2024
@E2ern1ty E2ern1ty mentioned this pull request Sep 26, 2024
11 tasks
codesigner pushed a commit that referenced this pull request Oct 9, 2024
* feat(function): support md5

(cherry picked from commit 84bb690)

* format code

Signed-off-by: alexsehep <[email protected]>
(cherry picked from commit d2b17d4)

* clean cmake option

Signed-off-by: alexsehep <[email protected]>
(cherry picked from commit 993bea6)

* fix cmake

Signed-off-by: alexsehep <[email protected]>
(cherry picked from commit e607095)

* fix cmake error in #5846

---------

Co-authored-by: alexsehep <[email protected]>
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.

6 participants