Skip to content

scripts: runners: bossac: Enable BOSSAC to run on Windows (native) #53783

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
Mar 6, 2023
Merged

scripts: runners: bossac: Enable BOSSAC to run on Windows (native) #53783

merged 1 commit into from
Mar 6, 2023

Conversation

@joaodullius
Copy link
Contributor

@joaodullius joaodullius commented Jan 13, 2023

Fixes #37538 by correctly detecting WSL and blocking bossac from running on that platform, but allowing it to run on Windows native.

This is a new PR superseding the original one (#37584). Unfortunately the original fork was removed.

I implemented all the requests from @nandojve in the original PR.

Signed-off-by: João Dullius [email protected]

@zephyrbot zephyrbot added the area: West West utility label Jan 13, 2023
@joaodullius joaodullius changed the title Enable BOSSAC to run on Windows (native) scripts: runners: bossac: Enable BOSSAC to run on Windows (native) Jan 13, 2023
@joaodullius joaodullius force-pushed the bossac_on_windows branch 4 times, most recently from 3f2e278 to 9ecfafa Compare January 13, 2023 20:35
@nandojve
Copy link
Member

nandojve commented Jan 17, 2023

Hi @joaodullius ,

I will check this PR in regards of #37584 (review) .

@mbolivar-nordic
Copy link
Contributor

mbolivar-nordic commented Feb 24, 2023

@nandojve does your above comment mean you want this commit merged via another PR? I'm confused about the status of this PR otherwise.

@nandojve
Copy link
Member

nandojve commented Feb 24, 2023

I have stated on the #37584 few things that still not here. By what I look it seems it still missing the documentation about what user should do to proper configure environment for Windows. That should be made at a new topic Windows Native/WSL configuration at Host-Tools / BOSSAC. Then it is necessary instruct what user should do in case of WSL. User should be aware about limitations of WSL. This should go as [Experimental] since Windows is a beast and it is very difficult to handle all corner cases at once.

@mbolivar-nordic
Copy link
Contributor

mbolivar-nordic commented Feb 24, 2023

That should be made at a new topic Windows Native/WSL configuration at Host-Tools / BOSSAC. Then it is necessary instruct what user should do in case of WSL.

Is it? I'm asking because our only officially supported environment on windows is cmd.exe, not WSL or powershell.

@zephyrbot zephyrbot requested a review from kartben February 27, 2023 12:48
@joaodullius joaodullius force-pushed the bossac_on_windows branch 4 times, most recently from 7e2d109 to ba4f1c3 Compare February 27, 2023 12:53
@joaodullius
Copy link
Contributor Author

joaodullius commented Feb 27, 2023

Added the documentation requested by @nandojve and rebased.

@joaodullius joaodullius force-pushed the bossac_on_windows branch 2 times, most recently from 145ed3d to 952826a Compare February 27, 2023 19:19
@joaodullius joaodullius marked this pull request as draft February 27, 2023 19:37
@mbolivar-nordic
Copy link
Contributor

mbolivar-nordic commented Feb 27, 2023

@joaodullius just FYI, I am manually re-approving CI to run every time you force push, so it would be good if you can try to fix as many issues per push as you can.

@joaodullius
Copy link
Contributor Author

joaodullius commented Feb 27, 2023 •
edited
Loading

@joaodullius just FYI, I am manually re-approving CI to run every time you force push, so it would be good if you can try to fix as many issues per push as you can.

Sorry @mbolivar-nordic. I'm new to the PR process. I changed the PR to Draft, I don´t know if it makes less interrupts to you this way.
Will also use "make html" in docs to validate before pushing again.

@mbolivar-nordic
Copy link
Contributor

mbolivar-nordic commented Feb 27, 2023 •
edited
Loading

It's not about the interruptions -- it's that since the GitHub account you are using has never had a pull request merged, we have to manually approve CI runs every time you push in order for you to get results. If we don't approve, the CI jobs are never run.

This is a one-time limitation. After the first PR from your account is merged, you'll start to get CI results every time you push.

For this reason, I want to keep approving the runs so that you can fix any issues, but it would be more efficient if you can fix as many as you can each time you push, as you won't get new results again until someone re-approves the run after the push.

@joaodullius joaodullius marked this pull request as ready for review February 27, 2023 21:26
@joaodullius joaodullius force-pushed the bossac_on_windows branch 2 times, most recently from 47609f8 to 8104917 Compare February 28, 2023 16:20
nandojve
nandojve requested changes Mar 1, 2023
View reviewed changes
@joaodullius
Copy link
Contributor Author

joaodullius commented Mar 1, 2023

Force pushed changes requested by @nandojve

@nandojve
Copy link
Member

nandojve commented Mar 2, 2023

Force pushed changes requested by @nandojve

Hi @joaodullius ,

I'll check this soon.

nandojve
nandojve requested changes Mar 5, 2023
View reviewed changes
@joaodullius
scripts: runners: bossac: Enable BOSSAC to run on Windows (native)
5545a5a 
Fixes zephyrproject-rtos#37538 by correctly detecting WSL and blocking bossac from running
on that platform, but allowing it to run on Windows native.

Signed-off-by: João Dullius <[email protected]>
@joaodullius
Copy link
Contributor Author

joaodullius commented Mar 6, 2023

@nandojve I force pushed again with latest fix requests.

nandojve
nandojve approved these changes Mar 6, 2023
View reviewed changes
Copy link
Member

@nandojve nandojve left a comment

Choose a reason for hiding this comment

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

LGTM!

Thank you for the changes.

@nandojve nandojve added this to the v3.4.0 milestone Mar 6, 2023
@nandojve nandojve added the Release Notes To be mentioned in the release notes label Mar 6, 2023
@mbolivar-nordic mbolivar-nordic merged commit 79d9f45 into zephyrproject-rtos:main Mar 6, 2023
@joaodullius
Copy link
Contributor Author

joaodullius commented Mar 7, 2023

Thanks @mbolivar-nordic and @nandojve !

@joaodullius joaodullius deleted the bossac_on_windows branch March 7, 2023 14:09
@2bndy5 2bndy5 mentioned this pull request Apr 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nandojve nandojve nandojve approved these changes

@nashif nashif Awaiting requested review from nashif

@carlescufi carlescufi Awaiting requested review from carlescufi

@swinslow swinslow Awaiting requested review from swinslow

@kartben kartben Awaiting requested review from kartben

+1 more reviewer

@mbolivar-nordic mbolivar-nordic mbolivar-nordic approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mbolivar-nordic mbolivar-nordic

Labels

area: Documentation area: West West utility Release Notes To be mentioned in the release notes

Projects

None yet

Milestone

v3.4.0

Development

Successfully merging this pull request may close these issues.

BOSSAC support on Windows

5 participants

@joaodullius @nandojve @mbolivar-nordic @kartben @zephyrbot