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

arduino as component: Remove circular dependency around 'main' #5391

Merged
merged 1 commit into from
Jul 16, 2021

Conversation

projectgus
Copy link
Contributor

arduino-esp32 has to depend on main in autostart mode, for setup() and loop(),
but this can be done with undefined symbol entries to avoid a large dependency
cycle and other linker errors.

Closes espressif/esp-idf#6968

arduino-esp32 has to depend on main in autostart mode, for setup() and loop(),
but this can be done with undefined symbol entries to avoid a large dependency
cycle and other linker errors.

Closes espressif/esp-idf#6968
@projectgus projectgus requested a review from me-no-dev July 16, 2021 07:29
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

# linker will always include them.
#
# (As they are C++ symbol, we need to add the C++ mangled names.)
target_link_libraries(${COMPONENT_LIB} INTERFACE "-u _Z5setupv -u _Z4loopv")
Copy link
Member

Choose a reason for hiding this comment

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

How do you know what is the proper mangle prefix/suffix? Asking because we might need to add some more functions to the list

Copy link
Member

Choose a reason for hiding this comment

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

You can run something like:

echo "void setup() {}" | g++ -c -x c++ -o - - | nm -

Output:

0000000000000000 T __Z5setupv

@me-no-dev me-no-dev merged commit cf6ab9c into master Jul 16, 2021
@me-no-dev me-no-dev deleted the bugfix/remove_component_circular_dependency branch July 16, 2021 09:25
@me-no-dev
Copy link
Member

@projectgus Unfortunately this change caused arduino to not find the other components. I guess this means that sdkconfig was not expanded when it ran through CMakeLists and the components were not "required"

@igrr
Copy link
Member

igrr commented Jul 16, 2021

@me-no-dev as I mentioned on Wednesday, this pattern:

if(NOT CONFIG_ARDUINO_SELECTIVE_COMPILATION OR CONFIG_ARDUINO_SELECTIVE_ArduinoOTA)
list(APPEND priv_requires esp_https_ota)
endif()
if(NOT CONFIG_ARDUINO_SELECTIVE_COMPILATION OR CONFIG_ARDUINO_SELECTIVE_LITTLEFS)
if(CONFIG_LITTLEFS_PAGE_SIZE)
list(APPEND priv_requires esp_littlefs)
endif()
endif()
idf_component_register(INCLUDE_DIRS ${includedirs} PRIV_INCLUDE_DIRS ${priv_includes} SRCS ${srcs} REQUIRES ${requires} PRIV_REQUIRES ${priv_requires})

doesn't work in IDF build system. This is because IDF build system processes each component CMakeLists.txt twice:

  1. first time, in a process called "early expansion", to determine component requirements. All arguments except REQUIRES and PRIV_REQUIRES that are passed to idf_component_register are ignored.
  2. second time, when the components list is known, and CONFIG options are already expanded. At this stage, REQUIRES and PRIV_REQUIRES arguments are ignored.

Before this PR, arduino component had main as a requirement, while main implicitly required all components included into the build. Even though the lines in CMakeLists.txt which I linked above didn't do anything [1] (CONFIG_* items were not defined at the early expansion pass), this transitive dependency arduino -> main -> {every other component} caused the compilation to succeed.

Now that the dependency from arduino to every other component through main has been removed, arduino component dependencies on arduino_tinyusb, esp_littlefs, esp_https_ota no longer works.

I'll open a PR to correct this by adding these dependencies at a later stage in the component CMakeLists file.

Edit: [1]: i mistakenly said that the fragment didn't do anything. This isn't correct. It should be said that the linked CMakeLists.txt fragment always evaluated as if all of the Kconfig options were undefined, or set to "false". This means, for example, that NOT CONFIG_ARDUINO_SELECTIVE_COMPILATION evaluated as true.

