Skip to content

[BREAKING]libc/elf: rename modlib to elf #2985

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

Merged
merged 1 commit into from
Apr 11, 2025
Merged

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Feb 5, 2025

Summary

libc/elf: rename modlib to elf

Renaming "modlib" to "elf" is more in line with the implementation content,
which makes it easier for individual developers to understand the capabilities of this module.

CONFIG_LIBC_MODLIB -> CONFIG_LIBC_ELF

Signed-off-by: chao an [email protected]

Impact

N/A, depends on apache/nuttx#15765

Testing

sim/sotest

@nuttxpr
Copy link

nuttxpr commented Feb 5, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and testing information, it lacks crucial details.

Here's what's missing:

  • Summary: While the "what" is explained, the "why" is weak. Why is "elf" a better name than "modlib"? What confusion does the current name cause? More context is needed. The summary also needs the related NuttX issue number.
  • Impact: "N/A, depends on another PR" is insufficient. Even with a dependency, this PR will have an impact. At minimum, it will change the configuration option name, which has build system impact. All impact sections should be explicitly addressed with "YES" or "NO" and a description if "YES." Consider the impact if/when the other PR is merged. Will this rename break anything? Will documentation need updates?
  • Testing: "sim/sotest" is inadequate. Provide specifics: Which sim? Which sotest? What commands were run? Actual log output, even if minimal, is required before and after the change. The current entry provides no evidence of testing. It also needs details about the build host used.

Therefore, the PR needs revision before it can be considered complete.

Copy link
Member

@lupyuen lupyuen left a comment

Choose a reason for hiding this comment

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

@anchao anchao force-pushed the 25020501 branch 3 times, most recently from 87e6b78 to 6d0afa6 Compare February 5, 2025 10:58
Copy link
Member

@lupyuen lupyuen left a comment

Choose a reason for hiding this comment

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

Tested OK on rv-virt:knsh64. Thanks :-)
https://gist.github.com/lupyuen/edb4747d1235c77be624bd4c6a24a500

+ git clone https://github.com/anchao/nuttx --branch 25020501 nuttx
+ git clone https://github.com/anchao/nuttx-apps --branch 25020501 apps
NuttX Source: https://github.com/apache/nuttx/tree/fa059c19fad275324afdfec023d24a85827516e9
NuttX Apps: https://github.com/apache/nuttx-apps/tree/6d0afa6c9b8d4ecb896f9aa177dbdfcd40218f48
+ tools/configure.sh rv-virt:knsh64
nsh> uname -a
NuttX 10.4.0 fa059c19fa Feb  5 2025 19:25:45 risc-v rv-virt
nsh> ostest
ostest_main: Exiting with status 0

Copy link
Member

@lupyuen lupyuen left a comment

Choose a reason for hiding this comment

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

Also tested OK on real hardware: Oz64 SG2000 RISC-V SBC (milkv_duos:nsh). Thanks :-)
https://gist.github.com/lupyuen/15fd5e68f66b03f19f476b5ef3c066f1

+ git clone https://github.com/anchao/nuttx nuttx --branch 25020501
+ git clone https://github.com/anchao/nuttx-apps apps --branch 25020501
NuttX Source: https://github.com/apache/nuttx/tree/fa059c19fad275324afdfec023d24a85827516e9
NuttX Apps: https://github.com/apache/nuttx-apps/tree/6d0afa6c9b8d4ecb896f9aa177dbdfcd40218f48
+ tools/configure.sh milkv_duos:nsh
nsh> uname -a
NuttX 10.4.0 fa059c19fa Feb  6 2025 22:00:28 risc-v milkv_duos
nsh> ostest
ostest_main: Exiting with status 0

@xiaoxiang781216 xiaoxiang781216 added the breaking change This change requires a mitigation entry in the release notes. label Feb 6, 2025
Copy link
Contributor

@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.

Thanks @anchao :-)

  • Why this change is really necessary? What it fixes / improves?
  • Modlib library [1] is meant for loadable modules of various formats. So far we only support ELF but other formats may be added. When other formats are added then elf name will be confusing again. We should stick to name modlib as this is loadable module component, not just elf handler.
  • How does that change impact existing configurations and backward-compatibility (i.e. older nuttx-apps)? You marked impact N/A but this seems breaking change so description is invalid? Also providing reference to different change is not an impact factor just a reference / dependency and should belong to summary section.
  • This change will breaks existing libc and configuration. It will break compatibility with nuttx-apps out of sync with nuttx. This change should be marked as breaking change. It would be good to first discuss this kind of change on mailing lists.
  • Where are build / runtime test logs? Did you verify build and runtime with latest release of nuttx-apps to see if it does not break things?
  • How does that align with other standards (libc) and OS/RTOS implementations?
  • How does that align with existing libelf API / compatibility?

[1] https://nuttx.apache.org/docs/latest/applications/examples/module/index.html
[2] https://github.com/apache/nuttx/tree/fdc0b608b5893a207ea37a276e0adade6ded3b62/libs/libc/modlib

@anchao
Copy link
Contributor Author

anchao commented Feb 7, 2025

Thanks @anchao :-)

  • Why this change is really necessary? What it fixes / improves?
  • Modlib library [1] is meant for loadable modules of various formats. So far we only support ELF but other formats may be added. When other formats are added then elf name will be confusing again. We should stick to name modlib as this is loadable module component, not just elf handler.
  • How does that change impact existing configurations and backward-compatibility (i.e. older nuttx-apps)? You marked impact N/A but this seems breaking change so description is invalid? Also providing reference to different change is not an impact factor just a reference / dependency and should belong to summary section.
  • This change will breaks existing libc and configuration. It will break compatibility with nuttx-apps out of sync with nuttx. This change should be marked as breaking change. It would be good to first discuss this kind of change on mailing lists.
  • Where are build / runtime test logs? Did you verify build and runtime with latest release of nuttx-apps to see if it does not break things?
  • How does that align with other standards (libc) and OS/RTOS implementations?
  • How does that align with existing libelf API / compatibility?

[1] https://nuttx.apache.org/docs/latest/applications/examples/module/index.html [2] https://github.com/apache/nuttx/tree/fdc0b608b5893a207ea37a276e0adade6ded3b62/libs/libc/modlib

NuttX have 2 elf implementations before, one is libelf and other one is modlib, 90% of the code was duplicated. Modlib has more features than libelf, including dynamic loading, so the libelf code was completely deleted in the following commit:
apache/nuttx#14100
but actually modlib is the implementation of elf. In this commit, I changed the name of modlib back to libelf, so that readers can know more clearly what the internal implementation is

In other words, you asked this question because you don't know what happened in modlib, so libelf is a more suitable name

@acassis
Copy link
Contributor

acassis commented Feb 26, 2025

@anchao could you please verify, it is failing in the CI

@anchao anchao changed the title libc/elf: rename modlib to elf [BREAKING]libc/elf: rename modlib to elf Mar 3, 2025
@anchao
Copy link
Contributor Author

anchao commented Mar 3, 2025

@anchao could you please verify, it is failing in the CI

Thanks @acassis , this PR depends on apps repo apache/nuttx#15765, so it may not be possible to pass CI separately

Copy link
Contributor

@hartmannathan hartmannathan left a comment

Choose a reason for hiding this comment

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

Could there be an email to the mailing list to point out the (proposed) breaking change? Some members of the community don't monitor GitHub. Thanks!

@anchao
Copy link
Contributor Author

anchao commented Apr 7, 2025

Could there be an email to the mailing list to point out the (proposed) breaking change? Some members of the community don't monitor GitHub. Thanks!

@hartmannathan Thanks for your suggestion, I sent an email to the dev list

@lupyuen lupyuen marked this pull request as draft April 7, 2025 09:52
@lupyuen
Copy link
Member

lupyuen commented Apr 7, 2025

We'll change "Draft Mode" to "Ready for Review" after the voting. Thanks :-)

Renaming "modlib" to "libelf" is more in line with the implementation content,
which makes it easier for individual developers to understand the capabilities of this module.

CONFIG_LIBC_MODLIB -> CONFIG_LIBC_ELF

Signed-off-by: chao an <[email protected]>
@xiaoxiang781216 xiaoxiang781216 marked this pull request as ready for review April 10, 2025 03:52
Copy link
Contributor

@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 @anchao for your work, the additional information, and discussion/vote on the mailing list! :-)

Copy link
Member

@lupyuen lupyuen left a comment

Choose a reason for hiding this comment

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

@anchao Please close the voting. Thanks :-)

This Breaking PR compiles with the NuttX Breaking Changes Handling Process:

@anchao
Copy link
Contributor Author

anchao commented Apr 11, 2025

@anchao Please close the voting. Thanks :-)

This Breaking PR compiles with the NuttX Breaking Changes Handling Process:

@lupyuen Done, thank you very much for the extra work on this PR.

@lupyuen lupyuen merged commit 27846ff into apache:master Apr 11, 2025
12 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Benchmarks Area: Examples Area: Tools breaking change This change requires a mitigation entry in the release notes. Size: S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants