You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/04/03 04:12:51 UTC

[GitHub] [incubator-nuttx-apps] swkim101 opened a new pull request #647: nshlib: Disable mb, mh, and mw by default

swkim101 opened a new pull request #647:
URL: https://github.com/apache/incubator-nuttx-apps/pull/647


   ## Summary
   
   - Fix https://github.com/apache/incubator-nuttx/issues/3011
   
   ## Impact
   
   ## Testing
   
   ![image](https://user-images.githubusercontent.com/72803908/113467429-eba1bb80-947d-11eb-9818-ed7416a90ea5.png)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx-apps] v01d edited a comment on pull request #647: nshlib: Disable mb, mh, and mw by default

Posted by GitBox <gi...@apache.org>.
v01d edited a comment on pull request #647:
URL: https://github.com/apache/incubator-nuttx-apps/pull/647#issuecomment-812876361


   ~~@xiaoxiang781216 as @btashton mentioned, the Kconfig change is backwards to the intent of the PR. I don't think this should have been merged.~~ Ah, I was confused by @btashton command the fact that this is enabling the disable of the commands. I'm not sure if the CI errors were up to date though.
   
   Also, I see quite a few issues such as:
   ```
   < CONFIG_NSH_DISABLE_MB=y
   < CONFIG_NSH_DISABLE_MH=y
   63d60
   < CONFIG_NSH_DISABLE_MW=y
   ```
   in CI.
   
   Let's try not to make a habit of skipping CI (it is now quite faster and less wasteful when pushing again) as we will loose unexpected errors.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #647: nshlib: Disable mb, mh, and mw by default

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #647:
URL: https://github.com/apache/incubator-nuttx-apps/pull/647#issuecomment-812818855


   @swkim101 you need remove all CONFIG_NSH_DISABLE_M[B|H|W]=y from nuttx/boards/ to fix the error reported here:
   https://github.com/apache/incubator-nuttx-apps/pull/647/checks?check_run_id=2258212861


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx-apps] Ouss4 commented on pull request #647: nshlib: Disable mb, mh, and mw by default

Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #647:
URL: https://github.com/apache/incubator-nuttx-apps/pull/647#issuecomment-812825176


   > That PR does the opposite it need to remove all the explicit disables since this PR makes it the default
   
   Ah you're right, I spoke too fast.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx-apps] btashton commented on pull request #647: nshlib: Disable mb, mh, and mw by default

Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #647:
URL: https://github.com/apache/incubator-nuttx-apps/pull/647#issuecomment-812889682


   The os repo PR was updated to just remove these entries instead of add them which is what we needed, any CI issue should have just been a matter of the order they were merged. 
   
   But yeah generally I agree we should wait on CI. Sometimes I merge if it's obvious not going to have an issue and I'm trying to unblock things.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx-apps] btashton commented on pull request #647: nshlib: Disable mb, mh, and mw by default

Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #647:
URL: https://github.com/apache/incubator-nuttx-apps/pull/647#issuecomment-812824893


   That PR does the opposite it need to remove all the explicit disables since this PR makes it the default


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx-apps] v01d commented on pull request #647: nshlib: Disable mb, mh, and mw by default

Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #647:
URL: https://github.com/apache/incubator-nuttx-apps/pull/647#issuecomment-812876361


   @xiaoxiang781216 as @btashton mentioned, the Kconfig change is backwards to the intent of the PR. I don't think this should have been merged.
   
   Also, I see quite a few issues such as:
   ```
   < CONFIG_NSH_DISABLE_MB=y
   < CONFIG_NSH_DISABLE_MH=y
   63d60
   < CONFIG_NSH_DISABLE_MW=y
   ```
   in CI.
   
   Let's try not to make a habit of skipping CI (it is now quite faster and less wasteful when pushing again) as we will loose unexpected errors.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx-apps] xiaoxiang781216 merged pull request #647: nshlib: Disable mb, mh, and mw by default

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #647:
URL: https://github.com/apache/incubator-nuttx-apps/pull/647


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx-apps] Ouss4 commented on pull request #647: nshlib: Disable mb, mh, and mw by default

Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #647:
URL: https://github.com/apache/incubator-nuttx-apps/pull/647#issuecomment-812824616


   > @swkim101 you need remove all CONFIG_NSH_DISABLE_M[B|H|W]=y from nuttx/boards/ to fix the error reported here:
   > https://github.com/apache/incubator-nuttx-apps/pull/647/checks?check_run_id=2258212861
   
   There is this PR https://github.com/apache/incubator-nuttx/pull/3240, but it looks like it addresses only NSH configs. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #647: nshlib: Disable mb, mh, and mw by default

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #647:
URL: https://github.com/apache/incubator-nuttx-apps/pull/647#issuecomment-812879402


   > @xiaoxiang781216 as @btashton mentioned, the Kconfig change is backwards to the intent of the PR. I don't think this should have been merged.
   > 
   
   This patch is needed to make the default option more safer.
   
   > Also, I see quite a few issues such as:
   > 
   > ```
   > < CONFIG_NSH_DISABLE_MB=y
   > < CONFIG_NSH_DISABLE_MH=y
   > 63d60
   > < CONFIG_NSH_DISABLE_MW=y
   > ```
   > 
   
   It's strange that I can't find these words in the latest nuttx when I want to fix the issue.
   
   > in CI.
   > 
   > Let's try not to make a habit of skipping CI (it is now quite faster and less wasteful when pushing again) as we will loose unexpected errors.
   
   Ok.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #647: nshlib: Disable mb, mh, and mw by default

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #647:
URL: https://github.com/apache/incubator-nuttx-apps/pull/647#issuecomment-812874370


   The change is minor, and the dependence was merged. Let's merge it directly to save the build resource.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org