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

script: Use absolute path for module_initialized check #25891

Merged
merged 1 commit into from
Mar 31, 2023

Conversation

wqx6
Copy link
Contributor

@wqx6 wqx6 commented Mar 30, 2023

We should use absolute path for module_initialized check so that we will not get an error when we run the checkout_submodule.py script in non-CHIP_ROOT directory.

@github-actions
Copy link

PR #25891: Size comparison from a27bc8c to a9eb272

Full report (1 build for cc32xx)
platform target config section a27bc8c a9eb272 change % change
cc32xx lock CC3235SF_LAUNCHXL 0 0 0 0.0
(read only) 645769 645769 0 0.0
(read/write) 203848 203848 0 0.0
.ARM.attributes 44 44 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 197248 197248 0 0.0
.comment 194 194 0 0.0
.data 1480 1480 0 0.0
.debug_abbrev 933102 933102 0 0.0
.debug_aranges 87704 87704 0 0.0
.debug_frame 301596 301596 0 0.0
.debug_info 20310043 20310043 0 0.0
.debug_line 2681003 2681003 0 0.0
.debug_loc 2827360 2827360 0 0.0
.debug_ranges 286352 286352 0 0.0
.debug_str 3041144 3041144 0 0.0
.ramVecs 780 780 0 0.0
.resetVecs 64 64 0 0.0
.rodata 105953 105953 0 0.0
.shstrtab 232 232 0 0.0
.stab 204 204 0 0.0
.stabstr 441 441 0 0.0
.stack 2048 2048 0 0.0
.strtab 380543 380543 0 0.0
.symtab 257472 257472 0 0.0
.text 537696 537696 0 0.0

@github-actions
Copy link

PR #25891: Size comparison from a27bc8c to d3622af

Increases (1 build for cc32xx)
platform target config section a27bc8c d3622af change % change
cc32xx lock CC3235SF_LAUNCHXL .debug_info 20310043 20310044 1 0.0
Full report (1 build for cc32xx)
platform target config section a27bc8c d3622af change % change
cc32xx lock CC3235SF_LAUNCHXL 0 0 0 0.0
(read only) 645769 645769 0 0.0
(read/write) 203848 203848 0 0.0
.ARM.attributes 44 44 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 197248 197248 0 0.0
.comment 194 194 0 0.0
.data 1480 1480 0 0.0
.debug_abbrev 933102 933102 0 0.0
.debug_aranges 87704 87704 0 0.0
.debug_frame 301596 301596 0 0.0
.debug_info 20310043 20310044 1 0.0
.debug_line 2681003 2681003 0 0.0
.debug_loc 2827360 2827360 0 0.0
.debug_ranges 286352 286352 0 0.0
.debug_str 3041144 3041144 0 0.0
.ramVecs 780 780 0 0.0
.resetVecs 64 64 0 0.0
.rodata 105953 105953 0 0.0
.shstrtab 232 232 0 0.0
.stab 204 204 0 0.0
.stabstr 441 441 0 0.0
.stack 2048 2048 0 0.0
.strtab 380543 380543 0 0.0
.symtab 257472 257472 0 0.0
.text 537696 537696 0 0.0

.gitmodules Outdated Show resolved Hide resolved
Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested: web sockets not required for embedded.

If you need web sockets for specific platforms, you probably need it for test scripts. Then you should checkout submodules for multiple platforms: linux for chip-tool builds, other platform for embedded builds.

@chshu
Copy link
Contributor

chshu commented Mar 31, 2023

Changes requested: web sockets not required for embedded.

If you need web sockets for specific platforms, you probably need it for test scripts. Then you should checkout submodules for multiple platforms: linux for chip-tool builds, other platform for embedded builds.

@andy31415 The libwebsockets is not required by esp32 build, but is required by chip-tool build. For most of the users who want to build esp32 example, they also need to build chip-tool for testing, hence we want to make the libwebsockets available by default for esp32 checkout_submodules. Other Linux modules (except for libwebsockets) are not required by esp32.

