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: Check compiler version #7247

Merged
merged 9 commits into from Jun 27, 2018

Conversation

Projects
None yet
7 participants
@theotherjimmy
Contributor

theotherjimmy commented Jun 18, 2018

Description

In the past, we allowed any version of any toolchain. That will be a
problem if/when we upgrade to ARM Compiler 5.10 or IAR 8. This PR
has the tools enforce the version of the compiler on mbed compile,
preventing any issues with compiler version

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change
@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 18, 2018

So even though this is technically enforcing something that we should already be checking, therefore making it a fix, because this has the potential to stir up a whole bunch of commotion if it comes into a patch release, I'm marking this as for coming in during the next feature release.

@ARMmbed/mbed-os-maintainers Thoughts?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 18, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 18, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 19, 2018

/morph uvisor-test

1 similar comment
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 19, 2018

/morph uvisor-test

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 19, 2018

👍 for having the version checked - we have them documented so issues reported are with tools we use and test.

So even though this is technically enforcing something that we should already be checking, therefore making it a fix, because this has the potential to stir up a whole bunch of commotion if it comes into a patch release, I'm marking this as for coming in during the next feature release.

Definitely minor fix.

@0xc0170 0xc0170 requested review from bulislaw and SenRamakri Jun 19, 2018

@bulislaw

This comment has been minimized.

Member

bulislaw commented Jun 19, 2018

Are we sure that should be an error not warning?

if match:
found_version = match.group(0)
if found_version and not found_version.startswith(self.GCC_MAJOR + "."):
raise NotSupportedException(

This comment has been minimized.

@alzix

alzix Jun 19, 2018

Contributor

Do we really want to fail the build? Perhaps noticeable warning message will be sufficient?

Have you considered using LooseVersion from distutils.version package?

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 19, 2018

Contributor

LooseVersion is a good suggestion. I'll take a look.

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 19, 2018

@cmonr @0xc0170 Would a warning be patch-able?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 19, 2018

@alzix It now uses version ranges with LooseVersion

Everyone: It should now display an error, and keep compiling.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 19, 2018

@theotherjimmy I'm of the opinion that a warning could go into a patch, since it wouldn't lead to breaking changes for users.

@ARMmbed/mbed-os-maintainers Any objections?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 19, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 19, 2018

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:toolchain-version-check branch from ff26bd6 to c273de6 Jun 26, 2018

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 26, 2018

Tests in. Many bugs prevented.

@alzix

This comment has been minimized.

Contributor

alzix commented Jun 26, 2018

LGTM

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 27, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: review labels Jun 27, 2018

@cmonr

cmonr approved these changes Jun 27, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 27, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Jun 27, 2018

@cmonr cmonr merged commit dec4392 into ARMmbed:master Jun 27, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
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, 929 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9559 cycles (-812 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 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
@mattbrown015

This comment has been minimized.

Contributor

mattbrown015 commented Jun 28, 2018

Just wondering if there is any particular reason for using ARM GCC version 6?

I've been using ARM GCC version 7 but for no particular reason. Coincidentally ARM GCC V7 2018 q2 update was released yesterday.

So with this change I get an error:

[Error] @,: Compiler version mismatch: Have 7.2.1; expected version >= 6.0.0 and < 7.0.0

My guess is that updating to V7 is just a matter of time and inclination on your behalf. No doubt updating your infrastructure and testing every target is pretty tedious.

I'm wondering whether to ignore your error; I've got no issues on STM32L4. Or to play safe and consistent and go back to V6??

Interestingly V6 uses more flash, 5888 bytes, and RAM, 198 bytes, than V7.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 28, 2018

My guess is that updating to V7 is just a matter of time and inclination on your behalf. No doubt updating your infrastructure and testing every target is pretty tedious.

We update it from time to time. @ARMmbed/mbed-os-tools might know more about upcoming version bump

From our experience, we identified few bugs using different major (sometimes even minor version bumps) versions. It can happen that you face a bug that we can't reproduce (different tools used). Also consider that there are some precompiled binaries in the tree that are used with specific toolchain version. I would suggest to stay within the version specified as supported (if there are not any specific reason for v7).

Interestingly V6 uses more flash, 5888 bytes, and RAM, 198 bytes, than V7.

I would like to know more about this, how the diff can be that big, what has changed with the update.

@mattbrown015

This comment has been minimized.

Contributor

mattbrown015 commented Jun 28, 2018

I wondered if the difference would diminish in a release build, but apparently not.

The following is from a release build for STM32L4.

6 2017-q2-update:

| [lib]\c.a                           |  36555 |  2472 |    89 |
| [lib]\stdc++.a                      |   9866 |    44 |   204 |
Total Static RAM memory (data + bss): 28992 bytes
Total Flash memory (text + data): 193458 bytes

7 2017-q4-major:

| [lib]\c.a                           |  33623 |  2472 |    89 |
| [lib]\stdc++.a                      |   8285 |    12 |    44 |
Total Static RAM memory (data + bss): 28808 bytes
Total Flash memory (text + data): 187783 bytes

Difference:

| [lib]\c.a                           |   2932 |     0 |     0 |
| [lib]\stdc++.a                      |   1581 |    32 |   160 |
Static RAM memory: 184 bytes
Flash memory: 5675 bytes

I highlighted c.a and stdc++.a because they have the largest differences but actually the .text sections for most of the modules are different. Some of the modules were bigger with '7 2017-q4-major' but most were smaller.

I wonder if there is a significant default flag change but I haven't looked at the compiler release notes and, while I find this interesting, I've got more important things to be doing!

We shall probably follow your advice and use the supported compiler.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 30, 2018

@mattbrown015 Interesting. Could you open an issue against the Mbed OS repo about this so that we can capture it?

To your question, the intention of this PR was to notify users that we have a specific set/range of compiler versions that we support (in that we actively test and fix issues for). That's not to say that newer and/or older versions of compilers won't work, but to say so without being in a place to continuously say that that is the case without having the backend systems in place would be irrisponsible.

I'm wondering whether to ignore your error; I've got no issues on STM32L4. Or to play safe and consistent and go back to V6??

I think that depends on how much support you end up needing. We're not in a place to be able to help you if you're using an unsupported compiler, however since it sounds like things are still working fine for you, I'd suggest sticking to what you have now unless something breaks.
I don't personally know what the differences are between 6 and 7, but I would imagine that code changes coming in will only be more compatible with 7 as time goes on.

ARMmbed/mbed-os-tools might know more about upcoming version bump.

We're actually actively looking into enabling v6 in the least distupive manner as possible.

Interestingly V6 uses more flash, 5888 bytes, and RAM, 198 bytes, than V7.

I'm not terribly surprised about this since I would expect a newer compiler version to have more optimizations. No idea what the specifics might be though.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 2, 2018

For what it's worth,

I'm wondering whether to ignore your error; I've got no issues on STM32L4. Or to play safe and consistent and go back to V6??

I think that depends on how much support you end up needing.

That's exactly the conversation we intend to happen! Now @mattbrown015 knows that he's taking a risk by using a compiler version that we don't test with.

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