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

Fix for secure partition #9939

Merged
merged 4 commits into from
Mar 7, 2019
Merged

Fix for secure partition #9939

merged 4 commits into from
Mar 7, 2019

Conversation

NirSonnenschein
Copy link
Contributor

Description

Fix issues found in secure crypto service by compliance tests and update the default pre-built image for PSOC6.

targeted at 5.12RC2

Pull request type

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

Reviewers

@itayzafrir @mikisch81 @avolinski

Release Notes

Copy link
Contributor

@mikisch81 mikisch81 left a comment

Choose a reason for hiding this comment

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

Can you also update the README of the prebuilt folder with the command to build the acl tests

@ciarmcom ciarmcom requested review from alekshex, itayzafrir, mikisch81 and a team March 5, 2019 14:00
@ciarmcom
Copy link
Member

ciarmcom commented Mar 5, 2019

@NirSonnenschein, thank you for your changes.
@itayzafrir @mikisch81 @avolinski @ARMmbed/mbed-os-maintainers please review.

Copy link
Contributor

@itayzafrir itayzafrir left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mikisch81 mikisch81 left a comment

Choose a reason for hiding this comment

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

Can you also update the README of the prebuilt folder with the command to build the acl tests

@NirSonnenschein
Copy link
Contributor Author

@mikisch81 it is already there, added by @orenc17 before this PR.

@ghost ghost added the PM_ACCEPTED label Mar 5, 2019
Copy link
Contributor

@orenc17 orenc17 left a comment

Choose a reason for hiding this comment

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

LGTM

@cmonr
Copy link
Contributor

cmonr commented Mar 5, 2019

CI started for now.

Suspect export issue is still a problem, but that job can be restarted once fixed.

@mbed-ci
Copy link

mbed-ci commented Mar 5, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 6, 2019

Restarted exporters

@cmonr
Copy link
Contributor

cmonr commented Mar 6, 2019

CI job restarted: jenkins-ci/exporter

@orenc17
Copy link
Contributor

orenc17 commented Mar 6, 2019

@ARMmbed/mbed-os-maintainers can you run the exporters again?

@cmonr
Copy link
Contributor

cmonr commented Mar 6, 2019

@orenc17 Will do in a bit.

Testing with a different PR, but it's possible a complete rebuild might be needed.
Restarting just the exporter job appears to not be working as expected.

@alekla01
Copy link
Contributor

alekla01 commented Mar 6, 2019

@cmonr If restarting does not work for some reason (and is required for some reason, i.e. the Job has failed due to e.g. instability of some sort and is not still exexuting due to e.g. congestion), you can manually build it with parameters and copy the parameters from the old build.

@cmonr
Copy link
Contributor

cmonr commented Mar 6, 2019

@alekla01 My typical workflow is to go into the Jenkins UI and click Rebuild instead of using Blue Ocean's Rerun option.

The last issue I saw was here: #9312 (comment)

you can manually build it with parameters and copy the parameters from the old build.
Will keep that in mind next time.

@alekla01
Copy link
Contributor

alekla01 commented Mar 6, 2019

@cmonr, yes, i know, and it should Be OK, but If it is not OK for some reason, error of some sort, you can build it with parameters (select the Job but no build and it should Be in place of rebuild) instead of rebuilding, it does basically the same thing thing, i don't currently have acceess to the Jenkins, but you should at least check it's not already exexuting before either building or rebuilding, and act accordingly, If it is not executing, you should Be at least able to build it If not rebuild currently for some reason.

@cmonr
Copy link
Contributor

cmonr commented Mar 6, 2019

Kk. Will retry this current export job with a manual rerun once it completes.

It appears that it's about to fail again.

@alekla01
Copy link
Contributor

alekla01 commented Mar 6, 2019

How so, If it timeouts due to node connection error, instead of license, unless the license issues are back, etc, it'll retry the failed targets automatically. If couse If it fails due to some sort of ci error, it required restarting, and potentially ci fix of some sort. Though, of course If it seems to Be failing, it May require abort/fix/rebuild.

@cmonr
Copy link
Contributor

cmonr commented Mar 6, 2019

Finally got a chance to dig into the failures, and now I feel silly for the export-only restarts.

Warning: A3912W: Option 'c' is deprecated.
<built-in>:388:10: fatal error: 'mbed_config.h' file not found
#include "./mbed_config.h"
         ^~~~~~~~~~~~~~~~~
1 error generated.

This PR will need to run through CI again, since master has a fix for this problem, but the merge for this PR in Jenkins does not.

@ARMmbed/mbed-os-maintainers Any reason why anyone else didn't look at the logs? Going to check the other export-only PRs needing restart.

@cmonr
Copy link
Contributor

cmonr commented Mar 6, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Mar 7, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 2
Build artifacts

@cmonr cmonr merged commit 63242cf into ARMmbed:master Mar 7, 2019
@cmonr cmonr removed the needs: CI label Mar 7, 2019
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.

None yet

10 participants