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

manifest.md- How sdkconfig files are processed #634

Open
wants to merge 2 commits into
base: public
Choose a base branch
from

Conversation

jparker324
Copy link
Contributor

It looks like the description for step 4 may have been originally a copy and paste of step 2. The way it was worded, it read as if step 4 would somehow follow step 1.  This is confusing with the use of "additionally" in the sentence following step 3, and it didn't follow the same pattern of wording for all the other steps saying "... merged on top of the merge performed in step [n-1]."

Reference code

mergedConfig = mergedConfig.concat(entries.split(regex));

@andycarle
Copy link
Member

Hi @jparker324,

I believe the text in manifest.md is correct as-is. But, let's see if we can sort out the confusion here to verify that and to improve the text to be more clear.

To break it down a bit:

  • All ESP32 debug builds use the base sdkconfig.defaults SDKCONFIG file for the relevant sub-platform.
  • Release builds use a merge of sdkconfig.defaults + sdkconfig.defaults.release.
  • Release instrumented builds use a merge of sdkconfig.defaults + sdkconfig.defaults.release + sdkconfig.inst

Individual applications may provide additional SDKCONFIG fragments (they do not need to be a complete SDKCONFIG files) that are specific to builds for that application. This is done by setting the environment variable SDKCONFIGPATH in the application's manifest.

For clarity, let's imagine an application that provides all three SDKCONFIG fragments and let's refer to those fragments as app/sdkconfig.defaults, app/sdkconfig.defaults.release, and app/sdkconfig.inst.

  • Debug builds of that application will use a merge of sdkconfig.defaults + app/sdkconfig.defaults.
  • Release builds of that application will use a merge of sdkconfig.defaults + sdkconfig.defaults.release + app/sdkconfig.defaults + app/sdkconfig.defaults.release.
  • Release instrumented builds of that application will use a merge of sdkconfig.defaults + sdkconfig.defaults.release + sdkconfig.inst + app/sdkconfig.defaults + app/sdkconfig.defaults.release + app/sdkconfig.inst.

The merges happen sequentially in the order listed, with later files overwriting duplicate options from earlier files.

Note that debug builds of the app with the extra SDKCONFIG fragments do not use the .release or .inst fragments from either the base SDKCONFIG directory or the app-specific one.

Does that make sense? Any recommendations for how we could concisely clarify that in manfest.md?

  - Andy

@jparker324
Copy link
Contributor Author

jparker324 commented Apr 22, 2021

Hi @andycarle,

I agree with everything you said. Maybe it's how I'm reading step 4?

I'll take your last example, an instrumented build of an application that provides all three possible additional sdkconfig* file fragments.

Going step by step, after step 3 it would have merged sdkconfig.defaults + sdkconfig.defaults.release + sdkconfig.inst which is correct.

But at step 4, I understood the document as saying the app/sdkconfig.defaults file is inserted before step 2 because the "the base Moddable SDK sdkconfig.defaults file" refers to that file in step 1. The result then after step 4 would have been sdkconfig.defaults + app/sdkconfig.defaults + sdkconfig.defaults.release + sdkconfig.inst which is different from the code.

This confusion compounds again with steps 5 and 6, but clarifying step 4 prevents this further confusion. It was the way step 4 ends, different than all the other steps, that led me to think that maybe it was unique on purpose somehow, but it's really not. Step 4 happens after 3.

Thinking out loud here- Maybe the more simplifying edit is to remove the ending phrase of each step. Saying what each step "is merged..." and "are merged..." on is largely redundant with the last sentence of the preceding paragraph, which already says they are applied in the indicated sequence. Something like this may be more straightforward...

  1. All base sdkconfig.defaults options are applied to the build.

  2. On release builds, the sdkconfig.defaults.release options are merged.

  3. On release instrumented builds, the sdkconfig.inst options are merged.

    When applications specify optional sdkconfig files using the SDKCONFIGPATH manifest environment variable, the merge processing additionally includes the following:

  4. The application $(SDKCONFIGPATH)/sdkconfig.defaults options, when provided, are merged.

  5. On release builds, the application $(SDKCONFIGPATH)/sdkconfig.defaults.release options, when provided, are merged.

  6. On release instrumented builds, the application $(SDKCONFIGPATH)/sdkconfig.inst options, when provided, are merged.

-Jason

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

Successfully merging this pull request may close these issues.

2 participants