zephyrproject-rtos / west

West, Zephyr's meta-tool
https://docs.zephyrproject.org/latest/guides/west/index.html
Apache License 2.0
231 stars 122 forks source link

west manifest: detect when target directory already exists, and fail #676

Closed attie-argentum closed 1 year ago

attie-argentum commented 1 year ago

This detection would ideally occur before the lengthy clone operation, but manifest_path is derived from the files retrieved...

EDIT: see newer commit message

mbolivar-ampere commented 1 year ago

Thanks and sorry for the long review time. I agree that it's not clear that this PR fixes #558 but that it indeed looks like a good fix. (To be honest, I'm tempted to close 558 because it seems like many of the issues there have been diagnosed as environment related problems on Windows that are outside of west's control...)

marc-hb commented 1 year ago

Also, the Fixes: #558 syntax is a Github joke: https://github.com/orgs/community/discussions/17308#discussioncomment-6842022

attie-argentum commented 1 year ago

Very fair comments - thanks both.

Apologies for not tagging anyone, I honestly submitted this and forgot about it. 🤦‍♀️

I've made changes that hopefully address your concerns.


I'm tempted to close 558 because it seems like many of the issues there have been diagnosed as environment related problems on Windows that are outside of west's control

From my reading of #558, I'd agree with that path forwards - locking on Windows, due to other tools.

Also, the Fixes: #558 syntax is a Github joke

Wow, I've seen some stuff spill over from forks elsewhere, but that's not good! Removed.

attie-argentum commented 1 year ago

No problem - I've made both of these changes too.

marc-hb commented 1 year ago

Proof that this new error message is useful: #684