Skip to content

apps/Makefile: Support passing additional arguments to mksymtab.sh#2637

Merged
xiaoxiang781216 merged 2 commits into
apache:masterfrom
JianyuWang0623:br_wjy_mksymtab_SYMTABADDS_0930
Oct 7, 2024
Merged

apps/Makefile: Support passing additional arguments to mksymtab.sh#2637
xiaoxiang781216 merged 2 commits into
apache:masterfrom
JianyuWang0623:br_wjy_mksymtab_SYMTABADDS_0930

Conversation

@JianyuWang0623
Copy link
Copy Markdown
Contributor

@JianyuWang0623 JianyuWang0623 commented Sep 30, 2024

Summary

  1. tools/mksymtab.sh: Use getopts for parameter parsing
  2. apps/Makefile: Passing additional arguments to mksymtab

Impact

Usage of mksymtab.sh about param "additionalsymbolspath" changed, has checked by searching in nuttx-apps and nuttx.
"additionalsymbolspath" should not depends on "symtabprefix".

# Without this PR
usage="Usage: $0 <imagedirpath> [symtabprefix [additionalsymbolspath]]"
# With this PR
usage="Usage: $0 <imagedirpath> [symtabprefix] [-a additionalsymbolspath]"

Testing

  1. Selftest result attached to commit message
  2. Github NuttX CI

@nuttxpr
Copy link
Copy Markdown

nuttxpr commented Sep 30, 2024

[Experimental Bot, please feedback here]

The PR appears to meet most NuttX requirements, but some clarifications and additions are needed:

Summary:

  • Why is the change necessary? The summary should state the reason for changing the parameter parsing. Is it for consistency, clarity, or to fix a bug?
  • What functional part is changed? Be more specific than "tools/mksymtab.sh". Mention the parameter parsing logic within the script.

Impact:

  • Impact on the user: While you mention the usage change, specify if users need to modify any scripts or commands using mksymtab.sh. A simple "YES" with a clear explanation of the required adaptation is better than implicitly assuming the user understands the impact from the code snippet.

Testing:

  • Build Host(s): Specify the operating system, CPU architecture, and compiler used for testing. Don't just say "Github NuttX CI" - list the specific configurations tested.
  • Target(s): Provide details about the target architecture(s) and boards used for verification.
  • Testing logs: Instead of just stating "Selftest result attached to commit message," include a summarized excerpt of the relevant log messages demonstrating the change's effectiveness. This provides immediate context within the PR.

By addressing these points, you'll make your PR easier to review and merge.

@JianyuWang0623 JianyuWang0623 force-pushed the br_wjy_mksymtab_SYMTABADDS_0930 branch 3 times, most recently from 631686c to 8e5581e Compare October 6, 2024 11:08
@JianyuWang0623
Copy link
Copy Markdown
Contributor Author

Using getopt may be simpler (e.g. the first version of this PR), but getopt is not compatible between GNU and macOS, see:

  • util-linux:GETOPT(1)
  • macOS:GETOPT(1)

@JianyuWang0623 JianyuWang0623 marked this pull request as ready for review October 6, 2024 14:35
@JianyuWang0623 JianyuWang0623 marked this pull request as draft October 6, 2024 14:35
@JianyuWang0623 JianyuWang0623 marked this pull request as ready for review October 6, 2024 15:03
Comment thread tools/mksymtab.sh Outdated
Comment thread tools/mksymtab.sh Outdated
Comment thread tools/mksymtab.sh Outdated
Comment thread tools/mksymtab.sh Outdated
Comment thread Makefile Outdated
Comment thread Makefile Outdated
Use the "-a" option to specify additional lists

