-
Notifications
You must be signed in to change notification settings - Fork 53
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
Restore bash completions #183
Conversation
Please see my comment here: maybe it is fine to have an extra step for the bash completions stuff when installed from pip or similar. Because from what I understand here, the files will be generated, but very likely not used when installed in this location, no? In this case, maybe no need to generate them, but suggest (in the There is also the issue with the config file: can it not be installed in a "shared" directory? e.g. the ones suggested by: https://pypi.org/project/platformdirs/ |
I'm trying to keep this as close to the original logic as possible.
I'm not going to change that here. Sounds unrelated to the breakage this is seeking to fix. |
The point of installing bash-completion and the config file is to install them in the location where they are expected. If they are installed somewhere 'private' that dos not address the problem caused by 'buildsystem modernization'. |
This is addressed in the PR description:
I don't know of a path toward hermetic builds that doesn't require coordination with distro packaging. If you do, please let us know. |
I'm getting this if I try to build virtme-ng with this change on Arch. |
How are you building it? |
Versions of python, pip, would be helpful to know as well. |
|
0b82767
to
f23966a
Compare
|
f23966a
to
907bb11
Compare
I see. This should be fixed now.
The solution was to use argparse as a library instead of a binary; I guess pip doesn't install binstubs during setup.py. |
484deaf
to
ec96058
Compare
Thinking about this more I think we can possibly restore the |
[ 2s] ----------------------------------------------------------------- |
05f9481 removed these as the shortest path to preserving hermeticity of the build, but this solution was overly aggressive. Restore the bash completions to their previous locations. This should place them in system-global locations when built with `--break-system-packages` while otherwise preserving hermiticity. In a hermetic build these files end up in the venv's site-packages: ``` tamird@Tamirs-MacBook-Pro src % python3 -m venv virtme-venv tamird@Tamirs-MacBook-Pro src % source virtme-venv/bin/activate (virtme-venv) tamird@Tamirs-MacBook-Pro src % cd virtme-ng (virtme-venv) tamird@Tamirs-MacBook-Pro virtme-ng % VNG_PACKAGE=1 BUILD_VIRTME_NG_INIT=1 pip install . Processing /Users/tamird/src/virtme-ng [snip] (virtme-venv) tamird@Tamirs-MacBook-Pro virtme-ng % cd ../virtme-venv (virtme-venv) tamird@Tamirs-MacBook-Pro virtme-venv % ls lib/python3.13/site-packages/{etc,usr/share} lib/python3.13/site-packages/etc: total 8 drwxr-xr-x 3 tamird staff 96B Oct 17 21:01 . drwxr-xr-x 26 tamird staff 832B Oct 17 21:01 .. -rw-r--r-- 1 tamird staff 33B Oct 17 21:01 virtme-ng.conf lib/python3.13/site-packages/usr/share: total 0 drwxr-xr-x 4 tamird staff 128B Oct 17 21:01 . drwxr-xr-x 3 tamird staff 96B Oct 17 21:01 .. drwxr-xr-x 3 tamird staff 96B Oct 17 21:01 bash-completion drwxr-xr-x 3 tamird staff 96B Oct 17 21:01 man ```
@hramrach thanks. should be fixed now. |
ec96058
to
0cf73c3
Compare
@tamird this one seems to work also for me, good job! I'd still prefer to tag a new release without this PR, then merge this and then see if we need to fix other cases. |
Sounds good, thanks. |
I tried to add back the detection if argparse-manpage is available hramrach@96f7e40 but now its unavailability also breaks bash completion: [ 1s] ----------------------------------------------------------------- |
Why do you need detection? |
Your detection logic is incorrect, you've skipped the entire BuildPy implementation, so you're also not building virtme-ng-init nor generating completion scripts. |
Why is get_build_py_cmd imported from build_manpages? Sure, stuff that should be built isn't because of that when build_manpages is unavailable, that's the problem. |
Alright v1.31 is out, so let's merge this and fix other potential breakage in tree. |
Can the argparse-manpage still work with this build system? |
detection |
Addressed in #186 I suppose it might make sense to release this as well to get wider testing and see if there are any remaining issues. |
05f9481 removed these as the shortest
path to preserving hermeticity of the build, but this solution was
overly aggressive. Restore the bash completions (and the config file) by
placing them in directories inside the package (rather than
system-wide).
Downstream distro packaging can move place the content of ./etc/ in
/etc/ and ./share/ in /usr/share/.
See #181.