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/01/05 04:48:55 UTC

[GitHub] [incubator-nuttx] yamt opened a new pull request #2654: Always specify an extension for "sed -i"

yamt opened a new pull request #2654:
URL: https://github.com/apache/incubator-nuttx/pull/2654


   ## Summary
   An empty extension for sed -i is not widely available.
   
   * Where it isn't available (eg. macOS's sed) "sed -i -e" will
     create a backup file with the "-e" suffix. It isn't our intention.
   
   * Even GNU sed documentation says it's "not recommended".
   
   ## Impact
   
   ## Testing
   


----------------------------------------------------------------
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] xiaoxiang781216 commented on a change in pull request #2654: Always specify an extension for "sed -i"

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2654:
URL: https://github.com/apache/incubator-nuttx/pull/2654#discussion_r551729511



##########
File path: boards/risc-v/nr5m100/nr5m100-nexys4/README.txt
##########
@@ -143,7 +143,7 @@ Debugger
      shadowed variable warnings, so disable the -Werror flag also:
 
      cd openocd
-     sed -i 's/ -Werror//g' configure.ac
+     sed -i.bak 's/ -Werror//g' configure.ac

Review comment:
       don't need to moidfy README.txt?




----------------------------------------------------------------
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] jerpelea merged pull request #2654: Deal with "sed -i" portability

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


   


----------------------------------------------------------------
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] btashton commented on a change in pull request #2654: Deal with "sed -i" portability

Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2654:
URL: https://github.com/apache/incubator-nuttx/pull/2654#discussion_r551760293



##########
File path: tools/configure.sh
##########
@@ -296,7 +296,7 @@ done
 
 if [ "X${defappdir}" = "Xy" ]; then
   # In-place edit can mess up permissions on Windows
-  # sed -i -e "/^CONFIG_APPS_DIR/d" "${dest_config}"

Review comment:
       > maybe this instance can be replaced with kconfig-tweak too
   
   Looks like the commented section can be removed and the next line updated. It was coping to a temporary file editing via sed and moving back. 




----------------------------------------------------------------
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] btashton commented on a change in pull request #2654: Always specify an extension for "sed -i"

Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2654:
URL: https://github.com/apache/incubator-nuttx/pull/2654#discussion_r551716159



##########
File path: tools/Makefile.unix
##########
@@ -505,7 +505,7 @@ gconfig: apps_preconfig
 
 savedefconfig: apps_preconfig
 	$(Q) APPSDIR=${CONFIG_APPS_DIR} EXTERNALDIR=$(EXTERNALDIR) kconfig-conf --savedefconfig defconfig.tmp Kconfig
-	$(Q) sed -i -e "/CONFIG_APPS_DIR=/d" defconfig.tmp

Review comment:
       Can we just use kconfig-tweak for the config patches?

##########
File path: tools/configure.sh
##########
@@ -296,7 +296,7 @@ done
 
 if [ "X${defappdir}" = "Xy" ]; then
   # In-place edit can mess up permissions on Windows
-  # sed -i -e "/^CONFIG_APPS_DIR/d" "${dest_config}"

Review comment:
       Commented out seems like we can just drop this

##########
File path: tools/testbuild.sh
##########
@@ -244,11 +244,11 @@ function configure {
     varname=`echo $setting | cut -d'=' -f1`
     if [ ! -z "$varname" ]; then
       echo "  Disabling $varname"
-      sed -i -e "/$varname/d" $nuttx/.config

Review comment:
       kconfig tweak would be better here as well




----------------------------------------------------------------
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] yamt commented on a change in pull request #2654: Always specify an extension for "sed -i"

Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2654:
URL: https://github.com/apache/incubator-nuttx/pull/2654#discussion_r551747059



##########
File path: boards/risc-v/nr5m100/nr5m100-nexys4/README.txt
##########
@@ -143,7 +143,7 @@ Debugger
      shadowed variable warnings, so disable the -Werror flag also:
 
      cd openocd
-     sed -i 's/ -Werror//g' configure.ac
+     sed -i.bak 's/ -Werror//g' configure.ac

Review comment:
       why not?




----------------------------------------------------------------
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] yamt commented on a change in pull request #2654: Deal with "sed -i" portability

Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2654:
URL: https://github.com/apache/incubator-nuttx/pull/2654#discussion_r551758572



##########
File path: tools/testbuild.sh
##########
@@ -244,11 +244,11 @@ function configure {
     varname=`echo $setting | cut -d'=' -f1`
     if [ ! -z "$varname" ]; then
       echo "  Disabling $varname"
-      sed -i -e "/$varname/d" $nuttx/.config

Review comment:
       done

##########
File path: tools/Makefile.unix
##########
@@ -505,7 +505,7 @@ gconfig: apps_preconfig
 
 savedefconfig: apps_preconfig
 	$(Q) APPSDIR=${CONFIG_APPS_DIR} EXTERNALDIR=$(EXTERNALDIR) kconfig-conf --savedefconfig defconfig.tmp Kconfig
-	$(Q) sed -i -e "/CONFIG_APPS_DIR=/d" defconfig.tmp

Review comment:
       done




----------------------------------------------------------------
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] yamt commented on pull request #2654: Always specify an extension for "sed -i"

Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2654:
URL: https://github.com/apache/incubator-nuttx/pull/2654#issuecomment-754440010


   > @yamt can you merge all patch into one? it's important to keep the history clean without the intermediate commit
   
   i will merge them later


----------------------------------------------------------------
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] xiaoxiang781216 commented on a change in pull request #2654: Deal with "sed -i" portability

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2654:
URL: https://github.com/apache/incubator-nuttx/pull/2654#discussion_r551760041



##########
File path: tools/configure.sh
##########
@@ -296,7 +296,7 @@ done
 
 if [ "X${defappdir}" = "Xy" ]; then
   # In-place edit can mess up permissions on Windows
-  # sed -i -e "/^CONFIG_APPS_DIR/d" "${dest_config}"

Review comment:
       The change is same as before so, we can't use kconfig-tweak here?




----------------------------------------------------------------
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] yamt commented on a change in pull request #2654: Deal with "sed -i" portability

Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2654:
URL: https://github.com/apache/incubator-nuttx/pull/2654#discussion_r551758507



##########
File path: tools/configure.sh
##########
@@ -296,7 +296,7 @@ done
 
 if [ "X${defappdir}" = "Xy" ]; then
   # In-place edit can mess up permissions on Windows
-  # sed -i -e "/^CONFIG_APPS_DIR/d" "${dest_config}"

Review comment:
       maybe this instance can be replaced with kconfig-tweak too




----------------------------------------------------------------
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] btashton commented on a change in pull request #2654: Deal with "sed -i" portability

Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2654:
URL: https://github.com/apache/incubator-nuttx/pull/2654#discussion_r551760001



##########
File path: tools/configure.sh
##########
@@ -296,7 +296,7 @@ done
 
 if [ "X${defappdir}" = "Xy" ]; then
   # In-place edit can mess up permissions on Windows
-  # sed -i -e "/^CONFIG_APPS_DIR/d" "${dest_config}"

Review comment:
       Looks like the commented section can be removed and the next line updated. It was coping to a temporary file editing via sed and moving back. 




----------------------------------------------------------------
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] xiaoxiang781216 commented on a change in pull request #2654: Always specify an extension for "sed -i"

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2654:
URL: https://github.com/apache/incubator-nuttx/pull/2654#discussion_r551755975



##########
File path: boards/risc-v/nr5m100/nr5m100-nexys4/README.txt
##########
@@ -143,7 +143,7 @@ Debugger
      shadowed variable warnings, so disable the -Werror flag also:
 
      cd openocd
-     sed -i 's/ -Werror//g' configure.ac
+     sed -i.bak 's/ -Werror//g' configure.ac

Review comment:
       I think it's a demo, but anyway it's fine to modify it.




----------------------------------------------------------------
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] yamt commented on pull request #2654: Deal with "sed -i" portability

Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2654:
URL: https://github.com/apache/incubator-nuttx/pull/2654#issuecomment-754457877


   > > @yamt can you merge all patch into one? it's important to keep the history clean without the intermediate commit
   > 
   > i will merge them later
   
   done


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