-
Notifications
You must be signed in to change notification settings - Fork 7
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
Packaging fixes #6
Conversation
Signed-off-by: Andrej Shadura <andrew.shadura@collabora.co.uk>
Signed-off-by: Andrej Shadura <andrew.shadura@collabora.co.uk>
Signed-off-by: Andrej Shadura <andrew.shadura@collabora.co.uk>
aiohttp and pymodbus are not in setup.py, but only in requirements.txt, so pybuild cannot detect them. Closes: #1055220 Signed-off-by: Andrej Shadura <andrew.shadura@collabora.co.uk>
${python3:Depends} | ||
${python3:Depends}, | ||
python3-aiohttp, | ||
python3-pymodbus, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it not be better to add to setup.py in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes no practical difference, but adding it here is easier since it requires no patching. I tried convincing dh-python in some other way, but haven’t found an easy one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it would make pdudaemon more consistent at least :) as install_requires is a subset of requiremnts.txt which seems odd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it would make pdudaemon more consistent at least :) as install_requires is a subset of requiremnts.txt which seems odd
Actually, requirements.txt is not used at all by anything but pip (?), and even pip AFAIR requires an explicit path to the requirements file.
But I agree that upstream this should be fixed properly (possibly also by dropping setup.py and replacing it with setup.cfg or pyproject.toml)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah pip is used for the docker file build; Could you submit a small MR to pdudaemon to get setup.py in sync then i can quickly merge it and fix this issue by packaging the new version instead :)
override_dh_instalsystemd: | ||
dh_install_systemd -ppdudaemon --name=pdudaemon --no-restart-on-upgrade share/pdudaemon.service | ||
override_dh_installsystemd: | ||
dh_installsystemd -ppdudaemon --name=pdudaemon --no-restart-on-upgrade pdudaemon.service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the service file is in share/pdudaemon.service
in the source tree right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but dh_installsystemd
needs a name of the unit (already installed in the correct place), not a path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah i see and i didn't notice the snippet name also got changed; tbh if that snippet never ran we may as well drop it entirely. Fwiw for future if you could make your commit message a bit more clear then it saves me for asking :)
${python3:Depends} | ||
${python3:Depends}, | ||
python3-aiohttp, | ||
python3-pymodbus, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it would make pdudaemon more consistent at least :) as install_requires is a subset of requiremnts.txt which seems odd
I've cherry-picked the 2 patches that were relevant. The other 2 patches are fixed in different ways now. So closing this PR, thanks for patches! |
This is mainly to add missing runtime dependencies, see #1055220.
aiohttp and pymodbus are not in setup.py, but only in requirements.txt, so pybuild cannot detect them.
I’m not sure the systemd override is necessary, but I have fixed it nevertheless.