JOJ0 / synadm

Command line admin tool for Synapse (the Matrix reference homeserver)
https://synadm.readthedocs.io
GNU General Public License v3.0
186 stars 25 forks source link

Allow to run the module without installing it #138

Closed n-peugnet closed 9 months ago

n-peugnet commented 11 months ago

As explained previously, I want to add the click generated bash completion script to the Debian package. Here is the change I sent to the Debian maintainer of synadm : https://salsa.debian.org/matrix-team/synadm/-/merge_requests/1

The only way I found to make it work was to add a "main" file that allows to run synadm without installing it. As I thought it could be useful outside the Debian package, here is the forwarded patch, that allows to run synadm directly with python -m synadm.

I am no expert in Python so this might not be the ideal way to do it.

JOJ0 commented 11 months ago

but that would require it to be installed already...

the debian package requires it to be able to run without installing? why? during packaging?

n-peugnet commented 11 months ago

the debian package requires it to be able to run without installing? why? during packaging?

Not the Debian package directly, but the way Click based CLI's shell completion work is by calling the program with a specific environment variable set to generate the completion script.

I make this call in Line 4 of debian/rules, and I didn't find any better way to make it than by making the package executable with python -m. I also tried pybuild's AFTER_INSTALL hook but didn't manage to make it work without this change either.

But if you don't want this change, it can also stay a Debian only patch, as it is very small and should not change too often. And if you have better suggestions I am also interested.

JOJ0 commented 10 months ago

Hmm interesting, not possible for me to try out myself atm but wouldn't that work already:

python -m synadm.cli

Nope that doesn't work :-)

$ python -m synadm.cli -h
/home/jojo/.venvs/synadm310/bin/python: No module named synadm.cli.__main__; 'synadm.cli' is a package and cannot be directly executed
JOJ0 commented 10 months ago

the debian package requires it to be able to run without installing? why? during packaging?

Not the Debian package directly, but the way Click based CLI's shell completion work is by calling the program with a specific environment variable set to generate the completion script.

I make this call in Line 4 of debian/rules, and I didn't find any better way to make it than by making the package executable with python -m. I also tried pybuild's AFTER_INSTALL hook but didn't manage to make it work without this change either.

But if you don't want this change, it can also stay a Debian only patch, as it is very small and should not change too often. And if you have better suggestions I am also interested.

Hi @n-peugnet, I'm not opposed to this change at all. Please let's add it. Some things I'd like to add within this PR though:

Some docstrings in the new module __main__ describing why it is here. You could even state the initial reasons: Running without installing, Debian packaging

Also I would like to add to the docs, the new possibility to run synadm programatically, it might be of interest to other packagers or to people who'd like to integrate synadm into their own programs. Not sure where in the docs I would see that located though...

And also, but this may be off-topic here, but I would like to include it in the PR while we are at it and we have the expert at hand already (you ;-), let's add to the docs what commands and setup would be required to make synadm bash completion work when synadm is installed via the pip package. Maybe have the concrete commands for bash and zsh right in our docs but as well linking to the Click docs: https://click.palletsprojects.com/en/8.1.x/shell-completion/

If you find time to help us with these things within this PR that would be wonderful. Certainly not urgent since we are not fixing a bug here but at some point this or next year?

JOJ0 commented 10 months ago

I realized where in the docs the main module could be included. Since it is very much only interesting for developers, it would fit as a new page in the "package documentation" section. Creating a file named doc/source/synadm.module.main.rst and including this should do the trick:

Synadm Main Package
==================

.. automodule:: synadm.__main__
   :members: root
   :undoc-members:
   :show-inheritance:
   :private-members:
   :member-order: bysource

The docstring you add to the source would then automatically get rendered within our docs.

Finally adding a line

synadm.module.main to the index file below here:

https://github.com/JOJ0/synadm/blob/ea1b1e0b19260c34177aba241747885a8b55364b/doc/source/index_modules.rst?plain=1#L8-L9

and it should be accessible in the TOC.

You can build the docs locally (but I see that a howto for that is missing in our docs, I should add that) or tell me to check out your branch and I'll test it.

JOJ0 commented 10 months ago

update again: @n-peugnet I'm not 100% sure about my previous post's description https://github.com/JOJ0/synadm/pull/138#issuecomment-1864132521 anymore. Maybe forget about it for now and just add the docstring in __main.py__ and a description about how to set up bash/zsh completion. The latter you could even just add as a comment here, and I'll handle the adding to docs later in master or a new PR myself.

JOJ0 commented 9 months ago

Hi @n-peugnet, I just wanted to the following commit to this PR to finalize it: https://github.com/JOJ0/synadm/commit/4e41cd51ae8191ccf84ee07bddbb1dd899eb386b

It seems I'm not allowed to push that commit to your branch. Would it be possible for you to allow "maintainer changes" on your end? https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

What I did is adding you as the author of the file and I elaborated on why this file is here. Please review and tell me if you would agree with these changes.

Thanks!

n-peugnet commented 9 months ago

Hi @n-peugnet, I just wanted to the following commit to this PR to finalize it: 4e41cd5

[...]

What I did is adding you as the author of the file and I elaborated on why this file is here. Please review and tell me if you would agree with these changes.

This is perfect ! Sorry, I didn't answer you previous message, thank you for doing this yourself.

It seems I'm not allowed to push that commit to your branch. Would it be possible for you to allow "maintainer changes" on your end? https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

Well, this is strange, I don't have this checkbox on this pull request page. It might be because I forked the repo in an organisation. I think I would need to create a new pull request and close this one. But if you are OK to simply merge https://github.com/JOJ0/synadm/commit/4e41cd51ae8191ccf84ee07bddbb1dd899eb386b into main, this is fine by me.

n-peugnet commented 9 months ago

I figured I could simply pull your commit in my branch.

JOJ0 commented 9 months ago

Yeah a cherry-pick would do it. Thanks!

n-peugnet commented 9 months ago

Yeah a cherry-pick would do it. Thanks!

Already done, in case you didn't see

JOJ0 commented 9 months ago

Ah yeah missed it. Many thanks!