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

Separated library sources in cmake for selective. #5136

Merged
merged 6 commits into from Jan 31, 2024

Conversation

AronRubin
Copy link
Contributor

Separated library sources and requirements in cmake for selective compilation with Kconfig system.

@me-no-dev
Copy link
Member

@projectgus could you have a look please

CMakeLists.txt Outdated
if(NOT CONFIG_ARDUINO_SELECTIVE_COMPILATION OR CONFIG_ARDUINO_SELECTIVE_LITTLEFS)
list(APPEND priv_requires esp_littlefs)
endif()
set(priv_requires fatfs nvs_flash app_update spiffs bootloader_support openssl bt main ${ARDUINO_LIBRARIES_REQUIRES})
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a good way to structure the source file dependencies, nice and clean!

The only difficulty is ARDUINO_LIBRARIES_REQUIRES, because in the ESP-IDF build system component requirements can't depend on config. This is actually already a problem in the current CMakeLists.txt file, though - it's not a problem added in this PR.

What happens in the build system is the component CMakeLists.txt files are expanded twice - once to build the dependency tree of component requirements and determine which components are included in the build, and then again to do everything else (add source files, compiler command line arguments, etc.). The first expansion doesn't have the project config available (because the project config depends on which components are included in the build, so it would otherwise be a cyclic dependency.)

As a result, I expect that the test if(NOT CONFIG_ARDUINO_SELECTIVE_COMPILATION OR CONFIG_ARDUINO_SELECTIVE_${libname}) is always true on the first pass, and all of the component requirements (esp_littlefs, esp_https_ota, etc.) will be added to ARDUINO_LIBRARIES_REQUIRES even if these options are disabled.

On the second pass, the config is available so the SRCS files are filtered correctly as expected.

Long term, we know this is not ideal and we have a plan to add "weak" requirements of the variety "component X requires component Y, but only if component Y is available". But this isn't available right now, sorry!

Suggest for now making the requirements list constant (ie remove ARDUINO_LIBRARIES_REQUIRES, place esp_littlefs and esp_https_ota on this line.)

The rest LGTM!

Copy link
Contributor Author

@AronRubin AronRubin Jun 4, 2021

Choose a reason for hiding this comment

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

I have actually been able to get around that limitation with another project by using

idf_build_get_property(__hack_component_targets __COMPONENT_TARGETS)

And then testing both __hack_component_targets and BUILD_COMPONENTS in the rest of the CMake file. esp_littlefs requires subprojects and esp_https_ota should not be required for projects that do not use OTA (e.g. are BLE only).

Copy link
Member

Choose a reason for hiding this comment

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

@projectgus it seems to work though. I tested this exact case when fixing arduino to compile as stand-alone component.

@AronRubin could you please rebase to the current CMakeLists.txt?

Copy link
Contributor

Choose a reason for hiding this comment

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

@me-no-dev Oh you're right - sorry. I see now that in current master that if CONFIG_LITTLEFS_PAGE_SIZE and CONFIG_TINYUSB_ENABLED are not set then the relevant component is not added, so that will work fine. (In the first expansion these variables are always empty as there is no config yet, in the second expansion their value will depend on the config.)

Looking at __COMPONENT_TARGETS to check the list at build time should work as well. You can also do the check this way:

if (NOT CMAKE_BUILD_EARLY_EXPANSION)
    idf_build_get_property(components_to_build BUILD_COMPONENTS)
    if(some_component IN_LIST components_to_build)
      message(STATUS "Including optional component some_component")
      list(APPEND priv_requires some_component)
   endif()
endif()

(The only significant differences: checking CMAKE_BUILD_EARLY_EXPANSION means this is skipped on the initial pass before any config is available, and the BUILD_COMPONENTS property is documented so it shouldn't change in an update.)

EDIT: This is wrong, I forgot that priv_requires is ignored after the CMAKE_BUILD_EARLY_EXPANSION phase. To get correct behaviour need to follow this approach: #5401

@stale
Copy link

stale bot commented Apr 16, 2022

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

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

@VojtechBartoska VojtechBartoska added the Status: Review needed Issue or PR is awaiting review label Jan 30, 2024
Copy link
Contributor

github-actions bot commented Jan 31, 2024

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "Fixed missing SimpleBLE in library list":
    • summary looks empty
    • type/action looks empty
  • the commit message "Reodered selective process to match CI script":
    • summary looks empty
    • type/action looks empty
  • the commit message "Separated library sources in cmake for selective.":
    • summary should not end with a period (full stop)
    • summary looks empty
    • type/action looks empty

Please fix these commit messages - here are some basic tips:

  • follow Conventional Commits style
  • correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue
  • allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test
  • sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter
  • avoid Jira references in commit messages (unavailable/irrelevant for our customers)

TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

⚠️ Please consider squashing your 6 commits (simplifying branch history).

👋 Hello AronRubin, we appreciate your contribution to this project!


Click to see more instructions ...


This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...


We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against ae71362

@P-R-O-C-H-Y
Copy link
Member

@AronRubin Can you please sign CLA? Thanks

@P-R-O-C-H-Y P-R-O-C-H-Y added Resolution: Awaiting response Waiting for response of author and removed Status: Review needed Issue or PR is awaiting review labels Jan 31, 2024
Copy link
Member

@P-R-O-C-H-Y P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

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

Rebased, fixed conflicts and tested within IDF.

@VojtechBartoska VojtechBartoska added this to the 3.0.0-RC1 milestone Jan 31, 2024
@VojtechBartoska VojtechBartoska added Status: Pending and removed Resolution: Awaiting response Waiting for response of author labels Jan 31, 2024
@me-no-dev me-no-dev merged commit bbe09cc into espressif:master Jan 31, 2024
52 checks passed
@AronRubin
Copy link
Contributor Author

I apologize for not seeing these emails. I trust that you have done the right thing. Thank you for your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants