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.

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

Cypress: Rename CY8CKIT_064B0S2_4343W to CY8CKIT064B0S2_4343W #13516

Merged
merged 1 commit into from
Sep 9, 2020

Conversation

romanjoe
Copy link
Contributor

@romanjoe romanjoe commented Aug 31, 2020

Summary of changes

This PR addresses issue #13424. Target name for existing CY8CKIT_064B0S2_4343W is changed to CY8CKIT064B0S2_4343W to meet requirement of 20 characters in name.

Impact of changes

Change of target name

Migration actions required

Any CI or automated test environment should use new name CY8CKIT064B0S2_4343W

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


@mergify mergify bot added the needs: work label Aug 31, 2020
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Aug 31, 2020
@ciarmcom ciarmcom requested review from maclobdell and a team August 31, 2020 20:00
@ciarmcom
Copy link
Member

@romanjoe, thank you for your changes.
@maclobdell @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-test please review.

@romanjoe
Copy link
Contributor Author

romanjoe commented Sep 1, 2020

There are some CI fails related to "frozen tools". I'm not familiar with that unfortunately. I checked https://os.mbed.com/blog/entry/Introducing-the-new-Mbed-Tools/ page and assume it is related to mbed-os 6.2+ maintenance, what should i do to make CI pass in this case?

0xc0170
0xc0170 previously approved these changes Sep 1, 2020
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 assume there won't be breakages as the old name was not available to users due to the limit?

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 1, 2020

CI started

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 1, 2020

There are some CI fails related to "frozen tools". I'm not familiar with that unfortunately. I checked https://os.mbed.com/blog/entry/Introducing-the-new-Mbed-Tools/ page and assume it is related to mbed-os 6.2+ maintenance, what should i do to make CI pass in this case?

I read your comment right after my review and noticed Travis failing.
tools/targets/PSOC6.py this is not allowed. This fix as stated in the issue unblocks enabling the target to be Mbed enabled. If this is merged, it won't be propagated to the online compiler. @romanjoe was this considered?

@ARMmbed/mbed-os-tools Please can you review this change?

@romanjoe
Copy link
Contributor Author

romanjoe commented Sep 1, 2020

There are some CI fails related to "frozen tools". I'm not familiar with that unfortunately. I checked https://os.mbed.com/blog/entry/Introducing-the-new-Mbed-Tools/ page and assume it is related to mbed-os 6.2+ maintenance, what should i do to make CI pass in this case?

I read your comment right after my review and noticed Travis failing.
tools/targets/PSOC6.py this is not allowed. This fix as stated in the issue unblocks enabling the target to be Mbed enabled. If this is merged, it won't be propagated to the online compiler. @romanjoe was this considered?

@ARMmbed/mbed-os-tools Please can you review this change?

Hi Martin,
Thank you for answer. Let me clarify a bit - this fix can be accepted and merged, it would unblock mbed enablement of this target.

This change however, would not be propagated to mbed online compiler, due to "frozen tools" check failure in this PR? Since this target is not yet supported in mbed online compiler i am not sure this is a problem right now.

@ifyall could you please confirm we are okay with not be present in online compiler?

@mbed-ci
Copy link

mbed-ci commented Sep 1, 2020

Jenkins CI Test : ❌ FAILED

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-ARM
jenkins-ci/mbed-os-ci_build-GCC_ARM

@romanjoe
Copy link
Contributor Author

romanjoe commented Sep 1, 2020

Jenkins CI failed. I have reviewed the cause and assume, that CI configuration may still use CY8CKIT_064B0S2_4343W - old name, instead of new one CY8CKIT064B0S2_4343W. Because of this, cysecuretools init command is not called and thus no policy file found on post build.

@mergify
Copy link

mergify bot commented Sep 2, 2020

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

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 2, 2020

This change however, would not be propagated to mbed online compiler, due to "frozen tools" check failure in this PR? Since this target is not yet supported in mbed online compiler i am not sure this is a problem right now.

Please confirm if this is true (this pull request is addressing an issue with mbed-cli, no online compiler update needed).

maclobdell
maclobdell previously approved these changes Sep 3, 2020
Copy link
Contributor

@maclobdell maclobdell left a comment

Choose a reason for hiding this comment

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

As far as I know, the online compiler cannot support this target due to special python package requirement (cysecuretools). In past discussions, Cypress only cares about supporting Mbed CLI.

I double checked that this aligns to the mbed-os-tools repo.

https://github.com/ARMmbed/mbed-os-tools/blob/9879d29212e3304626542182cae1fd5a9359713c/src/mbed_os_tools/detect/platform_database.py#L253

    u'1910': u'CY8CKIT064B0S2_4343W',

So it looks good to me.

@romanjoe
Copy link
Contributor Author

romanjoe commented Sep 3, 2020

This change however, would not be propagated to mbed online compiler, due to "frozen tools" check failure in this PR? Since this target is not yet supported in mbed online compiler i am not sure this is a problem right now.

Please confirm if this is true (this pull request is addressing an issue with mbed-cli, no online compiler update needed).

I confirm - this PR address mbed-cli.

@mergify mergify bot dismissed stale reviews from 0xc0170 and maclobdell September 7, 2020 05:27

Pull request has been modified.

@romanjoe
Copy link
Contributor Author

romanjoe commented Sep 7, 2020

PR branch rebased to latest master

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 7, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Sep 7, 2020

Jenkins CI Test : ❌ FAILED

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

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM
jenkins-ci/mbed-os-ci_build-ARM

@romanjoe
Copy link
Contributor Author

romanjoe commented Sep 7, 2020

CI failed again with similar issue in log - can't find policy file. Could someone please check CI config - if it is actually uses new target name and keys, policies are present in correct place (by default it is mbed-os/targets/TARGET_Cypress/TARGET_PSOC6/TARGET_CY8CKIT064B0S2_4343W/policies, mbed-os/targets/TARGET_Cypress/TARGET_PSOC6/TARGET_CY8CKIT064B0S2_4343W/keys), but can de overwritten from mbed_app.json, using field "policy_file": "%path_to_policy_file%"

it is expected, by default that keys are located level up from policy file location and in folder keys.

Example:

/user/home/policy/policy_single_CM0_CM4.json
/user/home/keys/USERAPP_CM4_KEY_PRIV.pem

"policy_file": "/user/home/policy/policy_single_CM0_CM4.json"

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 7, 2020

I'll talk to the test team now

@jamesbeyond
Copy link
Contributor

jamesbeyond commented Sep 7, 2020

Hi @romanjoe,
what is the target name in the cysecuretools ? is that have any changes? cy8ckit-064b0s2-4343w
I mean do I still run:

cysecuretools -t cy8ckit-064b0s2-4343w init
or 
cysecuretools -t cy8ckit064b0s2-4343w init

@romanjoe
Copy link
Contributor Author

romanjoe commented Sep 7, 2020

cysecuretools -t cy8ckit-064b0s2-4343w init

Hi, yes target name in cysecuretools is correct. Did you check if missed files are present on build machine, where CI runs? It is hard for me to understand what went wrong with CI now, because everything worked fine until these changes have been introduced.

Do you use mbed_app.json to override policy file location?

Per this error message
Policy file /builds/ws/mbed-os-ci_build-GCC_ARM@7/examples/mbed-os-example-blinky/policy_single_CM0_CM4.json not found. Aborting.

policy file location is not default.

@jamesbeyond
Copy link
Contributor

On CI target name changed, and CI restarted

@mbed-ci
Copy link

mbed-ci commented Sep 8, 2020

Jenkins CI Test : ✔️ SUCCESS

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

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-ARM ✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️

@romanjoe
Copy link
Contributor Author

romanjoe commented Sep 8, 2020

On CI target name changed, and CI restarted

Okay, great!

Thanks, CI finished successfully now!

@jamesbeyond
Copy link
Contributor

On CI target name changed, and CI restarted

Okay, great!

Thanks, CI finished successfully now!

Jenkins CI Passed, thought there still some issue, need to be fixed. if checking the successful log, the Greentea images didn't get build properly. due to the mbed-so folder restructure. will fix it and restart CI again soon.

However, the Travis CI did filed with tools freeze check, @0xc0170 @Patater what shall we do about this? ignore the failure?

@jamesbeyond
Copy link
Contributor

CI restarted

@mbed-ci
Copy link

mbed-ci commented Sep 8, 2020

Jenkins CI Test : ✔️ SUCCESS

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

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-ARM ✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@jamesbeyond
Copy link
Contributor

Now CI log looks much better! I'd Say Jenkins is passed, I'll let you deride what to do with TravisCI

@romanjoe
Copy link
Contributor Author

romanjoe commented Sep 8, 2020

Now CI log looks much better! I'd Say Jenkins is passed, I'll let you deride what to do with TravisCI

Thanks!

Please check this comment #13516 (review)

this target is not expected to work with online compiler anyways, changes in this PR are targeting mbed-cli, so this issue can be ignored per comments from @maclobdell and @0xc0170 (they both have approved PR)

@jamesbeyond
Copy link
Contributor

Now CI log looks much better! I'd Say Jenkins is passed, I'll let you deride what to do with TravisCI

Thanks!

Please check this comment #13516 (review)

this target is not expected to work with online compiler anyways, changes in this PR are targeting mbed-cli, so this issue can be ignored per comments from @maclobdell and @0xc0170 (they both have approved PR)

Thanks, but I think what is failing in Travis is have nothing to do with online compiler. it only have things to do with your PR changed the files inside tools folder, where travis-ci/frozen_tools_check mean to blocking it. @Patater has more details regarding this

Copy link

@MarceloSalazar MarceloSalazar left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 9, 2020

Approved by @Patater and @andypowers, I'll merge this now.

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

Successfully merging this pull request may close these issues.

8 participants