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

Improvement to artifact delivery method #9633

Merged
merged 4 commits into from Feb 11, 2019

Conversation

Projects
None yet
10 participants
@orenc17
Copy link
Contributor

commented Feb 6, 2019

Description

A target can define a delivery directory instead of the default option

Pull request type

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

Reviewers

@theotherjimmy

Release notes

When a target declares a deliver_to_target in targets.json the output binary will be copied to DELIVERY directory in the root of mbed-os.
We now allow to declare a path to where the deliverables will be delivered to by declaring delivery_dir in the the target in targets.json
delivery_dir is a relative to where it was last changed.
so we have the following scenarios:

  1. delivery_dir not specified -> {mbed-os root}/DELIVERY/TARGET_{REQUESTED_TARGET}
  2. delivery_dir specified in targets.json -> the path will be relative to targets folder inside mbed-os.
  3. delivery_dir specified in mbed_app.json -> the path will be relative to that file's base folder.
Improvement to artifact delivery method
A target can define a delivery directory instead of the default option

@ciarmcom ciarmcom requested review from theotherjimmy and ARMmbed/mbed-os-maintainers Feb 6, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

@orenc17, thank you for your changes.
@theotherjimmy @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

@theotherjimmy
Copy link
Contributor

left a comment

Back to where I started. Works for me.

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

CI started, but still needs maintainer review

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

@orenc17 please take a look at the CI failure - nevermind , unrelated CI issue

@mikisch81
Copy link
Contributor

left a comment

Perhaps we should change DELIVERY_DIR now to DEFAULT_DELIVERY_DIR?

@alekla01

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

Restarted CI

@0xc0170 0xc0170 removed the needs: review label Feb 7, 2019

@0xc0170

0xc0170 approved these changes Feb 7, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

Perhaps we should change DELIVERY_DIR now to DEFAULT_DELIVERY_DIR?

Why would you change it? I dont see anywhere in the config used DEFAULT_ ?

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 7, 2019

Test run: SUCCESS

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

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

@ARMmbed/mbed-os-maintainers does it really needs-work? eventually tests passed.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

needs: work

Functionality change should contain release notes section , please add it (following ARMmbed/mbed-os-5-docs#933)

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

Perhaps we should change DELIVERY_DIR now to DEFAULT_DELIVERY_DIR?

Why would you change it? I dont see anywhere in the config used DEFAULT_ ?

Not really important.. thats why I approved.

@orenc17

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

@0xc0170 release notes added

@orenc17

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

@ARMmbed/mbed-os-maintainers could you please run the CI again once travis is done?

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

starting CI

Show resolved Hide resolved tools/config/__init__.py Outdated

@0xc0170 0xc0170 added needs: work and removed needs: review labels Feb 7, 2019

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 7, 2019

Test run: FAILED

Summary: 1 of 12 test jobs failed
Build number : 6
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_dynamic-memory-usage
@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

LGTM
Did you check the scenario of building from inside/outside mbed-os folder?

@orenc17

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

LGTM
Did you check the scenario of building from inside/outside mbed-os folder?

Yes, it works flawlessly

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

@theotherjimmy, delivery for was changed as you requested.

@mikisch81
Copy link
Contributor

left a comment

@0xc0170 Can we run CI?

We addressed @theotherjimmy remarks and waiting approval.

Show resolved Hide resolved tools/config/__init__.py Outdated
Show resolved Hide resolved tools/config/__init__.py Outdated
Update tools/config/__init__.py
Co-Authored-By: orenc17 <oren.cohen@arm.com>
@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2019

starting CI while we wait for technical writer review.

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 10, 2019

Test run: SUCCESS

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

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

@ARMmbed/mbed-os-maintainers No technical writer around?
Is it really necessary to postpone this PR like that?

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

starting CI while we wait for technical writer review.

@AnotherButler

@0xc0170 0xc0170 merged commit 36df2aa into ARMmbed:master Feb 11, 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 9178 cycles (-741 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

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

@mikisch81 Talking with other maintainers on the Release Notes review. Seems odd to me as well that we'd block a PR on a description update.

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.