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

CMake improvements and compiler warning fixes #810

Closed
wants to merge 7 commits into from

Conversation

nzqo
Copy link

@nzqo nzqo commented Nov 12, 2024

Pull Request Details

I extracted these changes from my other MR that was closed just now (#679) without any changes to the examples. The original changes were made over a year ago and after rebasing a few of the fixes done there have already been committed in other MRs. What remains is rather little, so should hopefully pose no problems to merge

Description

Basically, after rebasing onto the current version of uhd, here's what these changes still entail:

  • install_name wasnt manually set in CMake, which caused a warning in newer CMake versions due to breaking policy changes
  • Version range in DPDK find package module wasnt handled properly. Fixed by simply adding HANDLE_VERSION_RANGE flag
  • Substituted use of sprintf with safer and more idiomatic snprintf variant
  • Included boost as System dependency. This effectively suppresses compiler warnings from included boost headers so one can focus on the uhd ones
  • Fixed compiler warnings (unused variables and missing copy assignment)
  • Some minor comment formatting improvements for readability.

Which devices/areas does this affect?

Minor changes to the build system and the host library.

Testing Done

Since this affects mainly the build system, testing was building and verifying that CMake and compiler warnings were fixed.

Checklist

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project. See CODING.md.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes, and all previous tests pass.
  • I have checked all compat numbers if they need updating (FPGA compat,
    MPM compat, noc_shell, specific RFNoC block, ...)

@mbr0wn
Copy link
Contributor

mbr0wn commented Feb 17, 2025

recheck

nzqo added 4 commits February 18, 2025 00:27
Newer versions of cmake do not set install_name, so we need to do it
ourselves.
FindDPDK module did not properly handle version range before, this
change fixes this.
Also made boost a system dependency to suppress compiler warnings from
it.
Some minor formatting, spaces etc
This change fixes a few annoying compiler warnings. Mostly, those
are related to unused variables which are simply suppressed.
Also fixes a missing virtual destructor which could have resulted
in memory issues.
@nzqo
Copy link
Author

nzqo commented Feb 17, 2025

recheck

rebased just now.

@mbr0wn
Copy link
Contributor

mbr0wn commented Feb 18, 2025

recheck

rebased just now.

Sorry, that was for the CLA checker bot (I did some upgrades to fix that). You already signed the cla on #679, so you don't need to do that again, but I was hoping the CLA checker bot would automatically figure that out.

@mbr0wn
Copy link
Contributor

mbr0wn commented Feb 18, 2025

Also, thanks for rebasing, and thanks for re-submitting this in the first place (after we had dealt with just the example portion of #679). We're finally in a good spot to review this and merge it. We might cherry-pick some changes and leave out others, though.

@mbr0wn
Copy link
Contributor

mbr0wn commented Feb 18, 2025

Fixed compiler warnings (unused variables and missing copy assignment)

What's your compiler?

- Added missing override specifiers
- Specified CMake policy for FindBoost
- Reintroduced capture of chan variable
@nzqo
Copy link
Author

nzqo commented Feb 18, 2025

If you enable trace logging, this will cause a compiler error.

Reintroduced. You are right, probably leftover from an older state.

What's your compiler?

Currently: AppleClang 16.0.0.16000026
I recompiled with that and strict CXX flags. Fixed a few more missing override specifiers and another CMake policy warning.

FYI: in mpm/python/setup.py.in, the tests_require keyword has been deprecated for a while (https://setuptools.pypa.io/en/latest/references/keywords.html#keyword-tests-require). I'm not touching this here since I'm not sure about your python build process and to avoid dragging the MR out any longer :)

@mbr0wn
Copy link
Contributor

mbr0wn commented Feb 18, 2025

I'm going to skip the policy change for now (we support an almost too wide range of CMake versions, and I also want to close this PR ASAP, and I don't want to spend time making sure we don't add a warning on some older CMake for now). Other than that, it's good to go. Should be another few days before it hits master, but thanks again!

@mbr0wn
Copy link
Contributor

mbr0wn commented Feb 19, 2025

When you build this, what does uhd_config_info --dpdk-version print?

@mbr0wn mbr0wn closed this Feb 20, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Feb 20, 2025
@mbr0wn
Copy link
Contributor

mbr0wn commented Feb 20, 2025

I merged this without the changes to FindDPDK and the policy changes for now. It took a while to untangle issues with our internal CI pipelines (older CMake caused an impressive grind-to-halt on our DPDK tests). I think the HANDLE_VERSION_RANGE changes would be nice to add though so maybe I'll take that into our CI as well.

DPDK_INCLUDE_DIRS
DPDK_CFLAGS DPDK_LDFLAGS
DPDK_LIBRARIES
FAIL_MESSAGE "DPDK not found or misconfigured"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is declared "not recommended".

@mbr0wn
Copy link
Contributor

mbr0wn commented Feb 20, 2025

What kind of warning do you see without HANDLE_VERSION_RANGE?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants