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

Remove uVisor from mbed-os #7592

Merged
merged 1 commit into from
Aug 26, 2018
Merged

Remove uVisor from mbed-os #7592

merged 1 commit into from
Aug 26, 2018

Conversation

orenc17
Copy link
Contributor

@orenc17 orenc17 commented Jul 25, 2018

Remove uVisor from mbed-os

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[X] Breaking change

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.

One question about the test, if its fine for removal

@@ -1,8 +1,3 @@
{
"name": "lib2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing tools/test/config/feature_recursive_add test as it uses uvisor feature, wont need any replacement?

Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

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

Yes, lets do that! (Someone else needs to review the details)

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 2, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Aug 2, 2018

Build : FAILURE

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

@orenc17
Copy link
Contributor Author

orenc17 commented Aug 2, 2018

@0xc0170 @cmonr
please build again, i've fixed the IAR bug

@cmonr
Copy link
Contributor

cmonr commented Aug 2, 2018

@orenc17 Thanks for the quick fix, but please also fix the intentation in that particular function.

On a quick glance, it was not obvious why that was a fix.

@@ -712,7 +712,6 @@ void __iar_program_start( void )
low_level_init_needed_local = __low_level_init();
if (low_level_init_needed_local) {
__iar_data_init3();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What is an error that this is actually fixing? I don't think this is related to uvisor anyhow.

By removing }, the lines 717 and 718 becomes unaligned.

@orenc17 orenc17 force-pushed the remove_uvisor branch 3 times, most recently from 2556c64 to feb9362 Compare August 4, 2018 20:00
@NirSonnenschein
Copy link
Contributor

/morph build

@mbed-ci
Copy link

mbed-ci commented Aug 5, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Aug 5, 2018

@mbed-ci
Copy link

mbed-ci commented Aug 5, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 6, 2018

@orenc17 @alzix Assuming with this PR the uvisor build & test should not be part of the build command . Please advise how to proceed.

@orenc17
Copy link
Contributor Author

orenc17 commented Aug 6, 2018

@studavekar FYI

@studavekar
Copy link
Contributor

Once this PR gets in we would need to

  • Disable uvisor-test command form post mbed-os success ccomment
  • Remove AWS-CI uVisor Build & Test from mbed-os setting page

cc @ARMmbed/mbed-os-maintainers for suggestions, how do we take this PR formward.

@cmonr
Copy link
Contributor

cmonr commented Aug 6, 2018

As much as we'd like to move this forward, removing uVisor at this point would cause one of two things to happen:

  1. introduce a major change into 5.9, when uVisor support is marked as deprecated
    -OR-
  2. cause PRs to come into master that might not be supported in the patch release because the uVisor test still runs in 5.9 CI.

The safest time to pull this in will probably be immediately after the last patch release is merged, at which point we won't have to worry about the 5.9 branch's CI.

@cmonr
Copy link
Contributor

cmonr commented Aug 6, 2018

I think the solution for now is to park this PR until the second to last patch release is made.

@mikisch81
Copy link
Contributor

Is 5.9.5 the last patch release of 5.9?
If so than I think now is the time (before all code freeze).

@cmonr
Copy link
Contributor

cmonr commented Aug 17, 2018

@mikisch81 Sorry, it's not. 5.9.6 will be happening next week, then 5.9.7 code freeze (the last patch) will be on Sept 6th.

@ARMmbed/mbed-os-maintainers

@bulislaw
Copy link
Member

Can we get that in for 5.10?

@alzix
Copy link
Contributor

alzix commented Aug 22, 2018

This was the plan every one agreed on.

@orenc17
Copy link
Contributor Author

orenc17 commented Aug 22, 2018

Rebased on top of master and remover #7830

@cmonr cmonr added the risk: G label Aug 23, 2018
@cmonr
Copy link
Contributor

cmonr commented Aug 24, 2018

Alright, so here's the plan. This is ready to go (aside from another run of CI since rebasing updates commit hashes),

We need this to come in as a part of 5.10, but not come into the 5.9.x branch. We should be able to do that by simply leaving the 5.10 label, and getting this merged before then.

Once this comes in, we'll remove the "AWS-CI uVisor Build & Test" merge check from the master branch, but keep it in the 5.9 branch.

For 5.9.7, if we encounter any patches that for some reason don't manage to pass "AWS-CI uVisor Build & Test" once the release PR is being made, we'll remove it from the patch and only bring it in for 5.10.

After 5.9.7 has been released, the Jenkins job(s) behind "AWS-CI uVisor Build & Test" can be removed.

@orenc17 @alzix @ARMmbed/mbed-os-maintainers Sound like a plan?

@alzix
Copy link
Contributor

alzix commented Aug 24, 2018

Finally someone with a plan. LGTM.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 24, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Aug 24, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Aug 24, 2018

@cmonr
Copy link
Contributor

cmonr commented Aug 24, 2018

Ooph. License checkout issues.

/morph export-build

@mbed-ci
Copy link

mbed-ci commented Aug 25, 2018

@cmonr
Copy link
Contributor

cmonr commented Aug 25, 2018

/morph uvisor-test

@alzix
Copy link
Contributor

alzix commented Aug 25, 2018

What do we expect uvisor-test check to do when this PR removed uVisor?
Instead we need to tweak GitHub branch protection to remove uVisor tests as a mandatory precondition.

@mbed-ci
Copy link

mbed-ci commented Aug 25, 2018

@cmonr
Copy link
Contributor

cmonr commented Aug 25, 2018

@alzix My mistake, was on autopilot.

@cmonr
Copy link
Contributor

cmonr commented Aug 26, 2018

Bringing this in. I disabled the test in GitHub for the master branch, but I guess the check isn't going away since it was run for this PR.

@ARMmbed/mbed-os-maintainers Make note that uVisor will not be tested on master, and if a PR fails for 5.9.7, it will be taken out of the patch.

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.

9 participants