If you have some concern on removing, how about adding esp32 under libwebsockets, and a comment to mention it's required by chip-tool?

@andy31415
Copy link
Contributor

andy31415 commented Mar 31, 2023

Changes requested: web sockets not required for embedded.
If you need web sockets for specific platforms, you probably need it for test scripts. Then you should checkout submodules for multiple platforms: linux for chip-tool builds, other platform for embedded builds.

@andy31415 The libwebsockets is not required by esp32 build, but is required by chip-tool build. For most of the users who want to build esp32 example, they also need to build chip-tool for testing, hence we want to make the libwebsockets available by default for esp32 checkout_submodules. Other Linux modules (except for libwebsockets) are not required by esp32.

If you have some concern on removing, how about adding esp32 under libwebsockets, and a comment to mention it's required by chip-tool?

if chip-tool is being compiled for linux, you seem to need the "linux build sub modules". I do not believe libwebsockets is sufficient (in the long run that is ... today it is 100% sufficent as we do not have other linux-specific submodules used by chip-tool), you need to say platform linux if you intend to build linux bits.

If you are concerned that platform linux is too large, maybe we figure out some different platform name and we add that as an extra list.

@andy31415
Copy link
Contributor

@chshu - my concern is the use case. Right now the individual platform selection was picked for our CI so that CI does not pull more than needed.

CI for embedded (nrf, esp32, others) will explicitly not build chip-tool, so pulling in stuff because chip tool needs it is not helpful for our CI, hence my push back.

if you need to build both, what is wrong with checkout_submodulese.py (no platform ... will pull all ... and be slow) or checkout_submodules.py --platform linux --platform esp32 and then you have all is needed for both esp32 and native builds.

@andy31415
Copy link
Contributor

The correct syntax seems to be:

scripts/checkout_submodules.py --platform esp32 linux

kind of not intuitive ... but it works. Maybe we can improve ergonomics a bit and accept --platform esp32,linux for clarity.

@chshu
Copy link
Contributor

chshu commented Mar 31, 2023

@andy31415 Ok, understood your concern on the CI size.

If you are concerned that platform linux is too large, maybe we figure out some different platform name and we add that as an extra list.

--platform esp32,linux works, and a different platform (perhaps only for building tools) would be good to have.

@wqx6 wqx6 force-pushed the fix_module_initialized_check branch from d3622af to a9eb272 Compare March 31, 2023 02:54
@github-actions
Copy link

PR #25891: Size comparison from 378a6c2 to a9eb272

Decreases (1 build for cc32xx)
platform target config section 378a6c2 a9eb272 change % change
cc32xx lock CC3235SF_LAUNCHXL .debug_info 20310332 20310331 -1 -0.0
Full report (1 build for cc32xx)
platform target config section 378a6c2 a9eb272 change % change
cc32xx lock CC3235SF_LAUNCHXL 0 0 0 0.0
(read only) 645825 645825 0 0.0
(read/write) 203848 203848 0 0.0
.ARM.attributes 44 44 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 197248 197248 0 0.0
.comment 194 194 0 0.0
.data 1480 1480 0 0.0
.debug_abbrev 933102 933102 0 0.0
.debug_aranges 87704 87704 0 0.0
.debug_frame 301596 301596 0 0.0
.debug_info 20310332 20310331 -1 -0.0
.debug_line 2681125 2681125 0 0.0
.debug_loc 2827568 2827568 0 0.0
.debug_ranges 286448 286448 0 0.0
.debug_str 3041144 3041144 0 0.0
.ramVecs 780 780 0 0.0
.resetVecs 64 64 0 0.0
.rodata 105953 105953 0 0.0
.shstrtab 232 232 0 0.0
.stab 204 204 0 0.0
.stabstr 441 441 0 0.0
.stack 2048 2048 0 0.0
.strtab 380543 380543 0 0.0
.symtab 257472 257472 0 0.0
.text 537752 537752 0 0.0

@andy31415 andy31415 merged commit 12072b3 into project-chip:master Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants