Skip to content

[#630] fix: Replace deprecated PanicInfo with direct panic hook implementation #631

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Nawazz-ish
Copy link

Notes for Reviewer

  • Replaces deprecated PanicInfo with modern std::panic::set_hook implementation
  • Removes human-panic dependency while maintaining the same functionality
  • Keeps better-panic for debug mode for detailed debugging information

Pre-Review Checklist for the PR Author

  • Add sensible notes for the reviewer
  • PR title is short, expressive and meaningful
  • Branch follows the naming format (iox2-630)
  • Commits messages have the issue ID ([#630])
  • Every source code file has a copyright header with SPDX-License-Identifier: Apache-2.0 OR MIT
  • Code formatted using rustfmt
  • Changelog updated in the unreleased section
  • Tests follow the best practice for testing

Changes

  • Removed human-panic dependency from Cargo.toml
  • Implemented direct panic hook using std::panic::set_hook
  • Maintained better-panic for debug builds
  • Added user-friendly error messages for release builds

Testing

  • Built in both debug and release modes
  • Verified removal of deprecation warnings
  • Tested panic message formatting in both modes

References

Closes #630

@Nawazz-ish
Copy link
Author

I've completed the requested changes: applied rustfmt and ensured proper code formatting.

@elfenpiff
Copy link
Contributor

@elBoberido it seems Bazel does not build since human_panic is no longer required. How would this be cleaned up?

@elBoberido
Copy link
Member

@Nawazz-ish you need to remove human-panic from iceoryx2-cli/BUILD.bazel

@elBoberido
Copy link
Member

@Nawazz-ish oh, you opened a second PR. I guess because of the branch name. Maybe we should rephrase the template to state that it is not a hard requirement for first time contributors.

Btw, can you please rebase your branch to main to eliminate the commit from elfenpiff in this PR. Then you can also add the issue number to your first commit. If you need advise on how to do that, don't hesitate to ask.

@Nawazz-ish
Copy link
Author

@elBoberido I've removed human-panic from BUILD.bazel and cleaned up the commit history as requested. The PR now includes only my commits with the proper issue reference.

@elfenpiff
Copy link
Contributor

@Nawazz-ish the last thing missing is that you create an account for the eclipse foundation with the same e-mail address you used to commit your stuff to sign the eclipse contributor agreement: https://www.eclipse.org/legal/eca/

If this is done I am happy to merge your changes.

@Nawazz-ish
Copy link
Author

It's done. I have created an account with the same email. [email protected].

Thanks

@elfenpiff
Copy link
Contributor

Argh, I have the suspicion that this PR is blocked by a bug in classic iceoryx: eclipse-iceoryx/iceoryx#2422

@Nawazz-ish
Copy link
Author

@elfenpiff
Thanks for the update. I understand this might be blocked by the issue you mentioned. Please let me know if there's anything I can do to help move this forward.

@elBoberido
Copy link
Member

@Nawazz-ish the git commits have [email protected]. You need to do an interactive rebase and set your email address.

Just in case you haven't done it before. If your name and e-mail address is set via git config, you can execute the following command if you are on Linux

git rebase -r HEAD~3 --exec 'git commit --amend --no-edit --reset-author'

With git log --pretty=fuller you can check whether the Author and Commit has your name and e-mail address.

You also need to add the [#] 630 prefix to the first and last commit message. In case you haven't done it before, it can also be done with git rebase -i HEAD~3. Just change the pick with r to change the commit message with the next steps.

@elBoberido
Copy link
Member

@Nawazz-ish it also seems there are some CI issues unrelated to your PR. We are looking into it and fix it in a separate PR. Of course, if you like, you can also give it a try :)

Since more or less the whole team is on a business trip right now, it might take a few days until the CI issue is fixed.

@elfenpiff it seems there was a llvm update on FreeBSD. The last successful build hat v15 and now v19 is installed and the build script complains about not finding libclang. For the Windows issue we need to create a v2.95.4 tag on iceoryx classic.

@elBoberido
Copy link
Member

@Nawazz-ish the CI issues are fixed on main. Once you rebase, only the commits need to be fixed

@elBoberido
Copy link
Member

@Nawazz-ish please rebase to main to get the latest CI fix

@Nawazz-ish
Copy link
Author

@elBoberido sure will do that ASAP

@elBoberido
Copy link
Member

elBoberido commented Mar 17, 2025

@Nawazz-ish oh, just noticed that my last message sounds a bit commanding. One of the github actions we use has had a CVE and it's fixed on the main branch, therefore the somewhat commanding tone :)

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

Successfully merging this pull request may close these issues.

fix: Replace deprecated PanicInfo usage in CLI tools with std::panic::set_hook
3 participants