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

Tools changes for bare metal #9561

Merged
merged 33 commits into from Feb 13, 2019

Conversation

@theotherjimmy
Copy link
Contributor

commented Jan 30, 2019

Description

This PR implements (and tests :D) the changes to the scanning rules
and configuration system for the requires key in mbed_app.json
and mbed_lib.json. Note: This is a 5.12 feature.

The requires key in mbed_lib.json lists the other libraries
that are required to build this library. When this library is
required by an application, the libraries it requires will be
included in the build.

The requires key in mbed_app.json lists the libraries (name
key from mbed_lib.json) required to build the application. This
causes mbed compile to filter the libraries included to those
listed in the application and those listed by the required
libraries.

Note: This pull request includes unit tests for this new feature.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

(following section required to auto-generate release notes)

Release notes

Mbed OS now supports the requires section in both
mbed_app.json and mbed_lib.json.

The requires key in mbed_lib.json lists the other libraries
that are required to build this library. When this library is
required by an application, the libraries it requires will be
included in the build.

The requires key in mbed_app.json lists the libraries (name
key from mbed_lib.json) required to build the application. This
causes mbed compile to filter the libraries included to those
listed in the application and those listed by the required
libraries.

No migration is required.

@theotherjimmy theotherjimmy changed the title Tools channges for bare metal Tools changes for bare metal Jan 30, 2019

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Jan 30, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

@bridadan
Copy link
Contributor

left a comment

Really nice so far!

Show resolved Hide resolved tools/test/config/config_test.py Outdated
Show resolved Hide resolved tools/test/resources/resource_test.py Outdated
Show resolved Hide resolved tools/test/resources/resource_test.py
Show resolved Hide resolved tools/config/__init__.py Outdated

theotherjimmy added some commits Jan 30, 2019

@@ -269,7 +269,7 @@ def get_file_refs(self, file_type):
if not any(
path.startswith(dirname(e.path)) for e in self._excluded_libs
):
to_ret.append(ref)
to_ret.append(ref)

This comment has been minimized.

Copy link
@cmonr

cmonr Jan 30, 2019

Contributor

Woah

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Jan 31, 2019

Author Contributor

I switched editors recently, until it decided to never insert spaces when I type <tab>. I have switch back because that's infuriating when working with python.

@0xc0170
Copy link
Member

left a comment

Waiting for design doc, how this is supposed to be used? Release notes section might help there.

@@ -0,0 +1,136 @@
# mbed SDK
# Copyright (c) 2019 ARM Limited
#

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 11, 2019

Member

Please also add SPDX identifier for new files

@donatieng
Copy link
Member

left a comment

I'd like to +1 the request for a design doc before we can review this efficiently!

@@ -6,6 +6,13 @@
"type": "string"
}
},
"requires_definition": {
"descirption": "Required libraries",

This comment has been minimized.

Copy link
@donatieng

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Feb 11, 2019

Author Contributor

Thanks.

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

I'd like to +1 the request for a design doc before we can review this efficiently!

@donatieng
I sent offline link to confluence page about this, if possible to start look at the changes so @theotherjimmy can address any new issues (if any)?

spdx added

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Should this PR include design doc? docs/design-documents is proper location

@donatieng

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

Is there a deprecation path for "features" (and components)? I feel like this requirement overlaps with this functionality. If so, it'd be worth adding an mbed_lib.json file for Bluetooth as well.

@theotherjimmy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

@donatieng I'm not aware of a plan to deprecate features. I can understand the overlap you see. Adding an mbed_lib.json for Bluetooth could be something that is reasonable for the future. That can certainly be a follow on PR, as I would like to get this PR in soon.

@donatieng
Copy link
Member

left a comment

Thanks for the clarification @theotherjimmy ! Yes I think that the complexity of all these somewhat overlapping features ("features", "components" and now this) needs to be addressed otherwise this will hurt our UX.

@theotherjimmy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

@donatieng I agree that no guidance on "features", "components", extra_labels, and requires in mbed_app.json and mbed_lib.json will only lead to more confusion long term. I think we could probably deprecate "features" and "components" in favor of requires.

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

I'm also in favour of deprecating features and components in favour of requires as those have working dependency system.

Components and features have pushed the integration pain towards the application developer, who need to be aware of what library requires what feature.

@orenc17

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

@donatieng @theotherjimmy @SeppoTakalo i agree with you, but we need this for THIS release
in the one after that we could do whatever we want

@bridadan
Copy link
Contributor

left a comment

Agreed @orenc17, we need to

stay on target

😄

@orenc17

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

can someone run CI on this please?

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 13, 2019

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
@orenc17

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

@ARMmbed/mbed-os-maintainers why was this build aborted?

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 13, 2019

Test run: FAILED

Summary: 4 of 9 test jobs failed
Build number : 5
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_dynamic-memory-usage
  • jenkins-ci/mbed-os-ci_cloud-client-test
  • jenkins-ci/mbed-os-ci_exporter
  • jenkins-ci/mbed-os-ci_greentea-test
@alekla01

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

Restarted CI, was aborted & restarted due to CI related error.

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 13, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 7
Build artifacts

@cmonr cmonr added ready for merge and removed needs: review labels Feb 13, 2019

@cmonr

cmonr approved these changes Feb 13, 2019

@cmonr cmonr merged commit b820ec8 into ARMmbed:master Feb 13, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10504 cycles (+1267 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Feb 13, 2019

@orenc17

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

Hallelujah!!!!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.