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

Krnlmon housekeeping #121

Merged
merged 10 commits into from
Apr 18, 2024
Merged

Krnlmon housekeeping #121

merged 10 commits into from
Apr 18, 2024

Conversation

ffoulkes
Copy link
Contributor

@ffoulkes ffoulkes commented Apr 12, 2024

  • Removed include/bf_pal, include/bf_types, and include/ipu_types from target_include_directories() commands.

    • Krnlmon now communicates with the non-TDI portions of the SDE through the SDE abstraction layer (switchsde).
    • #includes for SDE header files now specify the file path relative to the include directory, to work better with Bazel. This removes the need to specify anything but ${SDE_INSTALL_DIR}/include in the directory path.
  • Updated cmake file headers.

  • Removed redundant target properties.

    • The switchapi/es2k/lnw_v2 and switchapi/es2k/lnw_v3 cmake listfiles defined the same target properties as the parent listfile.
    • The switchapi/dpdk and switchapi/es2k listfiles specify some of the same properties as their parent.
    • The subdirectories add to the parent target. There is no need for them to add properties that have already been specified.
  • Replaced a couple of cc_library() calls with krnlmon_cc_library().

- Removed include/bf_pal, include/bf_types, and include/ipu_types
  from all target_include_directories() commands.

  1) The SDE abstraction layer (switchsde) is the only part of
     krnlmon that is allowed to #include files from these
     directories. All other parts of krnlmon must go through the
     abstraction layer.

  2) All #includes for SDE header files should be qualified by a
     parent directory(e.g., #include "bf_types/bf_types.h").
     This provides context for the reader as well as disambiguating
     the reference. (There are are an awful lot of "utils.h" files
     floating around.)

Signed-off-by: Derek G Foster <[email protected]>
Signed-off-by: Derek G Foster <[email protected]>
@ffoulkes ffoulkes marked this pull request as ready for review April 12, 2024 23:11
@ffoulkes ffoulkes added cmake Affects CMake build system minor effort Minimal effort required labels Apr 12, 2024
Signed-off-by: Derek G Foster <[email protected]>
Signed-off-by: Derek G Foster <[email protected]>
Signed-off-by: Derek G Foster <[email protected]>
- Updated file headers.
- Deleted unused variables.
- Removed an unnecessary extern "C" block.

Signed-off-by: Derek G Foster <[email protected]>
@ffoulkes ffoulkes mentioned this pull request Apr 14, 2024
Copy link
Collaborator

@5abeel 5abeel left a comment

Choose a reason for hiding this comment

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

LGTM

@ffoulkes ffoulkes added bazel Affects Bazel build system medium effort Moderate effort required minor effort Minimal effort required and removed minor effort Minimal effort required medium effort Moderate effort required labels Apr 15, 2024
Copy link
Collaborator

@aashishkuma aashishkuma left a comment

Choose a reason for hiding this comment

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

LGTM

@ffoulkes ffoulkes merged commit 58d0e10 into main Apr 18, 2024
4 checks passed
@ffoulkes ffoulkes deleted the housekeeping branch April 18, 2024 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel Affects Bazel build system cmake Affects CMake build system minor effort Minimal effort required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants