galaxyproject / galaxyproject/planemo

Global config values bypass click.Path conversion (resolve_path is dead code)

Open
#1,667 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
110
Forks
102
Avg merge
4d 21h
Merged PRs (30d)
13

Description

> _Posted by Claude (AI assistant) on behalf of @jmchilton — they did not author this text personally._

Values that reach a `click.Path` option from `~/.planemo.yml` skip click's type conversion entirely, so `resolve_path=True` (and `exists`, `dir_okay`, `writable`, ...) never apply to them. The same option given on the command line or via an env var is converted normally.

## Cause

`planemo_option` forces `kwargs["default"] = None` and installs its own callback (`planemo/config.py:116-117`). Click type-casts a value *before* calling the callback, so only values click itself sourced get converted:

| value source | `click.Path` conversion applied? |
| --- | --- |
| command line | yes |
| env var (`use_env_var`) | yes |
| `~/.planemo.yml` (`default_`) | **no** |
| declared `default=` in `planemo_option` | **no** |
| profile (`planemo/cli.py:157-164`) | **no** |

`planemo/config.py:101` computes a `resolve_path` flag from the option's type and passes it to `_default_callback`, which accepts the argument and never uses it.

## Regression, not a design choice

The abspath call existed from e7691184 (2016) until 67d32183 (2022-11-22, "Implement --docker_extra_volume for --docker/--enable biocontainers"), which deleted these two lines:

```python
if resolve_path and result is not None:
result = os.path.abspath(result)
```

The same commit added `multiple=True` to `--docker_extra_volume`, i.e. the call was removed because `os.path.abspath()` cannot take a tuple. The dead `resolve_path` parameter is what was left behind.

## Reproduction

With `default_file_path: myfiles` in `~/.planemo.yml`, parsing `planemo serve` from `/tmp/workdir`:

```
[serve] --file_path from global config (relative 'myfiles'):
file_path = 'myfiles'
[serve] --file_path passed on the command line:
file_path = '/tmp/workdir/myfiles'
[run] --cwltool_cache_directory from global config (relative 'mycache'):
cwltool_cache_directory = 'mycache'
[run] --cwltool_cache_directory via env var:
cwltool_cache_directory = '/tmp/workdir/mycache'
```

## Affected options

`use_global_config`/`extra_global_config_vars` combined with `click.Path(..., resolve_path=True)`, in `planemo/options.py`:

`--galaxy_root` (199), `--cwl_galaxy_root` (231), `--dependency_resolvers_config_file` (272), `--cwltool_cache_directory` (335), `--file_path` (430), `--shed_data_dir` (491), `--tool_dependency_dir` (505), `--job_config_file` (515), `--tool_data_path` (554), `--docker_extra_volume` (609), the `--test_output*` family (1734-1776), `--test_output_json` (1919).

Impact is uneven:

- **Galaxy config paths** (`--file_path`, `--tool_dependency_dir`, `--tool_data_path`, `--job_config_file`, `--shed_data_dir`, `--dependency_resolvers_config_file`) are written verbatim into the generated `galaxy.yml`, and Galaxy is launched with `cd ` (`planemo/galaxy/config.py:1201`). Planemo meanwhile does `_ensure_directory` on the same relative path in the invocation cwd, so planemo and Galaxy silently disagree about where the directory is.
- **`--docker_extra_volume`** goes straight into `docker run -v` (`planemo/galaxy/config.py:295-297`). Docker treats a non-absolute source as a *named volume* rather than a bind mount. Separately, a scalar YAML value here is a `str`, so `volumes.extend(...)` iterates it character by character.
- **`--galaxy_root` / `--cwl_galaxy_root`** are `exists=True`; from the global config the existence check is skipped too, so a typo fails later with a confusing error instead of at parse time.
- **`--cwltool_cache_directory`** and the `--test_output*` family are harmless in practice - they are used from the invocation cwd.

## Suggested fix

Delegate to click rather than re-implementing, which also restores the skipped validation and handles `multiple`/`nargs` (the thing that broke in 2022):

```python
if option_source is not OptionSource.cli and result is not None:
if param.multiple and isinstance(result, str):
result = (result,)
result = param.type_cast_value(ctx, result)
```

The `param.multiple` guard is required or a scalar YAML value for `--docker_extra_volume` is iterated per character. Note `click.Path(resolve_path=True)` uses `os.path.realpath`, so the 2016 code was already subtly divergent from the CLI path; `type_cast_value` makes the two genuinely identical.

Unrelated but adjacent: `dependencies_script_options()` (`planemo/options.py:1961`), which owns `--download_cache`, is referenced nowhere in the repo and could be deleted.

Contributor guide

Open the contributing guide

Research direction

Start in planemo/config.py around lines 101 and 116-117, then trace the affected click.Path options in planemo/options.py and profile handling in planemo/cli.py:157-164. Reproduce the relative-path cases from the issue for global config, CLI, and environment values. Done means all sources receive equivalent conversion and validation, while scalar docker_extra_volume values remain safe for multiple options.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
cli, tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.