Expand the ability to specify parameters by is_base or not - #334
Expand the ability to specify parameters by is_base or not#334hmaarrfk wants to merge 4 commits into
Conversation
9514862 to
b36bf87
Compare
|
Hm, I'm not sure about this. The |
|
Your suggestion would work, and it isn’t a bad one. I might just do it. The problem is that any application will have duplicate their menuinst packages. That’s fine for me. But will generate future annoyance for users. I’ll try and see how it looks. Thanks again for the reviews! |
|
In the future, |
|
But in my use case, my main app is « my application » and not conda. I want my base to be protected too ;) this is less for miniforge, more for customized use cases. |
|
Fair enough. Also, I think you could accomplish the same with a single package, but two build variants, with some |
|
I've held off on commenting, but 1 year later i find this inconsistency to still be thorn in our setup process.
is pretty complicated. variants and keeping them in sync is a chore. Would you reconsider these additions? |
|
The #477 may provide a way out though: I made a suggestion to enable the I would also move the bash details into a separate PR because it's a separate concern. |
can i circle back to this after #477 is merged? it seems like a lot of changes, so i'm hopeful I'll be able to use it. |
|
Hi @hmaarrfk could you provide more with more context to your use-cases? |
Lets consider installing Spyder. And I know that Spyder can be installed in one environment, then USE the python of an other, but I've never found that to be hyper reliable. There is one usecase where you want to install spyder in 2 environments that are NOT base:
In this case, you want Spyder to show up on linux as In the case that you now wnat to ship Spyder, as a standalone application
You don't want That is the main usecase and the logic I want. I want to design different names for my application when it is installed in base (it can assume it is a standalone application), and when it is in an environment (where the There are many "customizations" accross most of the options in menuinst that need to happen. I would prefer to not have 2 packages to install. |
|
Thanks for working to learn more about my usecase! |
|
Hi @hmaarrfk thanks for sharing this. If the primary use case is just the name (Spyder vs Spyder (env)) I'd say that adding built-in placeholders would be the easiest. I.e.:
Other ways forward could be:
|
|
Unfortunately for me. A partial solution just isn’t enough. I might take Jaime’s suggestion and just deal with two helper package names. This is really not ideal since I feel like what I’m asking for is a natural extension of the schema that is already here. |
|
one year later, i just feel like this is a reasonable request. you may disagree, but please take this as empowering a user of yours to avoid making a fork for something that the original creators of Anaconda/conda felt was a gap too:
Feel free to close this at this point, but I think this is a minor expansion of the scope, that has the capabilities to really help maintain one of your users. |
b36bf87 to
a92e275
Compare
<details><summary>Claude's draft</summary> Refresh this staging branch onto the current feedstock main (menuinst 2.5.2, rattler-build v1 recipe) and carry all four of my open conda/menuinst pull requests as source patches: - 333.patch — conda/menuinst#333 Expand placeholders in lists of strings, not just scalars. - 334.patch — conda/menuinst#334 Allow name/command/StartupWMClass/TryExec to be specified per target_environment_is_base, and add the linux `run_in_bash` key. Rebased onto upstream main: `min_items` -> `min_length` for pydantic v2, `Optional[A, B]` -> `Optional[Union[A, B]]`, and SCHEMA_VERSION bumped to 1-1-4 with the generated schema/default JSON regenerated so `run_in_bash` has a default to resolve against. - 535.patch — conda/menuinst#535 Restore the missing f-string prefix on the Icon= desktop entry line. - 536.patch — conda/menuinst#536 Pass the opened file or URL to Linux shortcuts via an Exec field code. 334 and 536 both rewrite LinuxMenuItem._command; 536.patch carries the merged form, which keeps the field-code "$@" trick for the bash-wrapped case and appends the field code directly when run_in_bash is false. Build number bumped by 100 and the build restricted to linux-64, since this is a staging branch for my own channel. Resume this Claude session: ``` cd /home/mark/git/feedstock/menuinst-feedstock claude --resume b888d0ac-7cec-4e1e-ac68-9c15e6554011 ``` </details> Claude-Session: https://claude.ai/code/session_0135Eijr6BTzjVHcRcn3JP8w
<details><summary>Claude's draft</summary> Refresh this staging branch onto the current feedstock main (menuinst 2.5.2, rattler-build v1 recipe) and carry all four of my open conda/menuinst pull requests as source patches: - 333.patch — conda/menuinst#333 Expand placeholders in lists of strings, not just scalars. - 334.patch — conda/menuinst#334 Allow name/command/StartupWMClass/TryExec to be specified per target_environment_is_base, and add the linux `run_in_bash` key. Rebased onto upstream main: `min_items` -> `min_length` for pydantic v2, `Optional[A, B]` -> `Optional[Union[A, B]]`, and SCHEMA_VERSION bumped to 1-1-4 with the generated schema/default JSON regenerated so `run_in_bash` has a default to resolve against. - 535.patch — conda/menuinst#535 Restore the missing f-string prefix on the Icon= desktop entry line. - 536.patch — conda/menuinst#536 Pass the opened file or URL to Linux shortcuts via an Exec field code. 334 and 536 both rewrite LinuxMenuItem._command; 536.patch carries the merged form, which keeps the field-code "$@" trick for the bash-wrapped case and appends the field code directly when run_in_bash is false. Build number bumped by 100 so this can be uploaded to my own channel. All platforms stay enabled. Resume this Claude session: ``` cd /home/mark/git/feedstock/menuinst-feedstock claude --resume b888d0ac-7cec-4e1e-ac68-9c15e6554011 ``` </details> Claude-Session: https://claude.ai/code/session_0135Eijr6BTzjVHcRcn3JP8w
<details><summary>Claude's draft</summary> Rebased onto the noarch-except-windows branch (conda-forge#67) so this staging build produces one noarch package plus the Windows builds. Carry all four of my open conda/menuinst pull requests as source patches: - 333.patch — conda/menuinst#333 Expand placeholders in lists of strings, not just scalars. - 334.patch — conda/menuinst#334 Allow name/command/StartupWMClass/TryExec to be specified per target_environment_is_base, and add the linux `run_in_bash` key. Rebased onto upstream main: `min_items` -> `min_length` for pydantic v2, `Optional[A, B]` -> `Optional[Union[A, B]]`, and SCHEMA_VERSION bumped to 1-1-4 with the generated schema/default JSON regenerated so `run_in_bash` has a default to resolve against. - 535.patch — conda/menuinst#535 Restore the missing f-string prefix on the Icon= desktop entry line. - 536.patch — conda/menuinst#536 Pass the opened file or URL to Linux shortcuts via an Exec field code. 334 and 536 both rewrite LinuxMenuItem._command; 536.patch carries the merged form, which keeps the field-code "$@" trick for the bash-wrapped case and appends the field code directly when run_in_bash is false. Build number bumped by 100 over the base so this can be uploaded to my own channel. Resume this Claude session: ``` cd /home/mark/git/feedstock/menuinst-feedstock claude --resume b888d0ac-7cec-4e1e-ac68-9c15e6554011 ``` </details> Claude-Session: https://claude.ai/code/session_0135Eijr6BTzjVHcRcn3JP8w
|
and i'll put conda-forge/menuinst-feedstock#67 up as a token of me being a good "steward" of open source. |
| """ | ||
| ), | ||
| ) | ||
| run_in_bash: Optional[bool] = Field( |
There was a problem hiding this comment.
This one feels like it belongs in a different PR.
jaimergp
left a comment
There was a problem hiding this comment.
What is the criteria for adding the base-or-not dict to some fields but not others, simple necessity as of now? What's the update story for the inevitable moment when someone wants to add them to more places? Will we have more variables in this hardcoded conditional at some point?
If we accept to do this, I see ourselves going into the direction of generalised recipe.yaml-style if: / then: dictionaries, with some predefined runtime variables, and then accept those anywhere not just in a perhaps-arbitrary fashion.
That aside, we'll need to:
- Add tests and examples
- Update documentation
- Add news
37ca451 to
e4041bd
Compare
e4041bd to
e28a5a1
Compare
necessity is pretty close to my decision making process. Do note that this whole "I install things in my own conda environment inside a whole operating system) is somewhat new. These things also end up being somewhat "public" at the interface between different subsystems that try to help with identification (by humans or other code)
So if something doesn't end in
Understood. I don't see this as a problem. We have all been working toward better system integration. I see the present solution as a step in enabling that.
I will ping again when this is ready. |
7e36725 to
1fe5291
Compare
Agreed, but I'd like to avoid the intermediate steps so we don't have to support too many different schemas, although this particular one may not be a big burden. |
<details><summary>Claude's draft</summary> Three unrelated breakages were making every CI job red: * `SCHEMA_VERSION` said `1-1-4` in `menuinst/_schema.py` and `menuinst/platforms/base.py`, but the generated data files were named `menuinst-1-2-0.*` — a stale-rebase artifact. Importing `menuinst` at all raised `FileNotFoundError` on the missing `menuinst-1-1-4.default.json`, which took down the test suite on every platform (conftest import error) and the docs build (autodoc import error). Settled on `1-2-0` as the new schema version and regenerated both data files against it. Widening `command` from an array to array-or-object keeps every existing document valid, but it can break consumers that assume `command` is a list, so this is a SchemaVer REVISION rather than an ADDITION. * `Remove run_in_bash` (e28a5a1) deleted the option but kept the `run_in_bash: false` branch, so `LinuxMenuItem._command` stopped wrapping the command in `bash -c`. That is a behavior change for every Linux shortcut, not a revert. Restored the `main` behavior, which also makes the now-unused `shlex` import used again and unbreaks the ruff pre-commit hook. * `docs/source/reference.md` still pointed `autopydantic_model` at `menuinst._schema.MenuItemNameDict`, renamed on this branch. Docs are built with `-W`, so the dangling reference was fatal. Pointed it at `TargetIsBaseConStr` and added `TargetIsBaseConList`. Verified locally: `pytest tests/` (only the two pre-existing macOS-local failures, `test_osx_symlinks` and `test_elevation`, which fail on `main` too), `pre-commit run --all-files`, and `make dirhtml SPHINXOPTS="-W --keep-going -n"`. Resume this Claude session: ``` cd /Users/mark/git/menuinst claude --resume 08aef327-5520-4aa8-a267-4c3f0597d871 ``` </details>
<details><summary>Claude's draft</summary> Addresses the three follow-up bullets from review 5017801096. **Tests and examples** * New example `tests/data/jsons/target-environment.json` exercising every field that accepts the base-or-not form at once, with distinguishable branches. It is picked up automatically by `test_examples`, so the schema itself is covered. * `test_target_environment_dictionary` checks `name`, `command` and `precommand` resolve to the right branch on all platforms, including the case where a `platforms.win` block overrides a top-level value with another base-or-not dictionary. * `test_target_environment_desktop_entry` (Linux) checks `StartupWMClass`/`TryExec`/`Name`/`Exec` in the written `.desktop` file, and that placeholders are expanded *after* the branch is picked. * `test_target_environment_missing_branch` covers the error path. * `test_target_environment_leaves_other_dicts_alone` pins the resolver to mappings actually shaped like a branch pair, so `glob_patterns` and friends pass through untouched. **Code needed to make those tests pass** The schema declared `command` as base-or-not for every platform, but only `LinuxMenuItem` knew how to resolve it: a dictionary `command` on macOS or Windows reached the argument quoting as a `dict`. Conversely `precommand` was resolved in `linux.py` but not declared in the schema, so it could never validate. Both are fixed by resolving the branch once, in `MenuItem.__init__`, where `name` was already being handled: `_resolve_target_environment_keys` walks the flattened metadata and collapses any value shaped like a branch pair. The three ad-hoc branches in `linux.py` go away, and `precommand` is declared as accepting `TargetIsBaseConStr`. Adding a field to the set is now a schema-only change. **Documentation** `defining-shortcuts.md` grows a `(base-or-not)` section that states the rule once, tables the fields that accept it, and adds a `command` / `TryExec` example next to the existing `name` one. Field descriptions in `_schema.py` now say which fields accept the form (these are what the reference page renders), and the `TargetIsBaseConStr`/`TargetIsBaseConList` docstrings describe the models rather than repeating "Name when ...". **News** `news/334-target-environment-dicts`. Verified: `pytest tests/` on macOS and in a `condaforge/miniforge3` container (only the failures that also fail on `main` in each environment remain), `pre-commit run --all-files`, and `make dirhtml SPHINXOPTS="-W --keep-going -n"`. Not addressed here: the reviewer's opening question about the criteria for the field set and the eventual `if:/then:` generalisation. The docs now state the set is deliberately fixed and point at an issue, but that is a design decision for the maintainers, not something to settle in this PR. Resume this Claude session: ``` cd /Users/mark/git/menuinst claude --resume 08aef327-5520-4aa8-a267-4c3f0597d871 ``` </details>
1fe5291 to
3e6708f
Compare
In my workflow, I change a bit more than just the name depending on the environment being base or not.
This expands the functionality of #180
xref: #207
This provides those switches.
It also provides a way to "not run inside a bash shell"
Let me know if you are interested in this, I need to go through the checklist.
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?