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

Fix warning and depends of examples/sotest #2576

Merged
merged 2 commits into from
Sep 18, 2024

Conversation

JianyuWang0623
Copy link
Contributor

@JianyuWang0623 JianyuWang0623 commented Sep 14, 2024

Summary

  1. Fix warning of unused variable 'devname' - f876f3b
  2. Fix depends of SOTEST_BUILTINFS - c7418f1

Please see commit message for more details.

Impact

examples/sotest

Testing

  • Configuration
./tools/configure.sh -l sim:sotest
  • Run
nsh> sotest
main: Registering romdisk at /dev/ram3
main: Mounting ROMFS filesystem at target=/mnt/romfs with source=/dev/ram3
module_initialize:
module_initialize:
testfunc1: Hello, everyone!
   caller: Hello to you too!
testfunc2: Hope you are having a great day!
   caller: Not so bad so far.
testfunc3: Let's talk again very soon
   caller: Yes, don't be a stranger!
module_uninitialize: arg=0
module_uninitialize: arg=0
nsh>

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 @JianyuWang0623 :-)

  • There are too many unrelated changes in this PR, please provide separate dedicated PR for each functional change / area.
  • Please remember to provide descriptive git commit messages as well as PR descriptions.

nshlib/nsh_fscmds.c Outdated Show resolved Hide resolved
tools/mksymtab.sh Outdated Show resolved Hide resolved
@cederom
Copy link

cederom commented Sep 15, 2024

Sorry, but this is a perfect example of really terrible PR:

  • Various changes for different functional areas are introduced in one single PR.
  • There is zero description on why change is introduced what it does and why (both in commit message and the PR description).
  • Impact description in PR misses key points on global build process and other changes that PR would leave after it was merged.
  • Testing description in PR does not exist, provides no test logs, nothing.
  • We cannot accept this kind of PRs.

I will not close this PR so we could have a discussion and work out better PR submission in future.

@patacongo
Copy link
Contributor

I will not close this PR so we could have a discussion and work out better PR submission in future.

Marking it as Draft will prevent any accidental commits until you discussion is finished.

@patacongo
Copy link
Contributor

  • There are too many unrelated changes in this PR, please provide separate dedicated PR for each functional change / area.

There are 8 commits, so the changes have been contained in separate commits, just provided under one PR. So I don't think there is a major problem with the github content after the merge. It is just harder to review when they are combined (but each commit can also be reviewed separately).

Doing things in a consistent way is, however, also important.

@cederom cederom marked this pull request as draft September 16, 2024 00:31
@Donny9
Copy link
Contributor

Donny9 commented Sep 16, 2024

@JianyuWang0623 please split mutli PR~

@cederom
Copy link

cederom commented Sep 16, 2024

I think that PR review process needs some improvements, as we get more and more nice PRs, some updates seem necessary also on our side - to provide reporters with basic information that are required in each PR here goes the template update proposition (apache/nuttx#13494). Your feedback is much appreciated! Thank you! :-)

@patacongo
Copy link
Contributor

You should be aware of these

commit a59ae5536c58510a26e5a64868ea0705d1eb1c2b
Author: David Sidrane <[email protected]>
Date:   Sat Apr 18 05:00:11 2020 -0700

    github: Add PR Template

commit bd7217e21ede8a5f42cd5c63a68f68bffece15df
Author: Gregory Nutt <[email protected]>
Date:   Tue Apr 21 08:18:31 2020 -0600

    Remove boilerplate from the PR template.

    So far, the use of the new pull request template has been disastrous.  People are ignoring the writing instructions the template is polluting the PR history.  This change just removes the boilerplate and writing instructions fromt he template.  We don't need to see this unmodified serveral times per day.

All bad memories

Log:
  sotest_main.c: In function 'sotest_main':
  sotest_main.c:116:29: error: storage size of 'desc' isn't known
    116 |   struct boardioc_romdisk_s desc;
        |                             ^~~~
  sotest_main.c:116:29: error: unused variable 'desc' [-Werror=unused-variable]
  cc1: all warnings being treated as errors

Signed-off-by: wangjianyu3 <[email protected]>
Config:
  +CONFIG_EXAMPLES_SOTEST=y
  +CONFIG_EXAMPLES_SOTEST_BINDIR="/data"
Log:
  sotest_main.c: In function 'sotest_main':
  sotest_main.c:105:8: error: unused variable 'devname' [-Werror=unused-variable]
    105 |   char devname[32];
        |        ^~~~~~~

Signed-off-by: wangjianyu3 <[email protected]>
@JianyuWang0623 JianyuWang0623 changed the title sotest and mksymtab.sh Fix warning and depends of examples/sotest Sep 17, 2024
@JianyuWang0623
Copy link
Contributor Author

@cederom @patacongo @Donny9 PR has been splited, and that has comments will be linked here.

@JianyuWang0623 JianyuWang0623 marked this pull request as ready for review September 17, 2024 12:53
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 @JianyuWang0623 :-) Smaller and well described changes are much better to process :-)

examples/sotest/sotest_main.c Show resolved Hide resolved
@xiaoxiang781216 xiaoxiang781216 merged commit 758ed55 into apache:master Sep 18, 2024
27 checks passed
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.

5 participants