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

Move config system into it's own folder and refactor header generation #4150

Merged
merged 2 commits into from Apr 20, 2017

Conversation

Projects
None yet
6 participants
@theotherjimmy
Contributor

theotherjimmy commented Apr 10, 2017

Descritpion

The config system is a huge file, and seems to be growing to support
things such as bootloader. This implies that the Config class may grow
to beyond the size of this one file.

Furthermore, I saw an error generated by the online build system recently.
This was the traceback:

ValueError: max() arg is an empty sequence
  File "mbed/build_tasks/tasks.py", line 33, in perform_build
    result = _perform_build(build_params)
  File "mbed/build_tasks/tasks.py", line 139, in _perform_build
    result = build(build_params=build_params)
  File "mbed_compiler/build.py", line 223, in build
    mbed_compiler.build_api.build_program(build_params=build_params, notify=notify)
  File "mbed_compiler/build_api.py", line 227, in build_program
    resources.objects = toolchain.compile_sources(resources, build_params['chrooted_build']) + resources.objects
  File "tools/toolchains/__init__.py", line 802, in compile_sources
    self.get_config_header()
  File "tools/toolchains/__init__.py", line 1134, in get_config_header
    crt_data = Config.config_to_header(self.config_data) if self.config_data else None
  File "tools/config.py", line 823, in config_to_header
    if params else 0)

How to reproduce

Simply create an mbed_app.json that removes all macros. This will
cause the macros array to be empty without causing a ConfigException.
When the macros array is empty, max() becomes very confused, as it
is asked to take the maximum of an empty array.

New behavior

We just add a [0] to the end to prevent that silly traceback.

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:refactor-config-header branch 2 times, most recently Apr 10, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 10, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Apr 10, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1874

Build failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 11, 2017

morph test - known failure, will be fixed soon (patch already proposed, waiting for CI to complete)

@0xc0170

Looking good, just 2 small things in the template. Tools are slowly becoming more organized 👍

@@ -0,0 +1,26 @@
// Automatically generated configuration file.

This comment has been minimized.

@0xc0170

0xc0170 Apr 11, 2017

Member

Should this file contain license header? I would say yes.

This comment has been minimized.

@theotherjimmy

theotherjimmy Apr 11, 2017

Contributor

I also say yes. now it has one.

{%- endif %}
{% endfor %}
{%- endif %}
#endif

This comment has been minimized.

@0xc0170

0xc0170 Apr 11, 2017

Member

should have a new line at the end of file

This comment has been minimized.

@theotherjimmy

theotherjimmy Apr 11, 2017

Contributor

👍

@0xc0170 0xc0170 added the needs: work label Apr 11, 2017

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:refactor-config-header branch Apr 11, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 11, 2017

@0xc0170 The gig's up! you noticed that I was organizing the tools.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 11, 2017

Thanks for the review. I addressed all of the comments, so I'm moving this to needs Review and needs CI

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 11, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Apr 12, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1889

Example Build failed!

@tommikas

This comment has been minimized.

Contributor

tommikas commented Apr 12, 2017

Something wrong with mbed_config.h generation?

armcc failing:

Compile [  0.3%]: FlashIAP.cpp
[Warning] mbed_config.h@66,0:  #2775-D: white space is required between the macro name "MEM_ALLOC" and its replacement text
[Warning] mbed_config.h@67,0:  #2775-D: white space is required between the macro name "MEM_FREE" and its replacement text
[Warning] mbed_config.h@70,0:  #2775-D: white space is required between the macro name "MBED_HEAP_STATS_ENABLED" and its replacement text
[Warning] mbed_config.h@72,0:  #1-D: last line of file ends without a newline
Compile [  0.7%]: ns_cmdline.c
[Warning] mbed_config.h@66,0:  #2775-D: white space is required between the macro name "MEM_ALLOC" and its replacement text
[Warning] mbed_config.h@67,0:  #2775-D: white space is required between the macro name "MEM_FREE" and its replacement text
[Warning] mbed_config.h@70,0:  #2775-D: white space is required between the macro name "MBED_HEAP_STATS_ENABLED" and its replacement text
[Warning] mbed_config.h@72,0:  #1-D: last line of file ends without a newline
[Error] ns_cmdline.c@342,0:  #29: expected an expression
[Error] ns_cmdline.c@344,0:  #29: expected an expression
[Error] ns_cmdline.c@437,0:  #29: expected an expression

gcc:

Compile [  0.3%]: FlashIAP.cpp
[Warning] mbed_config.h@66,18: missing whitespace after the macro name
[Warning] mbed_config.h@67,17: missing whitespace after the macro name
[Warning] mbed_config.h@70,32: missing whitespace after the macro name
Compile [  0.7%]: Ethernet.cpp
[Warning] mbed_config.h@66,18: missing whitespace after the macro name
[Warning] mbed_config.h@67,17: missing whitespace after the macro name
[Warning] mbed_config.h@70,32: missing whitespace after the macro name
Compile [  1.0%]: ns_cmdline.c
[Warning] mbed_config.h@66,18: ISO C99 requires whitespace after the macro name
[Warning] mbed_config.h@67,17: ISO C99 requires whitespace after the macro name
[Warning] mbed_config.h@70,32: ISO C99 requires whitespace after the macro name
[Error] mbed_config.h@67,17: expected expression before '=' token
[Error] mbed_config.h@67,17: expected expression before '=' token
[Error] mbed_config.h@67,17: expected expression before '=' token
[Error] mbed_config.h@67,17: expected expression before '=' token
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 12, 2017

@tommikas Thanks for the info. I'll check into the failure.

theotherjimmy added some commits Apr 10, 2017

Move config to own dir
I also broke the config header template into it's own file. Further, I
fixed a bug in the config header generation where if no macros, builds
would crash.

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:refactor-config-header branch to 1570233 Apr 12, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 12, 2017

That should do it. I was using ConfigMacro.name instead of ConfigMacro.macro_name.

name included the value...

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 12, 2017

/morph test

1 similar comment
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 18, 2017

/morph test

@sg- sg- added needs: CI and removed needs: work labels Apr 19, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Apr 19, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 31

All builds and test passed!

@0xc0170 0xc0170 removed the needs: CI label Apr 19, 2017

@adbridge adbridge merged commit 41ff084 into ARMmbed:master Apr 20, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment