Initial Setup of Package#1
Conversation
johnzhou721
left a comment
There was a problem hiding this comment.
Some notes on this package.
|
@freakboy3742 I apologize if you simply haven't gotten to this yet, but pinging in case you've missed this (since this is important and it's been 4 days). |
|
Not missed - just on a long list of stuff to review; with the added complication that I need to set up some new test VMs to be able to evaluate this. Hoping to get a chance to look at this today. |
freakboy3742
left a comment
There was a problem hiding this comment.
The package itself seems to work well - although not in editable mode. That's a PITA, but I don't know that there's many options there, given that already in niche usage territory. It would be worth flagging in the README as something that anyone wanting to work on the package might want to know.
A few other issues flagged inline - mostly related to testing.
|
|
||
| # Below, locations are hardcoded for common distros' layout, and those | ||
| # directories for package and distribution info are searched instead of | ||
| # the entire system site packages as defined above. |
There was a problem hiding this comment.
Some notes here:
- Why distinguish between PACKAGE_DIR and DIST_INFO_DIR at all? They're all locations where paths can be found.
- Why hard code disto-specific paths when
site.getsitepackages()gives us an authoritative list for the actual python?
There was a problem hiding this comment.
@freakboy3742 For 2., site.getsitepackages() seems to only work when the pth file is first execued... appears that the pth file is executed twice, second time with site.getsitepackages() being the venv-isolated directory.
In Briefcase using site.getsitepackages() in the current interpreter works as expected, though. However people use venv in day-to-day development.
If you're talking about using site.getsitepackages() to retrieve the path in a subprocess and always using that... in Discord it was pointed out that if we we take the subprocess approach we should still hardcode paths for common distros.
Does this clear anything up? THanks.
There was a problem hiding this comment.
No, it doesn't clear anything up.
How is a .pth file being processed twice? It should only be processed when a site folder is added. And I'm not sure how Briefcase is involved here at all. Briefcase is an app that runs in a virtual environment. It uses the same Python import logic as all other Python programs.
As for hard coding - I have a vague recollection about mentioning hard coding paths, prior to seeing any actual implementation. What I'm saying now is that the implementation isn't difficult, and getsitepackages() will be reliable on all platforms, so why hardcode (with the risk that the path might change at some point)?
There was a problem hiding this comment.
@freakboy3742 I don't know why, but somehow it is processed twice indeed; the first time site.getsitepackages returns the global path and the second time it returns the venv path, so I'm saying that site.getsitepackages is unreliable in venv environments because the fact that it was called for the global path for the first time may be an accident.
An explicit test to clear things up, using a fresh venv and a very simple pth file:
(base) johnzhou@Johns-MacBook-Pro testing1 % python -m venv venv
(base) johnzhou@Johns-MacBook-Pro testing1 % conda deactivate
johnzhou@Johns-MacBook-Pro testing1 % source venv/bin/activate
(venv) johnzhou@Johns-MacBook-Pro testing1 % ls venv/lib
python3.10
(venv) johnzhou@Johns-MacBook-Pro testing1 % cat > venv/lib/python3.10/site-packages/testing.pth
import site; print(site.getsitepackages())
(venv) johnzhou@Johns-MacBook-Pro testing1 % python
['/opt/anaconda3/lib/python3.10/site-packages']
['/Users/johnzhou/testing1/venv/lib/python3.10/site-packages']
Python 3.10.14 (main, May 6 2024, 14:42:37) [Clang 14.0.6 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>>
(venv) johnzhou@Johns-MacBook-Pro testing1 %
In the above log, I made a new venv and made a test pth file to just print getsitepackages() result. Somehow 2 lines are printed, the first time with the global path and second with the limited venv path.
If our goal is to work properly only in Briefcase, then sure, we could use getsitepackages. But then our behavior in venv will not be very stable.
[Sidenote: That was my reasoning behind testing both venv and Briefcase -- there's some differences like this. Venv is the more complex case, but Briefcase is what we use in production, though.]
There was a problem hiding this comment.
I'm not sure I see how anything you've described means the .pth file is being processed twice.
You've shown a situation where you're returning the site packages for a (a) a conda interpreter, that is (b) wrapped in a virtual environment. And, in that environment, you're printing the site packages - and the answer has 2 locations in it. That's entirely expected.
The innovation that we're concerned about is very specifically calling the system python - not anything in a virtual environment, and not from a "non system" source. If the subprocess call isn't getting the system python, then that is a bug.
The only way I could see a .pth file being invoked twice would be if the .pth file is installed twice - once in your Conda environment, and once in your venv.
There was a problem hiding this comment.
But see that the 2 printings are on 2 separate lines. For the record I have not installed testing.pth in my global conda environment. Therefore the pth file is ran twice, there is only 1 statement.
Subprocess is calling the system python -- I thought you were referring to using getsitepackages directly in the current environemnt.
Sorry for misreading -- I will remove hardcoding. and since the subprocess approach is same as Briefcase and venv... I'll remove Briefcase testing.
Does this sound good?
There was a problem hiding this comment.
I'm still not sure I understand how a .pth file is being executed twice, unless it's a quirk of your specific environment (such as something in the conda site package that is recursively processing).
But, it sounds like that's a moot point, as we don't care about site packages in the active environment, only in the system environment, and that won't have the duplication concern.
| * **Ubuntu / Debian** - `sudo apt-get install python3-pyside6.qtwidgets` (and other modules you may need, such as ``python3-pyside6.qtcore``; Only available from Ubuntu 24.10+ / Debian 13+) | ||
|
|
||
| * **Fedora** - `sudo dnf install python3-pyside6` | ||
| * **Fedora** - `sudo dnf install python3-pyside6` and `sudo dnf upgrade --refresh` - **NOTE** that the second step is needed because installing PySide6 upgrades hundreds of packages, **some of them which have incorrect dependencies which MAY BRICK YOUR SYSTEM!** |
There was a problem hiding this comment.
This doesn't match my experience at all. On Fedora 42, dnf install python3-pyside6 installs 42 packages, almost all of which have qt- as a prefix. And the system is entirely stable afterwards - there's no "bricking" involved.
I'm not sure I even see how installing dependencies could brick a system...
There was a problem hiding this comment.
@freakboy3742 On a fresh Fedora 42 KDE Plasma Edition install in my VM, installing python3-pyside6 upgrades some of those Qt packages, which updates some KDE components; subsequently restarting gives me a blackscreen.
There was a problem hiding this comment.
I'm not denying you've seen that behavior... but it strikes me that this is much more likely to be a quirk of your specific VM config, or a transient bug that you hit. Fedora is a widely used, well-maintained project - the idea that it has pushed out a stable release that bricks machines on boot is inconsistent with that.
Can you point at a public ticket or announcement warning about this issue?
There was a problem hiding this comment.
@freakboy3742 I was able to track it down to at least 1 package that did not get updated. The reason that you're not seeing is that the KDE edition uses KWin as its window manager, and KWin depends on KDecoration package; updating Qt also upgrades KWin, but it didn't update the associated KDecoration version; the new KWins requires some function from KDecoration that did not exist before; so bad things happen.
An upstream bug report I've filed is at https://bugzilla.redhat.com/show_bug.cgi?id=2390486 with all the details for versions. This was consistnetly reproducable.
There was a problem hiding this comment.
That's a bug that you've opened, and it's had no response or reproduction by anyone else.
RedHat is a company that has a very large QA team. They have a 30 year history of producing robust and high quality software. I can't rule out that you've found a problem, but when you're the only person who apparently has the problem, that should start to raise questions about whether the problem actually exists (or, of it does, that it's a very niche problem that doesn't warrant a banner headline warning about bricking systems).
There was a problem hiding this comment.
Okay... on https://packages.fedoraproject.org/pkgs/kwin/kwin-common/fedora-42-updates.html#dependencies which is latest KWin, click on KDecoration in Dependencies which is https://packages.fedoraproject.org/pkgs/kdecoration/kdecoration/fedora-42.html which is < 6.3.90, even when on the KWin GitHub on 6.5.4 you see >=6.3.90 in CMakeLists.txt.
I think KDE Edition is the culprit here -- if you don't have KWin installed this is not an issue. Even if installing PySide does not upgrade KWin for some reason in the future... we should still warn about this.
I've also managed to trace down why the version is problematic in the bug report I posted, down to the specific API, consistent with system logs when it bricks.
This is the last concern -- but I think we should be on the defensive side about this.
EDIT -- FYI, to be clear: I don't doubt Fedora, and if this can be rewritten in a way that sounds less Fedora-reputation-defeating / scares people away, I'd be happy to accept suggestions.
There was a problem hiding this comment.
But we already are defensive - that's what "...and you need to run sudo dnf upgrade is. We've told people how to avoid the problem; we don't need to be alarmist about the problem beyond that.
|
@freakboy3742 I apologize that changes might not be made for a few days due to me being sick -- however tagging to let you know that I'd appreciate reponses to the things I've put inline and also to approve CI. |
|
@freakboy3742 I've made most of your requested changes, right now I have only 2 major concerns: 1 is the hardcoding of distro-specific paths, and 2 is whether testing in Briefcase is warranted. Both I have responded inline to, and in my opinion, your opinions no those are needed before I proceed with more changes. Thanks! |
freakboy3742
left a comment
There was a problem hiding this comment.
A few responses and review comments inline. CI is also failing, so there's something to look into there.
| * **Ubuntu / Debian** - `sudo apt-get install python3-pyside6.qtwidgets` (and other modules you may need, such as ``python3-pyside6.qtcore``; Only available from Ubuntu 24.10+ / Debian 13+) | ||
|
|
||
| * **Fedora** - `sudo dnf install python3-pyside6` | ||
| * **Fedora** - `sudo dnf install python3-pyside6` and `sudo dnf upgrade --refresh` - **NOTE** that the second step is needed because installing PySide6 upgrades hundreds of packages, **some of them which have incorrect dependencies which MAY BRICK YOUR SYSTEM!** |
There was a problem hiding this comment.
I'm not denying you've seen that behavior... but it strikes me that this is much more likely to be a quirk of your specific VM config, or a transient bug that you hit. Fedora is a widely used, well-maintained project - the idea that it has pushed out a stable release that bricks machines on boot is inconsistent with that.
Can you point at a public ticket or announcement warning about this issue?
|
|
||
| # Below, locations are hardcoded for common distros' layout, and those | ||
| # directories for package and distribution info are searched instead of | ||
| # the entire system site packages as defined above. |
There was a problem hiding this comment.
No, it doesn't clear anything up.
How is a .pth file being processed twice? It should only be processed when a site folder is added. And I'm not sure how Briefcase is involved here at all. Briefcase is an app that runs in a virtual environment. It uses the same Python import logic as all other Python programs.
As for hard coding - I have a vague recollection about mentioning hard coding paths, prior to seeing any actual implementation. What I'm saying now is that the implementation isn't difficult, and getsitepackages() will be reliable on all platforms, so why hardcode (with the risk that the path might change at some point)?
Removed unnecessary comments about system updates in CI workflow.
|
Solved all issues except the README one, which I've commented on. I just don't want anyone's screen not showing... it took some digging in the system logs to figure out the culprit. (and the KWin dependency is not the only culprit that showed up after installing python3-pyside6 on a fresh Fedora 42 KDE Edition install). |
freakboy3742
left a comment
There was a problem hiding this comment.
Ok - I think this is good enough to warrant an initial release. Thanks for all your work on this!
Fixes beeware/briefcase#2480
Initial setup of Package and CI workflows, in addition to small README and pyproject.toml fixes.
PR Checklist: