MycroftAI / mycroft-core

Mycroft Core, the Mycroft Artificial Intelligence platform.
https://mycroft.ai
Apache License 2.0
6.49k stars 1.27k forks source link

Do not overwrite new path if it already exists #3046

Closed krisgesling closed 2 years ago

krisgesling commented 2 years ago

Description

In the migration to XDG compatible paths, we move anything from the old path to the new path. However if for some reason both paths exist the old path would overwrite the new one.

This should only really happen for devs who are switching back and forth between branches. So just an extra precaution.

How to test

With an existing install pre-paired but stopped:

mkdir ~/.mycroft/identity
mv ~/.config/mycroft/identity/* ~/.mycroft/identity/
touch ~/.config/mycroft/identity/test
mycroft-start all

Prior to this change - the test file will get wiped when the old path overwrites the new one.

Contributor license agreement signed?

devops-mycroft commented 2 years ago

Voight Kampff Integration Test Succeeded (Results)

codecov-commenter commented 2 years ago

Codecov Report

Merging #3046 (607a471) into dev (ae99974) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #3046   +/-   ##
=======================================
  Coverage   53.11%   53.11%           
=======================================
  Files         123      123           
  Lines       11188    11188           
=======================================
  Hits         5942     5942           
  Misses       5246     5246           
Impacted Files Coverage Δ
mycroft/filesystem/__init__.py 80.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ae99974...607a471. Read the comment docs.

forslund commented 2 years ago

This was discussed in the big XDG-PR, the reasoning for leaving it like this was in case a move didn't finish and a corrupted version exists in the new location. Then it was considered it would be better to move again to ensure that a valid version gets to the new location.

Though this can of course be reconsidered

krisgesling commented 2 years ago

Yeah I can get that logic too.

Just hitting a real issue on the Mark II as presently stable is pre-xdg and latest is post-xdg so if a device switches between them the pairing info gets blown away.

I think this is a very short term problem and happy to just do this on the Mark II feature branch. The whole problem will disappear pretty soon once we no longer have the pre-xdg code on active devices.

krisgesling commented 2 years ago

I've also added in a symlink at the old directory to try and handle both upgrading from pre-XDG to XDG, and reversion from XDG to non-XDG. https://github.com/MycroftAI/mycroft-core/commit/294be08efcd89c0e2737e2be6d926d366134bce8

Eg if someone on the Mark II switches from Latest back to Stable - they will be moving from an XDG migrated system, back to a pre-XDG system.

It won't catch if you start on a system that's never migrated from pre-XDG, but I think core devs are the only people doing that anyway. And as I said, very soon this all becomes a thing of the past.