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.

Sign up for GitHub

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

Jump to bottom

mbed_retarget: enable IAR build #14935

Merged
merged 1 commit into from
Aug 17, 2021
Merged

mbed_retarget: enable IAR build #14935

merged 1 commit into from
Aug 17, 2021

Conversation

jeromecoutant
Copy link
Collaborator

Summary of changes

Since #14751 IAR build is no more possible.

@tymoteuszblochmobica
@MarceloSalazar @0xc0170 @kjbracey-arm

Impact of changes

IAR build is OK

Migration actions required

Documentation

Pull request type 

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Jul 19, 2021
@ciarmcom
Copy link
Member

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

@ciarmcom ciarmcom requested a review from a team July 19, 2021 13:30
0xc0170
0xc0170 reviewed Jul 19, 2021
View reviewed changes
platform/include/platform/mbed_retarget.h
@@ -37,7 +37,7 @@
#endif
#include <errno.h>

#if !defined(__ARMCC_VERSION)
#if !defined(__ARMCC_VERSION) && defined(__GNUC__)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this addition needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#14751 (comment)

@ciarmcom ciarmcom requested a review from a team July 19, 2021 14:00
@ciarmcom
Copy link
Member

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

@mergify mergify bot added needs: CI and removed needs: review labels Jul 20, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Jul 21, 2021

CI started

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 21, 2021

CI restarted. I accidentally started 5.15 job.

@mbed-ci
Copy link

mbed-ci commented Jul 21, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

0xc0170
0xc0170 requested changes Jul 21, 2021
View reviewed changes
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.

I remembered the discussion from the 14751, the second part of the change should be reverted.

platform/include/platform/mbed_retarget.h

#if defined ( __ICCARM__ )
#undef EILSEQ /* defined in IAR arm/inc/c/errno.h: #define EILSEQ 36 */
#define EILSEQ 84 /* Illegal byte sequence */
Copy link
Contributor

@0xc0170 0xc0170 Jul 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we want to add this. From the discussion, I was questioning our earlier approach with undef like here (see #14751).

The goal was to provide default values only if they are not defined. if IAR defines it, we should leave it as it is. a program should not use value 36 or 84 directly but rather use EILSEQ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But this can't compile as value 36 is already used....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is failing at https://github.com/ARMmbed/mbed-os/blob/master/platform/source/mbed_retarget.cpp#L2102
as there are 2 cases with the same value

Copy link
Contributor

Choose a reason for hiding this comment

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

Good test !

@kjbracey-arm what do to in this case ? fix it as it is here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

or @pan- but I think both are OoO these days.

Lets get back to this one the next week.

@mergify mergify bot added the needs: work label Jul 21, 2021
@ciarmcom ciarmcom added the stale Stale Pull Request label Aug 2, 2021
@ciarmcom
Copy link
Member

ciarmcom commented Aug 2, 2021

This pull request has automatically been marked as stale because it has had no recent activity. @jeromecoutant, please carry out any necessary work to get the changes merged. Thank you for your contributions.

@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Aug 6, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Aug 12, 2021

@pan- Can yo help us here? See #14935 (comment)

@0xc0170 0xc0170 requested a review from pan- August 12, 2021 15:01
@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Aug 13, 2021
@jeromecoutant
Copy link
Collaborator Author

Note this has no impact for current GCC or ARM build,
please make sure this is merged before next release.

@0xc0170 0xc0170 self-requested a review August 16, 2021 08:17
@0xc0170 0xc0170 added needs: review and removed needs: work stale Stale Pull Request labels Aug 16, 2021
@0xc0170 0xc0170 merged commit 2ceb3bf into ARMmbed:master Aug 17, 2021
@mergify mergify bot removed the ready for merge label Aug 17, 2021
@jeromecoutant jeromecoutant deleted the PR_IAR branch August 17, 2021 12:07
@mbedmain mbedmain added release-version: 6.14.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@pan- pan- Awaiting requested review from pan-

Assignees
No one assigned
Labels
component: core release-version: 6.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jeromecoutant @ciarmcom @0xc0170 @mbed-ci @mbedmain