Skip to content

CMake: remove version number from ARM toolchain #13316

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
Merged
merged 1 commit into from
Jul 23, 2020
Merged

CMake: remove version number from ARM toolchain #13316

merged 1 commit into from
Jul 23, 2020

Conversation

@hugueskamba
Copy link
Collaborator

@hugueskamba hugueskamba commented Jul 20, 2020

Summary of changes

The version number was required previously when ARM Compiler 5
and ARM Compiler 6 were both supported. There was a significant
underlying change between the two that justified having distinct
build options settinfgs. It is very unlikely that such a major change will happen
between ARM Compiler 6 and future versions. It is therefore also very unlikely
that distinct option settings for future versions will be needed.
We should therefore avoid appending version number the same way we do not
append it to GCC toolchain configuration settings.

Impact of changes

Migration actions required

Documentation

Pull request type 

[√] 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)
[√] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

0xc0170
0xc0170 previously approved these changes Jul 20, 2020
View reviewed changes
@mergify mergify bot added the needs: work label Jul 20, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Jul 20, 2020

@hugueskamba Please rebase, travis should be made green again (although if tools are passing not ARM, then we might need to wait for the new tools update).

@hugueskamba hugueskamba force-pushed the hk_cmake_remove_arm_version_number branch from 44b4896 to cd01e36 Compare July 20, 2020 14:15
@mergify mergify bot dismissed 0xc0170’s stale review July 20, 2020 14:15

Pull request has been modified.

@hugueskamba
Copy link
Collaborator Author

hugueskamba commented Jul 20, 2020

@0xc0170 This force-push rebases from feature-cmake.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 20, 2020

cmake test passed (it is testing gcc arm only at the moment, so not much testing done here) 🙄 will be soon.

@ARMmbed/mbed-os-tools please review

rwalton-arm
rwalton-arm previously approved these changes Jul 20, 2020
View reviewed changes
@mergify
Copy link

mergify bot commented Jul 20, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

@mergify mergify bot added needs: work and removed needs: CI labels Jul 20, 2020
@hugueskamba hugueskamba force-pushed the hk_cmake_remove_arm_version_number branch from cd01e36 to dbf73fa Compare July 20, 2020 18:20
@mergify mergify bot dismissed rwalton-arm’s stale review July 20, 2020 18:21

Pull request has been modified.

@hugueskamba
Copy link
Collaborator Author

hugueskamba commented Jul 21, 2020

@0xc0170, are the failures expected or due to my changes? I am not sure.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 21, 2020

I dont think so. Can you rebase just in case? I dont see this failure in #13321 (different one though).

@mergify
Copy link

mergify bot commented Jul 21, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 21, 2020

Let's rebase, travis should be fixed on feature-cmake now.

@hugueskamba hugueskamba force-pushed the hk_cmake_remove_arm_version_number branch from dbf73fa to f5e8d5e Compare July 21, 2020 13:09
@hugueskamba
Copy link
Collaborator Author

hugueskamba commented Jul 21, 2020

Let's rebase, travis should be fixed on feature-cmake now.

Done.

@hugueskamba
Copy link
Collaborator Author

hugueskamba commented Jul 21, 2020

@0xc0170 We still have failures. Any idea?

@mergify
Copy link

mergify bot commented Jul 21, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 22, 2020

Once you rebase this one, let me know. I'll review the failures if there are any. There should not be any in Travis

@hugueskamba hugueskamba force-pushed the hk_cmake_remove_arm_version_number branch from f5e8d5e to f21cac2 Compare July 22, 2020 11:04
@hugueskamba
Copy link
Collaborator Author

hugueskamba commented Jul 22, 2020

Once you rebase this one, let me know. I'll review the failures if there are any. There should not be any in Travis

Done.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 22, 2020

Scripts must be picking up something, I'll try to reproduce locally.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 22, 2020

I am trying to fetch this locally and getting merge conflicts with feature-cmake, was this properly rebased to the latest?

@0xc0170 0xc0170 mentioned this pull request Jul 22, 2020
Closed
@0xc0170
Copy link
Contributor

0xc0170 commented Jul 22, 2020

I cherry picked this one and applied to the recent feature-cmake, created #13336 to test the theory

@hugueskamba
Copy link
Collaborator Author

hugueskamba commented Jul 22, 2020

I cherry picked this one and applied to the recent feature-cmake, created #13336 to test the theory

That's exactly what I did.

@mergify
Copy link

mergify bot commented Jul 22, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

@hugueskamba
CMake: remove version number from ARM toolchain
ef6c688 
The version number was required previously when ARM Compiler 5
and ARM Compiler 6 were both supported. There was a significant
underlying change between the two that justified having distinct
build options settinfgs. It is very unlikely that such a major change will happen
between ARM Compiler 6 and future versions. It is therefore also very unlikely
that distinct option settings for future versions will be needed.
We should therefore avoid appending version number the same way we do not
append it to GCC toolchain configuration settings.
@hugueskamba hugueskamba force-pushed the hk_cmake_remove_arm_version_number branch from f21cac2 to ef6c688 Compare July 22, 2020 17:16
@hugueskamba
Copy link
Collaborator Author

hugueskamba commented Jul 22, 2020

@0xc0170 All seems ok now.

@mergify mergify bot added needs: CI and removed needs: work labels Jul 23, 2020
@0xc0170 0xc0170 merged commit ef6ba14 into ARMmbed:feature-cmake Jul 23, 2020
@mergify mergify bot removed the ready for merge label Jul 23, 2020
@hugueskamba hugueskamba deleted the hk_cmake_remove_arm_version_number branch July 23, 2020 08:36
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

@Patater Patater Awaiting requested review from Patater

@urutva urutva Awaiting requested review from urutva

+1 more reviewer

@rwalton-arm rwalton-arm rwalton-arm left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release-version: feature branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hugueskamba @0xc0170 @rwalton-arm