Re: [PR] espressif: Add variable to override MCUboot version and URL [nuttx]
xiaoxiang781216 merged PR #16320: URL: https://github.com/apache/nuttx/pull/16320 -- 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] espressif: Add variable to override MCUboot version and URL [nuttx]
tmedicci commented on code in PR #16320: URL: https://github.com/apache/nuttx/pull/16320#discussion_r2075788287 ## arch/risc-v/src/common/espressif/Bootloader.mk: ## @@ -35,8 +35,14 @@ BOOTLOADER_CONFIG= $(BOOTLOADER_SRCDIR)/bootloader.conf MCUBOOT_SRCDIR = $(BOOTLOADER_SRCDIR)/mcuboot MCUBOOT_ESPDIR = $(MCUBOOT_SRCDIR)/boot/espressif -MCUBOOT_URL= https://github.com/mcu-tools/mcuboot MCUBOOT_TOOLCHAIN = $(TOOLSDIR)/mcuboot_toolchain_espressif.cmake +ifndef MCUBOOT_VERSION + MCUBOOT_VERSION = $(CONFIG_ESPRESSIF_MCUBOOT_VERSION) Review Comment: Can it be provided on a separate PR? I would need to change the original implementation (for `ESP_HAL_3RDPARTY_[VERSION|URL]` and re-test everything on our internal CI. For the sake of consistency, we should keep them equal. -- 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] espressif: Add variable to override MCUboot version and URL [nuttx]
anchao commented on code in PR #16320: URL: https://github.com/apache/nuttx/pull/16320#discussion_r2075628670 ## arch/risc-v/src/common/espressif/Bootloader.mk: ## @@ -35,8 +35,14 @@ BOOTLOADER_CONFIG= $(BOOTLOADER_SRCDIR)/bootloader.conf MCUBOOT_SRCDIR = $(BOOTLOADER_SRCDIR)/mcuboot MCUBOOT_ESPDIR = $(MCUBOOT_SRCDIR)/boot/espressif -MCUBOOT_URL= https://github.com/mcu-tools/mcuboot MCUBOOT_TOOLCHAIN = $(TOOLSDIR)/mcuboot_toolchain_espressif.cmake +ifndef MCUBOOT_VERSION + MCUBOOT_VERSION = $(CONFIG_ESPRESSIF_MCUBOOT_VERSION) Review Comment: I just think the `?=` syntax here is more concise and easier to understand. -- 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] espressif: Add variable to override MCUboot version and URL [nuttx]
tmedicci commented on code in PR #16320: URL: https://github.com/apache/nuttx/pull/16320#discussion_r2075363360 ## arch/risc-v/src/common/espressif/Bootloader.mk: ## @@ -35,8 +35,14 @@ BOOTLOADER_CONFIG= $(BOOTLOADER_SRCDIR)/bootloader.conf MCUBOOT_SRCDIR = $(BOOTLOADER_SRCDIR)/mcuboot MCUBOOT_ESPDIR = $(MCUBOOT_SRCDIR)/boot/espressif -MCUBOOT_URL= https://github.com/mcu-tools/mcuboot MCUBOOT_TOOLCHAIN = $(TOOLSDIR)/mcuboot_toolchain_espressif.cmake +ifndef MCUBOOT_VERSION + MCUBOOT_VERSION = $(CONFIG_ESPRESSIF_MCUBOOT_VERSION) Review Comment: It's redundant. We already checked for the variable being (or not) defined one line before (and we did that to be more explicit). Are there other reasons that I'm missing here? (just for some background: this PR is highly inspired by https://github.com/apache/nuttx/pull/9545/commits/f8d8852cd6b722255f2da2ffe72b3d2fd7cd8b37, which our internal CI is using for two years) -- 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] espressif: Add variable to override MCUboot version and URL [nuttx]
anchao commented on code in PR #16320: URL: https://github.com/apache/nuttx/pull/16320#discussion_r2074362653 ## arch/risc-v/src/common/espressif/Bootloader.mk: ## @@ -35,8 +35,14 @@ BOOTLOADER_CONFIG= $(BOOTLOADER_SRCDIR)/bootloader.conf MCUBOOT_SRCDIR = $(BOOTLOADER_SRCDIR)/mcuboot MCUBOOT_ESPDIR = $(MCUBOOT_SRCDIR)/boot/espressif -MCUBOOT_URL= https://github.com/mcu-tools/mcuboot MCUBOOT_TOOLCHAIN = $(TOOLSDIR)/mcuboot_toolchain_espressif.cmake +ifndef MCUBOOT_VERSION + MCUBOOT_VERSION = $(CONFIG_ESPRESSIF_MCUBOOT_VERSION) +endif + +ifndef MCUBOOT_URL + MCUBOOT_URL = https://github.com/mcu-tools/mcuboot Review Comment: ```suggestion MCUBOOT_URL ?= https://github.com/mcu-tools/mcuboot ``` ## arch/risc-v/src/common/espressif/Bootloader.mk: ## @@ -35,8 +35,14 @@ BOOTLOADER_CONFIG= $(BOOTLOADER_SRCDIR)/bootloader.conf MCUBOOT_SRCDIR = $(BOOTLOADER_SRCDIR)/mcuboot MCUBOOT_ESPDIR = $(MCUBOOT_SRCDIR)/boot/espressif -MCUBOOT_URL= https://github.com/mcu-tools/mcuboot MCUBOOT_TOOLCHAIN = $(TOOLSDIR)/mcuboot_toolchain_espressif.cmake +ifndef MCUBOOT_VERSION + MCUBOOT_VERSION = $(CONFIG_ESPRESSIF_MCUBOOT_VERSION) Review Comment: ```suggestion MCUBOOT_VERSION ?= $(CONFIG_ESPRESSIF_MCUBOOT_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] espressif: Add variable to override MCUboot version and URL [nuttx]
nuttxpr commented on PR #16320: URL: https://github.com/apache/nuttx/pull/16320#issuecomment-2852025746 [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR meets the NuttX requirements. The summary clearly explains the change, its purpose, and how it works. The impact section adequately covers all the points, specifically noting the build change and explaining it. The testing section provides clear instructions on how to reproduce the test, including expected results. The use of specific commit hashes strengthens the verification. The information presented is concise and well-organized. -- 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]
