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

apps/nxdiag: Update nxdiag app due to script place changes #2925

Merged

Conversation

eren-terzioglu
Copy link
Contributor

Summary

Related to apache/nuttx#15304
Change path nxdiag script and remove system diagnostic scripts. Update suggested from apache/nuttx#15304 (comment)

Impact

Common layer update

Testing

esp32c6-devkitc:nsh config used with SYSTEM_NXDIAG option enabled

@eren-terzioglu
Copy link
Contributor Author

eren-terzioglu commented Dec 30, 2024

Error is normal, it is about related PR. It will be fixed when related PR merges. Error log:

2024-12-30T15:29:05.1065227Z python3: can't open file '/github/workspace/sources/nuttx/tools/host_sysinfo.py': [Errno 2] No such file or directory
2024-12-30T15:29:05.1082311Z make[2]: *** [Makefile:120: sysinfo.h] Error 2

Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @eren-terzioglu :-)

  • My question is why Python scripts are removed from tools/ and how does that relate to nxdiag relocation?
  • Why the removal takes place in this commit?
  • There is no message explaining the change - what was changed into what and what changes are related?
  • I guess you want to move some stuff from nuttx/tools to nuttx-apps/../nxdiag. But what if other folks used that nuttx/tools stuff already for other purpose? What if it has to be moved back again or re-created in nuttx/tools for some reason resulting in tools inconsistency?
  • Couldn't nxdiag use the nuttx/tools stuff already as a most compatible way so we keep all tools in the nuttx/tools?
  • Doesn't it require some documentation explanation?
  • I am in general not fan of removing working stuff. If there is a bigger consensus for this please convince me otherwise :-)

@eren-terzioglu
Copy link
Contributor Author

Thank you @eren-terzioglu :-)

  • My question is why Python scripts are removed from tools/ and how does that relate to nxdiag relocation?
  • Why the removal takes place in this commit?
  • There is no message explaining the change - what was changed into what and what changes are related?
  • I guess you want to move some stuff from nuttx/tools to nuttx-apps/../nxdiag. But what if other folks used that nuttx/tools stuff already for other purpose? What if it has to be moved back again or re-created in nuttx/tools for some reason resulting in tools inconsistency?
  • Couldn't nxdiag use the nuttx/tools stuff already as a most compatible way so we keep all tools in the nuttx/tools?
  • Doesn't it require some documentation explanation?
  • I am in general not fan of removing working stuff. If there is a bigger consensus for this please convince me otherwise :-)
  • We moved host_sysinfo.py to nuttx/tools due to keep every script in one place for conveinence
  • I changed the script for current nxdiag users, it is working without knowing anything. We can access nuttx/tools scripts (host_sysinfo.py in this case) through apps/nxdiag scripts
  • Last but not least, suggestion came from this comment. We can wait to see which implementation idea should be done.

Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Perfect! Thank you @eren-terzioglu !! :-)

I have restarted the CI jobs.. lets see if there are still some build errors.. probably comes from dependency on apache/nuttx#15304 ? :-)

@eren-terzioglu eren-terzioglu force-pushed the feature/add_debug_info branch from 874fad2 to 38e23dc Compare January 7, 2025 08:05
@eren-terzioglu
Copy link
Contributor Author

eren-terzioglu commented Jan 7, 2025

Perfect! Thank you @eren-terzioglu !! :-)

I have restarted the CI jobs.. lets see if there are still some build errors.. probably comes from dependency on apache/nuttx#15304 ? :-)

Thanks,
I updated and tested but seems error is still happening. I checked but there was no issue called error, failed or warning . I guess this lines reasons the error:

2025-01-07T08:45:11.1322592Z On branch master
2025-01-07T08:45:11.1323043Z Your branch is up to date with 'origin/master'.
2025-01-07T08:45:11.1323377Z 
2025-01-07T08:45:11.1323505Z Ignored files:
2025-01-07T08:45:11.1324175Z   (use "git add -f <file>..." to include in what will be committed)
2025-01-07T08:45:11.1324728Z 	tools/espressif/__pycache__/

Could it be?

Update: It does and fixed

@eren-terzioglu eren-terzioglu force-pushed the feature/add_debug_info branch from 38e23dc to 542ad75 Compare January 7, 2025 10:03
Nxdiag app build scripts updated due to changes to make diagnostic
tools independent from nxdiag app. Change aims that nxdiag app does
not a reqirement to fetch system information.
@cederom
Copy link

cederom commented Jan 7, 2025

All builds fine now, thank you @eren-terzioglu @xiaoxiang781216 :-)

@xiaoxiang781216 xiaoxiang781216 merged commit 6c7bc3c into apache:master Jan 7, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants