Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use entrypoint to generate mkosi instead of symlink #401

Merged
merged 2 commits into from
Aug 8, 2020

Conversation

behrmann
Copy link
Contributor

@behrmann behrmann commented Feb 18, 2020

First, sorry that the diff is unnecesarily big, this looks like a lot more than it actually should, all the relevant changes are in setup.py.

This PR renames mkosi to mkosi.py and generates a mkosi script via the console_script entry point mechanism provided by setuptools. The script looks like this on my machine

#!/usr/bin/python
# -*- coding: utf-8 -*-
import re
import sys
from mkosi import main
if __name__ == '__main__':
    sys.argv[0] = re.sub(r'(-script\.pyw|\.exe)?$', '', sys.argv[0])
    sys.exit(main())

and is installed to ~/.local/bin by pip.

Besides auto-generating this script, this also makes mkosi importable as a module and allows calling it via python3 -m mkosi. The former made it a bit easier for me to play around with stuff in the REPL without copy-pasting and the latter in principle allows multiple concurrent installations, which might be good for testing in the future. The shebang is adjusted to refer to the python interpreter running pip during the installation.

As a side I also changed the Python version check to use setuptools, as even Debian oldstable has a sufficiently modern pip and setuptools.

Edit (01.08.2020): The current version of this PR removes the mkosi.py symlink and makes mkosi a directory, with most of everything moved to mkosi/__init__.py except for main() which is moved to mkosi/__main__.py

@lgtm-com
Copy link

lgtm-com bot commented Feb 18, 2020

This pull request introduces 1 alert when merging 71b55a1 into 3a3a21e - view on LGTM.com

new alerts:

  • 1 for Unused import

@behrmann behrmann force-pushed the entrypoint branch 2 times, most recently from bde543b to d5c3182 Compare February 18, 2020 16:47
@behrmann
Copy link
Contributor Author

This pull request introduces 1 alert when merging 71b55a1 into 3a3a21e - view on LGTM.com

new alerts:

* 1 for Unused import

This has been fixed.

@lucasdemarchi
Copy link
Contributor

Not sure I like it. @poettering ?

@behrmann
Copy link
Contributor Author

What I mostly dislike is the gigantic diffstat, but this could for example also be used to start using black, a somehwat divisive (due to its style minimising diffs) code formatter, that has nevertheless become quite widely used in the Python world. This would for example solve the issue, that currently the mkosi file is indented with both tabs and spaces, which I have stumbled over just now.

@behrmann behrmann force-pushed the entrypoint branch 2 times, most recently from 8c40065 to 3352e96 Compare July 31, 2020 14:47
@behrmann
Copy link
Contributor Author

I tried to make this more palatable, as this now doesn't break git blame. Being a directory-based module now also gets rid of of the loader shenanigans needed to make pytest work.

@behrmann behrmann force-pushed the entrypoint branch 5 times, most recently from 38d0827 to f9bc794 Compare August 1, 2020 16:00
@behrmann
Copy link
Contributor Author

behrmann commented Aug 1, 2020

@poettering, @keszybz: I've grepped the systemd tree for direct calls to mkosi but didn't find anything that would need patching for this change, but as systemd is the primary user of mkosi, is there a way this would hurt you?

You would probably need to reinstall mkosi, preferably via pip's editable install

python3 -m pip install -e /path/to/mkosi/checkout

so as to always pick up the changes in your checkout of mkosi, but besides that the change would should be seemless and would be the first step to splitting up mkosi into smaller pieces, e.g. factoring out the distro specific stuff into submodules (as described in #483).

@behrmann behrmann force-pushed the entrypoint branch 3 times, most recently from 3cf93d1 to fb49fb9 Compare August 3, 2020 17:29
@behrmann
Copy link
Contributor Author

behrmann commented Aug 3, 2020

Just noticed that the resulting __init__.py still had the executable bit set, which is no longer necessary.

@behrmann behrmann force-pushed the entrypoint branch 3 times, most recently from 75ff6f1 to 38041a7 Compare August 7, 2020 17:23
@DaanDeMeyer
Copy link
Contributor

DaanDeMeyer commented Aug 8, 2020

With the gigantic diffstat gone this is way better. Can you make sure running ./mkosi from the repository keeps working? This is only anecdotal but I'm running mkosi like this directly from the git checkout so there might be others doing the same. If it's not too much complexity I think we should try to make sure that keeps working.

EDIT: Actually, the editable install thing is really cool. Could you add a quick section to the readme explaining how to get that working (run pip --user and probably make sure PATH includes .local/bin I guess?), then I think we don't need the mkosi script anymore.

EDIT2: Actually, maybe don't put it in the readme, just put it in ./mkosi in the repository itself. That way if anyone tries to use the old way, they immediately get the instructions on how to move to the new way.

P.S: Sorry for the late reply, I didn't get any notifications for the updates as I've never posted in this PR.

@DaanDeMeyer
Copy link
Contributor

What's the best way to uninstall an editable install? I tried pip3 uninstall mkosi but it only removed the egg-link and not ~/.local/bin/mkosi.

@behrmann
Copy link
Contributor Author

behrmann commented Aug 8, 2020

EDIT: Actually, the editable install thing is really cool. Could you add a quick section to the readme explaining how to get that working (run pip --user and probably make sure PATH includes .local/bin I guess?), then I think we don't need the mkosi script anymore.

EDIT2: Actually, maybe don't put it in the readme, just put it in ./mkosi in the repository itself. That way if anyone tries to use the old way, they immediately get the instructions on how to move to the new way.

I added it to the README, since ./mkosi is a directory in this branch, so that breaks that unfortunately.

P.S: Sorry for the late reply, I didn't get any notifications for the updates as I've never posted in this PR.

No worries. Github notifications have edge cases, like being silent on edits.

What's the best way to uninstall an editable install? I tried pip3 uninstall mkosi but it only removed the egg-link and not ~/.local/bin/mkosi.

Interesting. This is surely a bug in pip. Uninstalling leaves ~/.local/bin/mkosi around, but when doing the editable install inside a virtual environment the uninstall does remove venvroot/bin/mkosi properly. I'll have a look whether there is a workaround.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@behrmann
Copy link
Contributor Author

behrmann commented Aug 8, 2020

@DaanDeMeyer I reworded the sections in the README a bit, can you have a look whether that's sufficiently clear?

I have found out, that the pip's behaviour might be intentional, as they've had (?) a bug where they overeagerly removed files, that pip had not created (someone had named their console script "find") and pip doesn't keep track of which files it installs. That would explain why it works in a venv, since that bin/ is fully under pip's control.

There might a way around this with PEP517 (that introduced pyproject.toml), since pip supposedly keeps track of the files it installs since then. I'll have a look.

@behrmann
Copy link
Contributor Author

behrmann commented Aug 8, 2020

Ok, currently this seems to be a catch 22. Pip doesn't clean up the generated scripts from entrypoints, because it doesn't track them properly and the solution of PEP517 is incompatible with editable installs and/or entrypoints (the last hour has been a world of Permission errors).

The easiest solution for now would be to just document this, but I'll try to figure out if this works somehow.

@DaanDeMeyer
Copy link
Contributor

DaanDeMeyer commented Aug 8, 2020

From what I could find in issues, this shouldn't be broken when using console_scripts but that's what we're already using. (I hate it when basic stuff like this doesn't work correctly although to be fair, until recently a ton of basic stuff in mkosi didn't work as well, maybe that's still the case although I'd hope not!)

@DaanDeMeyer
Copy link
Contributor

DaanDeMeyer commented Aug 8, 2020

I added a comment with reproducer to pypa/pip#5997 (please don't delete the branch when we merge this as it's part of the reproducer)

@behrmann
Copy link
Contributor Author

behrmann commented Aug 8, 2020

The most straight-forward way I see, is to document this. Hopefully people won't want to remove mkosi that often :)

(I hate it when basic stuff like this doesn't work correctly although to be fair, until recently a ton of basic stuff in mkosi didn't work as well, maybe that's still the case although I'd hope not!)

While fiddling around with this, I also found that building the man page produces garbled output with a recent pandoc. Since I wanted to see whether that works if throwing it under the bus when moving to poetry as a build tool would help (which it doesn't).

@DaanDeMeyer
Copy link
Contributor

The uninstall bug is not something we can fix here so let's merge this.

@DaanDeMeyer DaanDeMeyer merged commit 78e3c3e into systemd:master Aug 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants