[PR196 3/3] Adding support for partial workspace cleaning #293

jbohren · 2016-02-12T06:59:15Z

This PR continues the refactoring of contributions from #196. It adds support for partial workspace cleaning, which takes advantage of the linked devel space pattern.

It varies slightly from the implementation in #196, and reorganizes some under-the-hood elements of catkin_tools:

devel and install manifests are stored in WS_ROOT/.catkin_tools/PROFILE/packages/PKG/
logs for each package are stored in WS_ROOT/.catkin_tools/PROFILE/logs/PKG/
linked devel collisions file is stored in WS_ROOT/.catkin_tools/PROFILE/
for linked devel, the actual isolated devel space is now referred to as the "private" devel space
private devel spaces are stored in DEVEL_SPACE/.private/PKG/
clean jobs are supplied by entry_points

Additionally, it includes the following changes:

clean's --dry-run prints out each file / directory which is to be unlinked or removed
adds CATKIN_TOOLS_FORCE_COLOR environment variable (this is to support [PR196 *] Adding documentation for forthcoming changes in 0.4.0 #250)
fixes alignment of catkin build skipped messages
fixes counter-intuitive development bug where jobserver might not be initialized properly
fixes bug with prebuild package scheduling

Tasks:

Full CLI interface:

usage: catkin clean [-h] [--workspace WORKSPACE] [--profile PROFILE]
                    [--dry-run] [--verbose] [--force] [-a] [-b] [-d] [-i]
                    [--deinit] [--deps] [--orphans] [-l] [--setup-files]
                    [PKGNAME [PKGNAME ...]]

Deletes various products of the build verb.

optional arguments:
  -h, --help            show this help message and exit
  --workspace WORKSPACE, -w WORKSPACE
                        The path to the catkin_tools workspace or a directory
                        contained within it (default: ".")
  --profile PROFILE     The name of a config profile to use (default: active
                        profile)
  --dry-run, -n         Show the effects of the clean action without modifying
                        the workspace.
  --verbose, -v         Verbose status output.
  --force, -f           Skip all interactive checks.

Basic:
  Clean workspace subdirectories.

  -a, --all             Remove all of the generated spaces associated with the
                        given or active profile. This will remove everything
                        but the source space and the hidden .catkin_tools
                        directory.
  -b, --build           Remove the entire buildspace.
  -d, --devel           Remove the entire develspace.
  -i, --install         Remove the entire installspace.
  --deinit              De-initialize the workspace, delete all build profiles
                        and configuration.

Packages:
  Clean products from specific packages in the workspace. These options will
  automatically enable the --force-cmake option for the next build
  invocation.

  PKGNAME               Explicilty specify a list of specific packages to
                        clean from the build, devel, and install space.
  --deps                Clean the packages which depend on other packages to
                        be cleaned.
  --orphans             Remove products from packages are no longer in the
                        source space. Note that this also removes packages
                        which are blacklisted or which contain `CATKIN_INGORE`
                        marker files.

Advanced:
  Clean only specific parts of the workspace for specified packages. These
  options will automatically enable the --force-cmake option for the next
  build invocation.

  -l, --logs            Only clear the catkin-generated logfiles.
  --setup-files         Clear the catkin-generated setup files from the devel
                        and install spaces.

…ment

…nctionality

wjwwood · 2016-02-18T21:39:07Z

@jbohren I have some questions about the design goals stated in the OP, I'll try to address them inline:

devel and install manifests are stored in WS_ROOT/.catkin_tools/PROFILE/packages/PKG/

These are files with a list of files installed to either the devel space or install space respectively or are they package.xml package manifests?

If the former, then why are they in a subfolder packages/<package name>/ rather than something like manifests/<package name>/ or install_manifests/<package name>.txt and devel_manifests/<package name>.txt?

logs for each package are stored in WS_ROOT/.catkin_tools/PROFILE/logs/PKG/

I assume this means the build logs? Why move them from the build space? (I thought the build space was quite an appropriate place for the logs)

linked devel collisions file is stored in WS_ROOT/.catkin_tools/PROFILE/

Is that in a single file or some other organization scheme?

wjwwood · 2016-02-18T21:40:20Z

for linked devel, the actual isolated devel space is now referred to as the "private" devel space
private devel spaces are stored in DEVEL_SPACE/.private/PKG/

Why not fall back to the default catkin behavior an let the devel space for each package be created in the package's build space, i.e. build/<package name>/devel?

wjwwood · 2016-02-18T21:41:47Z

clean jobs are supplied by entry_points

Can you elaborate on this more (bullet list of steps for the process)? You mean Python entry_points?

wjwwood · 2016-02-18T21:43:18Z

adds CATKIN_TOOLS_FORCE_COLOR environment variable (this is to support #250)

Having not looked at #250 in detail, why do you need an environment variable to do this? Can you not just use catkin --force-color ...?

wjwwood · 2016-02-18T21:44:37Z

That's all the questions I have so far. I'm going to start using it and testing it out locally.

Btw, this needs to be rebased against master to be mergeable.

jbohren · 2016-02-18T23:53:25Z

@jbohren I have some questions about the design goals stated in the OP, I'll try to address them inline:

devel and install manifests are stored in WS_ROOT/.catkin_tools/PROFILE/packages/PKG/

These are files with a list of files installed to either the devel space or install space respectively or are they package.xml package manifests?

If the former, then why are they in a subfolder packages/<package name>/ rather than something like manifests/<package name>/ or install_manifests/<package name>.txt and devel_manifests/<package name>.txt?

It is the former, these are like CMake install_manifest.txt files. I chose packages as the subdir name, but manifests or product_manifests could also be appropriate, but it might get cluttered if we wanted to store other package-specific information in there. For example, in the future there might be package-specific config defaults.

logs for each package are stored in WS_ROOT/.catkin_tools/PROFILE/logs/PKG/

I assume this means the build logs? Why move them from the build space? (I thought the build space was quite an appropriate place for the logs)

The build space was an appropriate place for the logs when the build verb was the only verb generating logs. With the more advanced clean verb, there are also clean logs. Additionally, a user might want logs to persist even after clearing the build space, so this puts them in a safer place.

linked devel collisions file is stored in WS_ROOT/.catkin_tools/PROFILE/

Is that in a single file or some other organization scheme?

That's a single file called devel_collisions.txt which lists files and their collision counts. This is a locked (synchronized) resource.

for linked devel, the actual isolated devel space is now referred to as the "private" devel space
private devel spaces are stored in DEVEL_SPACE/.private/PKG/

Why not fall back to the default catkin behavior an let the devel space for each package be created in the package's build space, i.e. build/<package name>/devel?

That could also be an appropriate place. It depends on whether you want the devel space to continue to work if the build space is removed.

clean jobs are supplied by entry_points

Can you elaborate on this more (bullet list of steps for the process)? You mean Python entry_points?

The job factories are registered along with build job factories like so:
https://github.com/catkin/catkin_tools/blob/pre-0.4.0-clean-pkgs/catkin_tools/jobs/catkin.py#L528-L533

These are gathered in the clean verb here:
https://github.com/catkin/catkin_tools/blob/pre-0.4.0-clean-pkgs/catkin_tools/verbs/catkin_clean/clean.py#L124-L127

adds CATKIN_TOOLS_FORCE_COLOR environment variable (this is to support #250)

Having not looked at #250 in detail, why do you need an environment variable to do this? Can you not just use catkin --force-color ...?

This is used to generate the asciicasts for the new documetation with colored output without having to put --force-color in front of all the commands: https://github.com/catkin/catkin_tools/blob/pre-0.4.0-docs/docs/examples/slowrun#L55

e.g.:

wjwwood · 2016-02-19T00:16:26Z

It is the former, these are like CMake install_manifest.txt files. I chose packages as the subdir name, but manifests or product_manifests could also be appropriate, but it might get cluttered if we wanted to store other package-specific information in there. For example, in the future there might be package-specific config defaults.

If the plan is to potentially put other package specific stuff there in the future, then that's probably ok.

The build space was an appropriate place for the logs when the build verb was the only verb generating logs. With the more advanced clean verb, there are also clean logs. Additionally, a user might want logs to persist even after clearing the build space, so this puts them in a safer place.

The clean logs use case is a compelling reason to not put them in the build space, but I worry that putting them in the .catkin_tools directory will mean they never get cleaned and just build up over time. Does the -l option to catkin clean clean the clean logs too (:dizzy_face:)?

What if we just put logs in a top level logs folder like we have for build, devel and install? It could still contain a folder hierarchy with profile names in the mix. For example:

% tree -L 2
.
├── build
│   ├── actionlib
│   ├── ...
│   └── xmlrpcpp
├── desktop-indigo.rosinstall
├── devel
│   ├── bin
│   ├── ...
│   └── share
├── install
│   ├── _setup_util.py
│   ├── ...
│   └── share
├── logs
│   ├── actionlib
│   ├── ...
│   └── xmlrpcpp
└── src
    ├── actionlib
    ├── ...
    └── xacro

And then either logs can get a suffix for each profile (like the build, devel, etc.) or it can be subdivided internally by profile and then package?

That could also be an appropriate place. It depends on whether you want the devel space to continue to work if the build space is removed.

Is that common? Maybe, but they could always specify the devel space root if they wanted it out of build. I'm not sure what the preferable default case would be, but I'm leaning towards the build space since it doesn't introduce a new folder and concept into the mix.

The job factories are registered along with build job factories like so:
https://github.com/catkin/catkin_tools/blob/pre-0.4.0-clean-pkgs/catkin_tools/jobs/catkin.py#L528-L533

These are gathered in the clean verb here:
https://github.com/catkin/catkin_tools/blob/pre-0.4.0-clean-pkgs/catkin_tools/verbs/catkin_clean/clean.py#L124-L127

I see, I thought you meant the actual instances of jobs, you just mean the specialization of clean for a particular build type.

This is used to generate the asciicasts for the new documetation with colored output without having to put --force-color in front of all the commands: https://github.com/catkin/catkin_tools/blob/pre-0.4.0-docs/docs/examples/slowrun#L55

Ok, so long as there was a reason. Sounds good to me.

Thanks for answering all my questions.

…ment

…nctionality

wjwwood · 2016-02-19T00:36:49Z

@jbohren I rebased against master and force pushed.

wjwwood · 2016-02-19T01:31:31Z

Some usability notes:

It's not clear from the documentation what the default behavior of catkin clean is with no extra arguments.
It's not clear from the console output that a dry run is happening. Perhaps there should be a message when -n is used.
When cleaning a space which doesn't exist, there is no output.
- I'm not sure if this is the right thing to do or not, but my initial reaction was that it should say it would have removed the install space (for example) but it does not exist.
--deinit doesn't seem to respect --dry-run.
catkin clean -n catkin doesn't seem to work, or rather catkin doesn't seem to have a private build space.
I'd recommend change the --deps option to be --dependents and -d for short, as --deps is commonly referred to as --dependencies.
Should there also be a --dependencies or --unique-dependencies? Like if I wanted to remove a package and all of its dependencies so long as another package does not use those dependencies? It's sort of like apt-get's autoremove but without the notion of selected versus implicit.

wjwwood · 2016-02-19T02:01:00Z

I just ran into this problem:

% catkin clean -n rviz --orphans
[clean] Determining orphaned packages...
[clean] Orphaned build products: .built_by
[clean] Note: Parts of the workspace have been cleaned which will necessitate re-configuring CMake on the next build.
Traceback (most recent call last):
  File "/usr/local/bin/catkin", line 9, in <module>
    load_entry_point('catkin-tools==0.3.1', 'console_scripts', 'catkin')()
  File "/Library/Python/2.7/site-packages/catkin_tools/commands/catkin.py", line 265, in main
    catkin_main(sysargs)
  File "/Library/Python/2.7/site-packages/catkin_tools/commands/catkin.py", line 260, in catkin_main
    sys.exit(args.main(args) or 0)
  File "/Library/Python/2.7/site-packages/catkin_tools/verbs/catkin_clean/cli.py", line 245, in main
    for pkg_devel_name in ctx.private_devel_path():
TypeError: 'str' object is not callable

The scenario was that I had an indigo desktop full workspace with everything built and configured to do the linked devel space.

jbohren · 2016-02-19T02:02:38Z

It's not clear from the documentation what the default behavior of catkin clean is with no extra arguments.

Yeah, we should get rid of the --all option, since that's the default behavior.

It's not clear from the console output that a dry run is happening. Perhaps there should be a message when -n is used.

+1

When cleaning a space which doesn't exist, there is no output.

I'm not sure if this is the right thing to do or not, but my initial reaction was that it should say it would have removed the install space (for example) but it does not exist.

+1

--deinit doesn't seem to respect --dry-run.

And it should.

catkin clean -n catkin doesn't seem to work, or rather catkin doesn't seem to have a private build space.

Yeah, currently prebuild packages are built directly into the devel space, but really they should be linked just like the others.

I'd recommend change the --deps option to be --dependents and -d for short, as --deps is commonly referred to as --dependencies.
catkin is

+1

Should there also be a --dependencies or --unique-dependencies? Like if I wanted to remove a package and all of its dependencies so long as another package does not use those dependencies? It's sort of like apt-get's autoremove but without the notion of selected versus implicit.

Maybe in the future.

… into pre-0.4.0-clean-pkgs

wjwwood · 2016-04-11T02:17:35Z

catkin_tools/verbs/catkin_clean/cli.py

+
+    # Check for all profiles option
+    if opts.all_profiles:
+        profiles = get_profile_names(workspace_path)


Neither get_profile_names nor workspace_path is defined here.

wjwwood · 2016-04-11T02:38:12Z

I made a few clean up commits.

wjwwood · 2016-04-11T02:41:34Z

catkin_tools/verbs/catkin_clean/clean.py

+        all_succeeded = run_until_complete(ej)
+    except Exception:
+        status_thread.keep_running = False
+        all_succeeded = False


all_succeeded doesn't appear to be used. Should it be? If not please remove it.

wjwwood · 2016-04-11T02:53:56Z

@jbohren the tool seems to work as expected to me. I think it would be best if you could comment on the few places I mentioned something in line and then merge this. We can follow up with new problems and UX problems in new issues against the master branch as we prepare for the 0.4 release.

… the workspace root (keep people from accidentally blowing away their system prefix)

…in_tools into pre-0.4.0-clean-pkgs

jbohren · 2016-04-12T21:35:53Z

@wjwwood I resolved the unused / missing elements you mentioned, and fixed the inconsistency that @NikolausDemmel mentioned.

Something else I did was replace the --force command with --yes since it better matches the relationship to the interactive prompt. Additionally, I realized that some users might try to shoot themselves in the foot and set up the installspace / destdir to be their root prefix. In this case I added a check (which can be overridden by --force) which prompts the user if any of the spaces to be removed are outside of the workspace root.

This should be good to merge, I'll update the documentation, and then I'll draft a call for testing / RFC that we can send out to ros-users.

wjwwood · 2016-04-12T21:40:19Z

@jbohren Thanks for the quick turn around. The changes look good to me, but the CI is failing. I'll test out the new CLI options in the meantime. If things look ok (and CI is passing) I'll merge.

jbohren · 2016-04-12T22:43:53Z

@wjwwood Yeah, I moved the cli check to the wrong place. 13aa6e5 passes locally, and should pass CI now.

jbohren · 2016-04-13T01:09:39Z

@wjwwood All tests passing.

wjwwood · 2016-04-13T01:11:05Z

Yup, I was just looking over it again. LGTM

wjwwood · 2016-04-13T01:11:37Z

🎉

…rfaces

- regenerating examples and cli interfaces - switching all text to sentence-per-line - adding scripts and documentation for regenerating examples - moving --locate-extra-shell-verbs to locate verb - adding details on linked develspace

color: Adding color env variable, fixing catkin build 'skipped' align…

396ab3b

…ment

jbohren added the enhancement label Feb 12, 2016

jbohren added this to the 0.4.0 - Second Beta Announcement milestone Feb 12, 2016

jbohren mentioned this pull request Feb 12, 2016

[major] executor/clean: Adding per-package cleaning, linked develspaces, and a new execution pipeline #196

Closed

25 tasks

clean: Adding support for partial workspace cleaning based on #196

5608ea8

jbohren force-pushed the pre-0.4.0-clean-pkgs branch from 30d54ab to 5608ea8 Compare February 12, 2016 07:14

jbohren added 2 commits February 13, 2016 10:37

list: Clarifying depends and depends-on behavior, adding recursive fu…

12872fd

…nctionality

build: Not forcing CMake when cmake args aren't given

b6c186d

jbohren force-pushed the pre-0.4.0-clean-pkgs branch from f1d9496 to b6c186d Compare February 13, 2016 15:47

tests: adding linked devel test and fixing pep8 stuff

5073cec

This was referenced Feb 13, 2016

Cmake is always forced #288

Closed

list: Clarify --depends-on behavior #212

Closed

jbohren added 5 commits February 18, 2016 16:34

color: Adding color env variable, fixing catkin build 'skipped' align…

007b3d4

…ment

clean: Adding support for partial workspace cleaning based on #196

a149648

list: Clarifying depends and depends-on behavior, adding recursive fu…

0e4619f

…nctionality

build: Not forcing CMake when cmake args aren't given

74bce75

tests: adding linked devel test and fixing pep8 stuff

d29c21b

wjwwood force-pushed the pre-0.4.0-clean-pkgs branch from 5073cec to d29c21b Compare February 19, 2016 00:36

Merge branch 'pre-0.4.0-clean-pkgs' of github.com:catkin/catkin_tools…

0d523ab

… into pre-0.4.0-clean-pkgs

wjwwood reviewed Apr 11, 2016
View reviewed changes

wjwwood added 3 commits April 10, 2016 19:28

remove unused imports

187a004

typo

6ce2f83

add missing import

bf89dd0

wjwwood reviewed Apr 11, 2016
View reviewed changes

jbohren added 6 commits April 12, 2016 17:02

clean: add an additional check before removing directories outside of…

912821a

… the workspace root (keep people from accidentally blowing away their system prefix)

clean: switching to use log function instead of print function

b9ccbac

clean, build: Cleaning up unused variables

043a692

Merge branch 'pre-0.4.0-clean-pkgs' of https://github.com/catkin/catk…

bec0447

…in_tools into pre-0.4.0-clean-pkgs

clean: pep8 cleanup

4be1c69

tests: updating for new clean cli args

f7cf89c

clean: Making cli more consistent

13aa6e5

jbohren force-pushed the pre-0.4.0-clean-pkgs branch from 1509cc3 to 13aa6e5 Compare April 12, 2016 22:43

wjwwood merged commit 406dd32 into master Apr 13, 2016

wjwwood deleted the pre-0.4.0-clean-pkgs branch April 13, 2016 01:11

jbohren added a commit that referenced this pull request Apr 13, 2016

docs: updating after merging #293, regenerating examples and cli inte…

8317023

…rfaces

wjwwood mentioned this pull request Apr 14, 2016

[PR196 *] Adding documentation for forthcoming changes in 0.4.0 #250

Closed

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PR196 3/3] Adding support for partial workspace cleaning #293

[PR196 3/3] Adding support for partial workspace cleaning #293

jbohren commented Feb 12, 2016

wjwwood commented Feb 18, 2016

wjwwood commented Feb 18, 2016

wjwwood commented Feb 18, 2016

wjwwood commented Feb 18, 2016

wjwwood commented Feb 18, 2016

jbohren commented Feb 18, 2016

wjwwood commented Feb 19, 2016

wjwwood commented Feb 19, 2016

wjwwood commented Feb 19, 2016

wjwwood commented Feb 19, 2016

jbohren commented Feb 19, 2016

wjwwood Apr 11, 2016

wjwwood commented Apr 11, 2016

wjwwood Apr 11, 2016

wjwwood commented Apr 11, 2016

jbohren commented Apr 12, 2016

wjwwood commented Apr 12, 2016

jbohren commented Apr 12, 2016

jbohren commented Apr 13, 2016

wjwwood commented Apr 13, 2016

wjwwood commented Apr 13, 2016

[PR196 3/3] Adding support for partial workspace cleaning #293

[PR196 3/3] Adding support for partial workspace cleaning #293

Conversation

jbohren commented Feb 12, 2016

wjwwood commented Feb 18, 2016

wjwwood commented Feb 18, 2016

wjwwood commented Feb 18, 2016

wjwwood commented Feb 18, 2016

wjwwood commented Feb 18, 2016

jbohren commented Feb 18, 2016

wjwwood commented Feb 19, 2016

wjwwood commented Feb 19, 2016

wjwwood commented Feb 19, 2016

wjwwood commented Feb 19, 2016

jbohren commented Feb 19, 2016

wjwwood Apr 11, 2016

Choose a reason for hiding this comment

wjwwood commented Apr 11, 2016

wjwwood Apr 11, 2016

Choose a reason for hiding this comment

wjwwood commented Apr 11, 2016

jbohren commented Apr 12, 2016

wjwwood commented Apr 12, 2016

jbohren commented Apr 12, 2016

jbohren commented Apr 13, 2016

wjwwood commented Apr 13, 2016

wjwwood commented Apr 13, 2016