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: Restrict toolchains reported by mbed compile -S to official ones #8249

Merged
merged 3 commits into from Oct 19, 2018

Conversation

Projects
None yet
6 participants
@theotherjimmy
Contributor

theotherjimmy commented Sep 25, 2018

Description

The mbed compile -S command is suposed to indicate what targets
support what toolchains. The command was printing out things that
don't make sense, like GCC_CR and things that make sense, but are
not offiially supported yet, like ARMC6. This PR fixes all of that.

Pull request type

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

@cmonr cmonr requested review from bridadan and cmonr Sep 26, 2018

@cmonr cmonr added the needs: review label Sep 26, 2018

@0xc0170

0xc0170 approved these changes Sep 26, 2018 edited

Isn't this available anywhere as a list (similar to other like allowed_features, etc): SUPPORTED_TOOLCHAINS ?I assume we check the validity for supported toolchains somwhere

unique_supported_toolchains.append(toolchain)
return unique_supported_toolchains
return ["ARM", "uARM", "GCC_ARM", "IAR"]

This comment has been minimized.

@bridadan

bridadan Sep 26, 2018

Contributor

To avoid creating a disconnect between the toolchain implementation, can we reference the keys present here instead:

TOOLCHAIN_CLASSES = {

I realize ARM6 is in that list though. So that would mean just removing it from the list here by name, or adding a "supported" variable (or something similar) to the toolchain class and then filter on that here.

@bridadan

This comment has been minimized.

Contributor

bridadan commented Sep 26, 2018

Side note, there are quite a few targets in targets.json that still list GCC_CR as a supported toolchain. Is it safe to go ahead and remove that compiler from those targets? Looking at the our supported toolchain code it doesn't look like we support that compiler anymore anyway.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 28, 2018

Side note, there are quite a few targets in targets.json that still list GCC_CR as a supported toolchain. Is it safe to go ahead and remove that compiler from those targets? Looking at the our supported toolchain code it doesn't look like we support that compiler anymore anyway.

I would think it should be fine. Not even sure what GCC_CR is. @ARMmbed/mbed-os-test Thoughts?

@bridadan

This comment has been minimized.

Contributor

bridadan commented Oct 1, 2018

GCC_CR referred to a variant of GCC called "Code Red" that used to be used (I think with some NXP products, can't remember). Anyway, I don't think you can get the compiler anymore so I think it should be ok to remove. Probably better to move this to a separate issue.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 2, 2018

I would think it should be fine. Not even sure what GCC_CR is. @ARMmbed/mbed-os-test Thoughts?

Not valid anymore, can be removed.

@bridadan bridadan referenced this pull request Oct 2, 2018

Merged

Remove GCC_CR #8304

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 3, 2018

@bridadan review please.

@bridadan

OFFICILLY_SUPPORTED is misspelled :)

Also, can we just say supported or released? We don't currently have a distinction between official/unofficial right?

@@ -48,6 +48,8 @@
CPU_COEF = 1
class mbedToolchain:
OFFICILLY_SUPPORTED = False

This comment has been minimized.

@bridadan

bridadan Oct 3, 2018

Contributor

This is the mispelling

@0xc0170 0xc0170 added needs: work and removed needs: review labels Oct 4, 2018

theotherjimmy added some commits Sep 25, 2018

Tools: Restrict toolchains reported by mbed compile -S to official ones
### Description

The `mbed compile -S` command is suposed to indicate what targets
support what toolchains. The command was printing out things that
don't make sense, like `GCC_CR` and things that make sense, but are
not offiially supported yet, like `ARMC6`. This PR fixes all of that.

### Pull request type

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

@cmonr cmonr force-pushed the theotherjimmy:official-tc-support branch from 5d2df3d to ec72ce7 Oct 18, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 18, 2018

@bridadan Typo corrected.

@theotherjimmy Thoughts on doing s/OFFICIALLY_SUPPORTED/SUPPORTED/g?

Typo updated. Re-review requested.

@cmonr cmonr added needs: review and removed needs: work labels Oct 18, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 18, 2018

@bridadan I think the idea is that in the past, there's been confusion about compilers being enabled vs supported in the tools. The thought goes, being a bit more verbose about this helps makes this a bit clearer, and can help contextualize questions if/when they come up.

@cmonr

cmonr approved these changes Oct 18, 2018

@bridadan

This comment has been minimized.

Contributor

bridadan commented Oct 19, 2018

I think the idea is that in the past, there's been confusion about compilers being enabled vs supported in the tools.

Yeah fair point, though I would think "enabled" would mean it's in the tools (an implementation exists) and "supported" would mean the flag is set. But if this helps clarify it more then I don't feel that strongly about it 😄

@cmonr cmonr added needs: CI and removed needs: review labels Oct 19, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 19, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 19, 2018

Build : SUCCESS

Build number : 3409
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8249/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit c40d860 into ARMmbed:master Oct 19, 2018

13 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 672 files (+0 files)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9131 cycles (+86 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the needs: CI label Oct 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment