-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Remove pointless ENABLE_ACLK
/ENABLE_CLOUD
split.
#17529
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is also working as expected. I have normal access to cloud, LGTM!
ead5231
to
4eb7b38
Compare
Rebased to pick up latest changes, also added a bit more cleanup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is working as expected after last commits!
CMakeLists.txt
Outdated
@@ -156,6 +155,8 @@ mark_as_advanced(BUILD_FOR_PACKAGING) | |||
cmake_dependent_option(FORCE_LEGACY_LIBBPF "Force usage of libbpf 0.0.9 instead of the latest version." False "ENABLE_PLUGIN_LIBBPF" False) | |||
mark_as_advanced(FORCE_LEGACY_LIBBPF) | |||
|
|||
set(ENABLE_ACLK ${ENABLE_CLOUD}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this and simply not use ENABLE_CLOUD
throughout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intent here was to avoid changing C code in this PR (we use ENABLE_ACLK
in essentially all places in the C code at the moment) because changing it would have made the PR massive and much harder to review, and thus theoretically would have delayed getting it merged. I plan to do a followup to convert the C code once this is merged, which will get rid of this bit of the CMakeLists file as well.
4eb7b38
to
4aeb26b
Compare
Rebased to resolve merge conflicts. |
4aeb26b
to
1ea3224
Compare
Rebased to resolve merge conflicts. |
This is a holdover from years ago when we had multiple versions of the agent-cloud link in our code. These days, everything in the code cares only about ENABLE_ACLK, and ENABLE_CLOUD is just used in the claiming script.
1ea3224
to
8c83f48
Compare
Summary
The distinction is a holdover from years ago when we had multiple versions of the agent-cloud link in our code. These days, everything in the code cares only about
ENABLE_ACLK
, andENABLE_CLOUD
is just used in the claiming script.This PR:
ENABLE_CLOUD
check in the claiming script so that it looks at only the things that the agent code looks at to determine cloud support.ENABLE_CLOUD
option and define as it now does nothing.ENABLE_ACLK
CMake option toENABLE_CLOUD
, because that is properly descriptive of what the option actually does.Test Plan
Local testing is required for this PR.