colcon / colcon-clean

A colcon extension to clean package workspaces
http://colcon.readthedocs.io
Apache License 2.0
44 stars 4 forks source link

Question on the mandatory need for relative path #37

Closed Benjamin-Tan closed 7 months ago

Benjamin-Tan commented 11 months ago

From https://github.com/colcon/colcon-clean/blob/6a4321f24f78ae40889c90a6172a9646b0a9f765/colcon_clean/subverb/__init__.py#L235, it would mean that any paths should be relative to the current working directory. May I ask why is this check on relative path mandatory?

By removing this check on the relative path, it would allow the user to configure their workspace paths (build,log,test,install) accordingly.

ruffsl commented 11 months ago

The line that you have cited merely formats the paths of files to be deleted as relative paths so that the print out to stdout is more often concise when asking for user confirmation, given the base paths are commonly within the root of the workspace path itself.

Is there a runtime error message with using relative or absolute base paths via standard colcon environment variables or user provided CLI arguments?

Benjamin-Tan commented 11 months ago

Yeah from https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.relative_to,

you could try this and it would raise a value error, if any of the path is not relative to the current working directory. For the current behaviour observed, ValueError is not raised, and basically it would skip any path that is not relative to the current working directory.

from pathlib import Path

a = Path("/").relative_to(Path.cwd())
Benjamin-Tan commented 11 months ago

@ruffsl what do you think about the idea of having the relative_to optional? This would allow the base paths that may be configured to be outside of the root of the workspace path itself.

Benjamin-Tan commented 10 months ago

@ruffsl Can I simply raise a PR to comment that line please?

[0.703s] ERROR:colcon:colcon clean: '/home/user/log' is not in the subpath of '/home/user/Documents' OR one path is relative and the other is absolute.
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/colcon_core/command.py", line 522, in verb_main
    rc = context.args.main(context=context)
  File "/usr/local/lib/python3.10/dist-packages/colcon_clean/subverb/workspace.py", line 58, in main
    clean_paths(
  File "/usr/local/lib/python3.10/dist-packages/colcon_clean/subverb/__init__.py", line 235, in clean_paths
    path = path.relative_to(cwd_path)
  File "/usr/lib/python3.10/pathlib.py", line 818, in relative_to
    raise ValueError("{!r} is not in the subpath of {!r}"
ValueError: '/home/user/path' is not in the subpath of '/home/user/Documents' OR one path is relative and the other is absolute.
ruffsl commented 10 months ago

Apologies @Benjamin-Tan , I've been offline for travel for a number of weeks.

From that Value error, I think I have a better idea of what you mean. I think a simple check for subpaths could resolve this.

But to be sure, could you provide a few lines of bash to replicate your example workspace layout, and that's replicate the value error? Similar to what is demonstrated in the readme. I'm guessing you just have to mv the example packages out of the root parent directory and change the colcon src dir arg?

Benjamin-Tan commented 10 months ago

Given your current working directory as /home/user/Documents,

colcon clean workspace --log-base /home/user/log

--log-base can be replaced by --build-base, --install-base, --test-result-base. any of these paths could be configured to be outside of current working directory /home/user/Documents.

ruffsl commented 10 months ago

@Benjamin-Tan , please checkout #41.

Benjamin-Tan commented 7 months ago

@ruffsl Thanks for the fix. When will the next release of this package be available on pypi that would contain this change?

ruffsl commented 6 months ago

@cottsay , should we cut a new minor release of this package?

I think I've forgotten the process to release to PIPY, or perhaps you did this for me previously. We could use the CI to automate this in the future?

https://packaging.python.org/en/latest/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/

cottsay commented 6 months ago

We could use the CI to automate this in the future?

That might release it to PyPI, but then we won't have a deb. Packages in the colcon org should be released with publish-python. If you tag the release in this repo, I'll push it to PyPI and PackageCloud.

ruffsl commented 6 months ago

If you tag the release in this repo, I'll push it to PyPI and PackageCloud.

Done: https://github.com/colcon/colcon-clean/releases/tag/0.2.1