Examples
  - The basic.txt
    $ cat basic.txt
    basic_func0
    basic_func1
    basic_func2

  - The additional.txt
    $ cat additional.txt
    additional_func0
    additional_func1
    additional_func2

  1. Get symbols from directory "EMPTY_DIR" and additional list basic.txt
    ./tools/mksymtab.sh ./EMPTY_DIR -a basic.txt
    #if defined(CONFIG_EXECFUNCS_HAVE_SYMTAB)
    const struct symtab_s CONFIG_EXECFUNCS_SYMTAB_ARRAY[] =
    #elif defined(CONFIG_NSH_SYMTAB)
    const struct symtab_s CONFIG_NSH_SYMTAB_ARRAYNAME[] =
    #else
    const struct symtab_s dummy_symtab[] =
    #endif
    {
      {"basic_func0", &basic_func0},
      {"basic_func1", &basic_func1},
      {"basic_func2", &basic_func2},
    };

  2. Get symbols from directory "EMPTY_DIR" and two additional lists basic.txt, additional.txt
    ./tools/mksymtab.sh ./EMPTY_DIR -a basic.txt -a additional.txt
    #if defined(CONFIG_EXECFUNCS_HAVE_SYMTAB)
    const struct symtab_s CONFIG_EXECFUNCS_SYMTAB_ARRAY[] =
    #elif defined(CONFIG_NSH_SYMTAB)
    const struct symtab_s CONFIG_NSH_SYMTAB_ARRAYNAME[] =
    #else
    const struct symtab_s dummy_symtab[] =
    #endif
    {
      {"additional_func0", &additional_func0},
      {"additional_func1", &additional_func1},
      {"additional_func2", &additional_func2},
      {"basic_func0", &basic_func0},
      {"basic_func1", &basic_func1},
      {"basic_func2", &basic_func2},
    };

  3. Set prefix and get symbols from directory "EMPTY_DIR" and two additional lists basic.txt, additional.txt
    ./tools/mksymtab.sh ./EMPTY_DIR PREFIX_TEST  -a basic.txt -a additional.txt
    const struct symtab_s PREFIX_TEST_exports[] =
    {
      {"additional_func0", &additional_func0},
      {"additional_func1", &additional_func1},
      {"additional_func2", &additional_func2},
      {"basic_func0", &basic_func0},
      {"basic_func1", &basic_func1},
      {"basic_func2", &basic_func2},
    };

  4. Error: Missing <imagedirpath>
    $ ./tools/mksymtab.sh
    ERROR: Missing <imagedirpath>

    Usage: ./tools/mksymtab.sh <imagedirpath> [symtabprefix] [-a additionalsymbolspath]

UNSUPPORTED usage examples
  # `getopt` supports these, but the usage in GNU and macOS is incompatible.
  - ./tools/mksymtab.sh ./EMPTY_DIR -a basic.txt PREFIX_TEST -a additional.txt
  - ./tools/mksymtab.sh -a basic.txt ./EMPTY_DIR PREFIX_TEST -a additional.txt

References
  BASH(1)   -- getopts
  GETOPT(1) -- getopt

Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
@JianyuWang0623 JianyuWang0623 force-pushed the br_wjy_mksymtab_SYMTABADDS_0930 branch from 8e5581e to f92a0a8 Compare October 6, 2024 22:02
Copy link
Copy Markdown
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 @JianyuWang0623 :-)

JianyuWang0623 added a commit to JianyuWang0623/nuttx-apps that referenced this pull request Oct 7, 2024
apache#2637

For example, now can passing file(s) contain additional symbols that may be used in the future to mksymtab.sh by using "-a" option to avoid freuent kernel rebuilds:
SYMTABLST="-a $(XXXX_PREFIX)/platform/nuttx/exports_xxxx_api.txt"
ifneq ($(CONFIG_LIBM),)
    SYMTABLST+="-a $(XXXX_PREFIX)/platform/nuttx/exports_xxxx_api_math.txt"
endif

Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
@JianyuWang0623 JianyuWang0623 force-pushed the br_wjy_mksymtab_SYMTABADDS_0930 branch from f92a0a8 to e5cd959 Compare October 7, 2024 06:14
apache#2637

For example, now can passing file(s) contain additional symbols that may be used in the future to mksymtab.sh by using "-a" option to avoid freuent kernel rebuilds:
SYMTABEXT="-a $(XXXX_PREFIX)/platform/nuttx/exports_xxxx_api.txt"
ifneq ($(CONFIG_LIBM),)
    SYMTABEXT+="-a $(XXXX_PREFIX)/platform/nuttx/exports_xxxx_api_math.txt"
endif

Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
@JianyuWang0623 JianyuWang0623 force-pushed the br_wjy_mksymtab_SYMTABADDS_0930 branch from e5cd959 to f1ad62a Compare October 7, 2024 06:18
@xiaoxiang781216 xiaoxiang781216 merged commit 58e5bc7 into apache:master Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants