Skip to content

Conversation

@alekla01
Copy link
Contributor

Description

IOTCORE-1057

def transform_release_toolchains(toolchains, version):

Returns always ['ARM', 'GCC_ARM', 'IAR'] for mbed5.

Pull request type

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

Reviewers

Release Notes

@ciarmcom
Copy link
Member

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

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

What is this fixing - always return toolchains?

@cmonr
Copy link
Contributor

cmonr commented Feb 27, 2019

For reference, this is when that change was added: #2260

@alekla01
Copy link
Contributor Author

alekla01 commented Feb 27, 2019

@cmonr
so, the purpose of transform_release_toolchains is to remove toolchains which are not supported by the given version == remove all but ARMC5, ARMC6, ARM, GCC_ARM, IAR for mbed5?

def transform_release_toolchains(toolchains, version):
    if version == '5':
        mbed5_toolchains = ['ARMC5', 'ARMC6', 'ARM', 'GCC_ARM', 'IAR']
        return list(set(toolchains).intersection(set(mbed5_toolchains)))
    else:
        return toolchains

or the purpose purpose of transform_release_toolchains is to set toolchains = [some version of 'ARM', 'GCC_ARM', 'IAR'] for mbed5 (similar to #9860 & the current implementation)?
or perhaps targets.json should simply include a lists of supported toolchains per version for each target, in that case no transformation would be required (in case it is indeed required at the moment).

@SenRamakri
Copy link
Contributor

@alekla01 - Please look at the changes in #9860.
This function should now consider ARMC6 related tools changes and needs additional changes.
I would recommend park this PR for now and follow whats being done in #9860 which also addresses Online tools dependencies.

Tagging @theotherjimmy @cmonr @0xc0170

Copy link
Contributor

@cmonr cmonr left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@cmonr cmonr left a comment

Choose a reason for hiding this comment

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

@cmonr
Copy link
Contributor

cmonr commented Feb 27, 2019

@alekla01 It looks like this current changeset would likely break backwards compatability.

Looking at #9878 (comment) and it's referenced PR, it looks like that manages to update the values in a backwards compatible way.

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.

6 participants