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

RFC: Clean workspace command and/or update option #310

Closed
bjda opened this issue Sep 26, 2019 · 8 comments
Closed

RFC: Clean workspace command and/or update option #310

bjda opened this issue Sep 26, 2019 · 8 comments
Labels
enhancement New feature or request invalid This doesn't seem right

Comments

@bjda
Copy link

bjda commented Sep 26, 2019

I have on multiple occasions had trouble compiling applications after checking out different versions and west updateing our multi-repo downstream (nRF Connect SDK), because of leftover repos and files. It has been fixable by deleting all the repos except the manifest repo, and running west update. This is especially prominent when using a single workspace and jumping significantly forward/backward in time.

I propose we introduce a west clean command that approximates a fresh workspace, without re-downloading all the repos. The steps could be:

  • Remove repos/folders not in the manifest (potentially even moving them to a caching folder)
  • Run git clean -fdx in each project folder.

Alternatively – or complementarily – adding this functionality as an option in west update could be useful. Then, west update --clean could be used to bring the rest of the workspace in line with the manifest repo with a single command.

There are several points of discussion, including but probably not limited to:

  • Do we even bother?
  • Is there some detail about the west/manifest/repo structure that makes implementing this impractical?
  • Do we call it clean, reset or something else
@carlescufi carlescufi added the enhancement New feature or request label Sep 26, 2019
@mbolivar
Copy link
Contributor

My first instincts are against this feature request.

  • west forall -c "git clean -fdx" (or a similar command like west forall -c "git reset --hard HEAD") is enough to clean out every repository west does know about. In general, I want to encourage use of forall for special purposes, keeping the number of built-in commands limited to "very common" use cases like diff and status.
  • I would be pretty terrified to introduce a built-in west command that walked the entire installation to find git repositories it didn't know about and just deleted them.
  • With a clean build directory, zephyr won't use any repositories that aren't managed by west, as ZEPHYR_MODULES is (by default) initialized using west list, and ZEPHYR_EXTRA_MODULES defaults to the empty list (refer to the zephyr docs for more info on these)

For Linux users (macOS ought to work too, unless its BSD find doesn't support some of the flags used here), here is a shell one-liner you can run to compare the git repositories west knows about with all the other ones inside the same workspace:

diff -u <(west list -f {abspath} | sort) <(find $PWD -name .git -type d | sed 's/\/\.git//' | sort)

You need to run it from the top level directory in the installation.

I can come up with a Windows equivalent if there's interest.

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 26, 2019

I would be pretty terrified to introduce a built-in west command that walked the entire installation to find git repositories it didn't know about and just deleted them.

+1 ! Especially dangerous on Linux systems that have no standard/built-in concept of a "Trash". Stay safe:

$ type -a rm
rm is aliased to `rm -i'
rm is /usr/bin/rm

Without going that far, I think one new feature and one change would make cleaning much easier:

  • Some official, scriptable and maintained west --show-top-level "API". Or did I miss it?
  • Extend west status to list unknown files and directories at the top level

Related: projects root should be configurable #237

Then, west update --clean could be used to bring the rest of the workspace in line with the manifest repo with a single command.

This looks safer because in this case west knows exactly which repos have been removed. There is still the risk of deleting a local branch that hasn't been shared or backed up anywhere.

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 26, 2019

I have on multiple occasions had trouble compiling applications after checking out different versions and west updateing our multi-repo downstream (nRF Connect SDK), because of leftover repos and files.

Yet:

With a clean build directory, zephyr won't use any repositories that aren't managed by west, as ZEPHYR_MODULES is (by default) initialized using west list, and ZEPHYR_EXTRA_MODULES defaults to the empty list (refer to the zephyr docs for more info on these)

So @bjda can you elaborate on what happened to you?

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 26, 2019

Fetching and using the new topdir command from PR #311 , I crafted this one-line "west status --untracked-files" equivalent:

diff -u <(west list -f {path} | awk -F/ '{print $1}' | sort -u) <(ls -1 $(west topdir))

--- /dev/fd/63	2019-09-26 12:32:40.501771362 -0700
+++ /dev/fd/62	2019-09-26 12:32:40.502771340 -0700
@@ -1,3 +1,6 @@
+spuriousfile
+spuriousdir
 modules
+net-tools
 tools
 zephyr

Funny enough it did find a spurious "net-tools" directory leftover, I guess from its older location

UPDATE: this scans only the top level. A more generic version would be much more complicated - and better implemented in west itself.

@mbolivar
Copy link
Contributor

$ type -a rm
rm is aliased to `rm -i'

Unrelated, but I personally think this sort of guard rail is not a good idea. Primarily because your muscle memory starts to rely on it, with potentially disastrous results on an unfamiliar server. It also can mess up expectations for things like scripts executed with source.

Some official, scriptable and maintained west --show-top-level "API". Or did I miss it?

You didn't, but here's one: #311

So @bjda can you elaborate on what happened to you?

I would also like some more information, as 'leftover' (meaning 'projects in the old manifest not known to the new one') projects should have no effect if your repositories and build directory are clean.

Then, west update --clean could be used to bring the rest of the workspace in line with the manifest repo with a single command.

It can't do what you want, I don't think.

West only parses the manifest file, exactly as it is on disk, so west update can only make the projects match what that file currently wants. It doesn't know what the file looked like "before", since the user is expected to manage the manifest repository.

So the only thing west could do to find "unknown" repositories is crawl the entire directory tree rooted at west topdir, applying heuristics like looking for .git directories. I don't think that is a good idea.

The "west never touches your manifest repository" guarantee was a design decision. Its main drawback is that it prevents features like this, but the benefits outweigh that drawback IMO..

This looks safer because in this case west knows exactly which repos have been removed.

So repo sync can do this, but west can't.

repo sync knows the manifest file contents before and after pulling the manifest repository remote. That lets it compare before vs. after and move project data that you no longer need somewhere under <topdir>/.repo.

As far as I can tell, this is one of the main reasons why the some-project/.git/{objects,refs,...} files created by repo sync are actually symlinks to somewhere inside <topdir>/.repo/project-objects/.

This use of symlinks is one of the main reasons we didn't use repo for zephyr: symlinks don't work on Windows :).

@mbolivar
Copy link
Contributor

Funny enough it did find a spurious "net-tools" directory leftover, I guess from its older location

Yep, it got moved in zephyr d3977996088a356107257adb828be649ec2fa7e3. The old one got left behind because west forgot it existed when you pulled zephyr to at least that version.

Moving projects around in a west manifest will always cause this sort of situation.

Note that nordic downstream does have a way to compare upstream and downstream:

https://github.com/NordicPlayground/fw-nrfconnect-nrf/blob/master/scripts/west_commands/ncs_commands.py#L220

(^^ @bjda you may be interested in that as well)

At some point, I expect that extension to mature to the point that we can merge it into mainline zephyr as a west diff-manifests or so, but it's currently got some features that are either downstream-specific or need more time to bake in my opinion.

@bjda
Copy link
Author

bjda commented Sep 27, 2019

@marc-hb When the issues have occured until now, I have just reinitialized to fix it and moved on. When there is time, I will try to reproduce the errors and post some specific examples

@mbolivar
Copy link
Contributor

@bjda please reopen if you can post some specific examples of incorrect behavior. In the meantime, I believe west forall plus the way that zephyr's build system uses west in a pristine build directory address your concerns.

@mbolivar mbolivar added wontfix This will not be worked on invalid This doesn't seem right and removed wontfix This will not be worked on labels Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

4 participants