igrr added a commit that referenced this pull request Jul 16, 2021
Until this commit, Kconfig options (e.g. CONFIG_TINYUSB_ENABLED) were
used in conditions preceding idf_component_register to determine which
components need to be added to `arduino` component requirements.
However the Kconfig options aren't known at the early expansion stage,
when the component CMakeLists.txt files are expanded the first time
and requirements are evaluated. So all the conditions evaluated as if
the options were not set.
This commit changes the logic to only add these components as
dependencies when the Kconfig options are known. Dependencies become
"weak", which means that if one of the components isn't included into
the build for some reason, it is not added as a dependency.
This may happen, for example, if the component is not present in the
`components` directory or is excluded by setting `COMPONENTS` variable
in the project CMakeLists.txt file.
This also ensures that if the component is not present, it will not be
added as a dependency, and this will allow the build to proceed.

Follow-up to #5391.
Closes #5319.
igrr added a commit that referenced this pull request Jul 16, 2021
Until this commit, Kconfig options (e.g. CONFIG_TINYUSB_ENABLED) were
used in conditions preceding idf_component_register to determine which
components need to be added to `arduino` component requirements.
However the Kconfig options aren't known at the early expansion stage,
when the component CMakeLists.txt files are expanded the first time
and requirements are evaluated. So all the conditions evaluated as if
the options were not set.
This commit changes the logic to only add these components as
dependencies when the Kconfig options are known. Dependencies become
"weak", which means that if one of the components isn't included into
the build for some reason, it is not added as a dependency.
This may happen, for example, if the component is not present in the
`components` directory or is excluded by setting `COMPONENTS` variable
in the project CMakeLists.txt file.
This also ensures that if the component is not present, it will not be
added as a dependency, and this will allow the build to proceed.

Follow-up to #5391.
Closes #5319.
me-no-dev pushed a commit that referenced this pull request Jul 16, 2021
…5404)

Until this commit, Kconfig options (e.g. CONFIG_TINYUSB_ENABLED) were
used in conditions preceding idf_component_register to determine which
components need to be added to `arduino` component requirements.
However the Kconfig options aren't known at the early expansion stage,
when the component CMakeLists.txt files are expanded the first time
and requirements are evaluated. So all the conditions evaluated as if
the options were not set.
This commit changes the logic to only add these components as
dependencies when the Kconfig options are known. Dependencies become
"weak", which means that if one of the components isn't included into
the build for some reason, it is not added as a dependency.
This may happen, for example, if the component is not present in the
`components` directory or is excluded by setting `COMPONENTS` variable
in the project CMakeLists.txt file.
This also ensures that if the component is not present, it will not be
added as a dependency, and this will allow the build to proceed.

Follow-up to #5391.
Closes #5319.
me-no-dev added a commit that referenced this pull request Jul 16, 2021
* Update toolchain

* Update package_esp32_index.template.json

* add optional component dependencies after Kconfig options are known (#5404)

Until this commit, Kconfig options (e.g. CONFIG_TINYUSB_ENABLED) were
used in conditions preceding idf_component_register to determine which
components need to be added to `arduino` component requirements.
However the Kconfig options aren't known at the early expansion stage,
when the component CMakeLists.txt files are expanded the first time
and requirements are evaluated. So all the conditions evaluated as if
the options were not set.
This commit changes the logic to only add these components as
dependencies when the Kconfig options are known. Dependencies become
"weak", which means that if one of the components isn't included into
the build for some reason, it is not added as a dependency.
This may happen, for example, if the component is not present in the
`components` directory or is excluded by setting `COMPONENTS` variable
in the project CMakeLists.txt file.
This also ensures that if the component is not present, it will not be
added as a dependency, and this will allow the build to proceed.

Follow-up to #5391.
Closes #5319.

* IDF master d93887f9f

* PlatformIO updates for CI (#5387)

* Update PlatformIO CI build script

- Switch to the latest toolchains 8.4.0 for ESP32, ESP32S2, ESP32C3
- Use PlatformIO from master branch for better robustness

* Update package.json for PlatformIO

Co-authored-by: Ivan Grokhotkov <[email protected]>
Co-authored-by: Valerii Koval <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants