Re: [PR] Added a small usability improve for Unix.mk savedefconfig [nuttx]
xiaoxiang781216 commented on PR #16312: URL: https://github.com/apache/nuttx/pull/16312#issuecomment-2943041915 > Re-requested review just to ask, what should be done with this PR then 😄 Should I refactor it somehow or can we merge it? @anchao do you have more concern? -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Added a small usability improve for Unix.mk savedefconfig [nuttx]
ValentiWorkLearning commented on PR #16312: URL: https://github.com/apache/nuttx/pull/16312#issuecomment-2942915387 Re-requested review just to ask, what should be done with this PR then 😄 Should I refactor it somehow or can we merge 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Added a small usability improve for Unix.mk savedefconfig [nuttx]
acassis commented on code in PR #16312: URL: https://github.com/apache/nuttx/pull/16312#discussion_r2083497065 ## tools/Unix.mk: ## @@ -768,6 +768,7 @@ savedefconfig: apps_preconfig $(Q) rm -f warning.tmp $(Q) rm -f defconfig.tmp $(Q) rm -f sortedconfig.tmp + $(info "The defconfig was generated successfully at $(PWD)/defconfig .If you're building out-of-tree configuration, then don't forget to copy it into your defconfig original location") Review Comment: > @acassis please think about when users would want to do the **savedefconfig**. Could it be to save a copy? I don't think so, What they actually want is to replace the original version. @anchao I think we are talking about different things. Normally users create a new config using an old one as base. But of course, it is possible to change the way the do it, now instead of using boardname:nsh as start point the need first to copy the "nsh" board profile to a new place (i.e. "newbrdcfg") and the run boardname:newbrdcfg I think this approach is ok, we just need to change the way normally people do it (as I described previously) -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Added a small usability improve for Unix.mk savedefconfig [nuttx]
anchao commented on code in PR #16312: URL: https://github.com/apache/nuttx/pull/16312#discussion_r2083495087 ## tools/Unix.mk: ## @@ -768,6 +768,7 @@ savedefconfig: apps_preconfig $(Q) rm -f warning.tmp $(Q) rm -f defconfig.tmp $(Q) rm -f sortedconfig.tmp + $(info "The defconfig was generated successfully at $(PWD)/defconfig .If you're building out-of-tree configuration, then don't forget to copy it into your defconfig original location") Review Comment: > @anchao your point makes sense, but then it should be important to have an option to not replace the original defconfig, because it will add a more steps for developers creating new profiles: currently after the "cmake savedefconfig" they will need: cp boards/xxx/xxx//configs/nsh/defconfig /backup/configs/ git checkout boards/xxx/xxx//configs/nsh/defconfig mkdir boards/xxx/xxx//configs/newbrdcfg/ cp /backup/configs/defconfig boards/xxx/xxx//configs/newbrdcfg/ > > Compared with current way using "make savedefconfig": mkdir boards/xxx/xxx//configs/newbrdcfg/ cp defconfig boards/xxx/xxx//configs/newbrdcfg/ Regarding first case, why not back up the old version first? -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Added a small usability improve for Unix.mk savedefconfig [nuttx]
anchao commented on code in PR #16312: URL: https://github.com/apache/nuttx/pull/16312#discussion_r2083493875 ## tools/Unix.mk: ## @@ -768,6 +768,7 @@ savedefconfig: apps_preconfig $(Q) rm -f warning.tmp $(Q) rm -f defconfig.tmp $(Q) rm -f sortedconfig.tmp + $(info "The defconfig was generated successfully at $(PWD)/defconfig .If you're building out-of-tree configuration, then don't forget to copy it into your defconfig original location") Review Comment: @acassis please think about when users would want to do the **savedefconfig**. Could it be to save a copy? I don't think so, What they actually want is to replace the original version. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Added a small usability improve for Unix.mk savedefconfig [nuttx]
acassis commented on code in PR #16312: URL: https://github.com/apache/nuttx/pull/16312#discussion_r2083478973 ## tools/Unix.mk: ## @@ -768,6 +768,7 @@ savedefconfig: apps_preconfig $(Q) rm -f warning.tmp $(Q) rm -f defconfig.tmp $(Q) rm -f sortedconfig.tmp + $(info "The defconfig was generated successfully at $(PWD)/defconfig .If you're building out-of-tree configuration, then don't forget to copy it into your defconfig original location") Review Comment: @anchao your point makes sense, but then it should be important to have an option to not replace the original defconfig, because it will add a more steps for developers creating new profiles: currently after the "cmake savedefconfig" they will need: cp boards/xxx/xxx//configs/nsh/defconfig /backup/configs/ git checkout boards/xxx/xxx//configs/nsh/defconfig mkdir boards/xxx/xxx//configs/newbrdcfg/ cp /backup/configs/defconfig boards/xxx/xxx//configs/newbrdcfg/ Compared with current way using "make savedefconfig": mkdir boards/xxx/xxx//configs/newbrdcfg/ cp defconfig boards/xxx/xxx//configs/newbrdcfg/ -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Added a small usability improve for Unix.mk savedefconfig [nuttx]
anchao commented on code in PR #16312: URL: https://github.com/apache/nuttx/pull/16312#discussion_r2083380505 ## tools/Unix.mk: ## @@ -768,6 +768,7 @@ savedefconfig: apps_preconfig $(Q) rm -f warning.tmp $(Q) rm -f defconfig.tmp $(Q) rm -f sortedconfig.tmp + $(info "The defconfig was generated successfully at $(PWD)/defconfig .If you're building out-of-tree configuration, then don't forget to copy it into your defconfig original location") Review Comment: > @anchao what is the goal of this cmake autosave? If I understood correctly when user run "make savedefconfig" it will replace the original defconfig for current board profile. Although this behavior seems good at first glance, the reason because Greg didn't implement this way it because normally the developer will use a base board:nsh profile to modify and create a local defconfig that he/she will move to a new configs/namedir that will be created. I think the cmake autosave should require a flag to force this behavior, something like "make -f savedefconfig". Maybe @patacongo could give more details here. My view is straightforward: change it in a way that is most convenient for developers. Not replacing the original defconfig with savedefconfig was reasonable before the emergence of git, developers did not have diff tools to check for differences between their work and original config After git, if the original configuration is directly replaced by savedefconfig, developers can easily use git diff to check whether the changes as expectations. If not, they only need to use git checkout to restore the file. I think for 90% of individual developers, this approach will enhance their efficiency. Furthermore, this improvement could also shorten the CI-CD time, and the process of refresh.sh could be optimized. **refresh.sh (6s)** ``` archer@archer:~/code/nuttx/n6/nuttx$ time ./tools/refresh.sh --silent sim/nsh [1/1] Normalize sim/nsh real 0m6.049s user 0m3.137s sys 0m1.988s ``` **autosave (1.5s)** ``` archer@archer:~/code/nuttx/n6/nuttx$ time make savedefconfig Loaded configuration '.config' Minimal configuration saved to 'defconfig.tmp' real 0m1.363s user 0m0.802s sys 0m0.422s ``` -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Added a small usability improve for Unix.mk savedefconfig [nuttx]
acassis commented on code in PR #16312: URL: https://github.com/apache/nuttx/pull/16312#discussion_r2083365876 ## tools/Unix.mk: ## @@ -768,6 +768,7 @@ savedefconfig: apps_preconfig $(Q) rm -f warning.tmp $(Q) rm -f defconfig.tmp $(Q) rm -f sortedconfig.tmp + $(info "The defconfig was generated successfully at $(PWD)/defconfig .If you're building out-of-tree configuration, then don't forget to copy it into your defconfig original location") Review Comment: @anchao what is the goal of this cmake autosave? If I understood correctly when user run "make savedefconfig" it will replace the original defconfig for current board profile. Although this behavior seems good at first glance, the reason because Greg didn't implement this way it because normally the developer will use a base board:nsh profile to modify and create a local defconfig that he/she will move to a new configs/namedir that will be created. I think the cmake autosave should require a flag to force this behavior, something like "make -f savedefconfig". Maybe @patacongo could give more details 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Added a small usability improve for Unix.mk savedefconfig [nuttx]
anchao commented on code in PR #16312: URL: https://github.com/apache/nuttx/pull/16312#discussion_r2074361455 ## tools/Unix.mk: ## @@ -768,6 +768,7 @@ savedefconfig: apps_preconfig $(Q) rm -f warning.tmp $(Q) rm -f defconfig.tmp $(Q) rm -f sortedconfig.tmp + $(info "The defconfig was generated successfully at $(PWD)/defconfig .If you're building out-of-tree configuration, then don't forget to copy it into your defconfig original location") Review Comment: There is no standard that specifies the behavior of savedefconfig. I think the change is worth if the improvement could make developers more efficient, -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Added a small usability improve for Unix.mk savedefconfig [nuttx]
ValentiWorkLearning commented on code in PR #16312: URL: https://github.com/apache/nuttx/pull/16312#discussion_r2073584351 ## tools/Unix.mk: ## @@ -768,6 +768,7 @@ savedefconfig: apps_preconfig $(Q) rm -f warning.tmp $(Q) rm -f defconfig.tmp $(Q) rm -f sortedconfig.tmp + $(info "The defconfig was generated successfully at $(PWD)/defconfig .If you're building out-of-tree configuration, then don't forget to copy it into your defconfig original location") Review Comment: @anchao hm.. That's good idea. It seems that this flow has to be same for other tools(buildroot for instance). So, technically make savedefconfig should directly write to the original defconfig file. Would this be the expected behaviour? -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
