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

[attie-argentum]

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

EDIT: see newer commit message

[marc-hb]

This looks like a good catch, thank you! Just a bit light on the English part of it :-) Can you please:

• Flesh out the commit message with a short description of how to reproduce the issue that this commit fixes. It's great that you reference west init completes but leaves everything in zephyr/manifest-tmp #558 but 558 has turned into a long and unreadable mess of multiple and possibly unrelated issues as recently noted there by someone named... "attie-argumentum" :-) Quoting: "I believe that the issue reported by X / Y / Z is different to the original issue". For now the commit message describes only what the fix does NOT do!

• Please add a one-line comment in the code, here's a suggestion below. Also, move the check between the mkdir and move commands which are closed related: them being close helps understand the issue too.

PS: next time please add some reviewers or "tag" some people.

--- a/src/west/app/project.py
+++ b/src/west/app/project.py
@@ -340,6 +340,10 @@ With neither, -m {MANIFEST_URL_DEFAULT} is assumed.
         self.dbg('moving', tempdir, 'to', manifest_abspath,
                  level=Verbosity.DBG_EXTREME)
         manifest_abspath.parent.mkdir(parents=True, exist_ok=True)
+        # What happens in that case is OS and filesystem-dependent; we
+        # _really_ don't want to go there.
+        if manifest_abspath.exists():
+            self.die(f'target directory already exists ({manifest_abspath})')
         try:
             shutil.move(os.fspath(tempdir), os.fspath(manifest_abspath))
         except shutil.Error as e:

[mbolivar-ampere]

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]

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

[attie-argentum]

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.

[marc-hb]

LGTM, thanks!

• I think you could/should still refer to 558 in the commit message, it's useful. I only said that the reference to 558 was not a substitute for a commit message.

• I would prefer the check to be after the self.dbg message but no big deal.

[attie-argentum]

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

[marc-hb]

Proof that this new error message is useful: #684