-
-
Notifications
You must be signed in to change notification settings - Fork 525
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
Updated some extra requirements #797
Conversation
It's great you did this testing, thanks.
That seems like a good idea to me - thanks for adding it. My understanding (out of date, topped up just now by skimming pypa/setuptools#1698) is that for it to be effective, either we'd also need to require a fairly recent setuptools (newer than the 30.3 we currently state, anyway) or we'd also need to include pyproject.toml in MANIFEST.in. Any idea about that, or practical experience? I don't have any.
There's unfortunately no comment saying what the pin is for, but blame says @philippjfr added it, so he might remember :) Meanwhile, I think the spacing around package and version is (unfortunately) significant - could you match it to the other examples? (It's used not only by setuptools.)
I can no longer remember the current situation, but nbsite (used for panel's docs) lists phantomjs as an optional dependency required for building the gallery. Philipp will say definitively, but I'm going to guess that it is still required. At this point, you can probably stop reading. However, there are a number of annoying asides about the above I'd like to note:
|
Last I remember we needed a very specific build of VTK compiled against libmesa, which made it possible to run the example without a windowing system on CI. |
Happy to help, although it looks like I might have caused you more work with this PR than I saved.
I don't have experience with pyproject.toml and setuptools, I only use it in conjunction with poetry. But I'm fairly sure that it should never be listed in MANIFEST.in, something about cleanly separating packaging metadata and source which was one of the more important reasons for PEP 518 in the first place. In case of sdist distributions the build backend is in charge of providing a build script that applies the packaging data in a setup.py fashion, and pyproject.toml is still not supposed to be part of the tarball. I only added setuptools.build_meta because my setuptools is recent enough to pick up pyproject.tomls and was complaining about it being unspecified and defaulting to setuptools - I can research more on the matter in order to be certain what it exactly does and if it can cause breaks with older versions.
It would be nice if it could be asserted if vtk8.1.1 is really necessary, in particular since python3.7 is listed as supported. Another alternative would be to bug the vtk people to release 8.1.1 for python3.7 to pyPI. And I rolled the additional space back, learned something scary/new right there.
Doing a pip install will always have the issue of missing non-python packages, no way around that without conda (this issue as a whole would be part of #706 and adding manual install instructions to a COLLABORATING.md, so it's fine if it isn't handled in this part of the setup). I should have listed my system dependencies in the "environment" section though, which I added right now (but I might have forgotten something, those installs are weeks ago already and my memory goes barely a handful of days back). But if a dev-pip-install is supposed to work, the lists behind the
It was, back in 2012. There is All in all I think v0.7.0rc3 is good, these changes are not needed since they only deal with the dev setup. The vtk situation should be resolved somehow, but doesn't feel like something blocking to me. |
Not saying it makes sense :) Just that things like pypa/setuptools#1650 (from For now, I think (but am not the authority here!) that we should restore phantomjs and the existing vtk pin in this PR unless you can demonstrate the website build working on CI (i.e. running without error, producing correct site) without them. You might need help to make the necessary builds happen to do that, I'm not sure. And meanwhile we'll accept that nobody can pip install the doc optional extras for now. However, that's an important issue, so I think we should move discussion over to #706 as you say. We do care about #706; sorry nobody's responded there yet. Does that seem reasonable? |
This is correct, it is still required for thumbnailing bokeh plots. Most thumbnails are downloaded at this point but there's at least a couple that are auto-generated and I would expect to fail if phantomjs isn't available. |
Well, I checked again and it seems I was plain wrong and the pyproject.toml needs to be part of an sdist after all. In that case it should be included in MANIFEST.in, but given that all of this is a bit uncertain right now and all other changes need to be rolled back, this PR should just be closed I think. I can look more into this, but then in the context of #706. No worries about slow responses there, I am patient =) |
Ok, I've closed this PR for now. But...
In theory, https://github.com/pyviz-dev/pyctdev/issues is where we track packaging issues across all our projects. Although there's code in that repo, you don't have to pay attention to it :) That issue tracker is also probably where we should discuss #706 since it also affects all our projects, but it's apparently not possible to transfer issues between orgs so I can't move it. |
And also added setuptools explicitly as build backend.
phantomjs
is no longer on pyPI and didn't seem to be necessary for generating the docs, andvtk
doesn't have a python3.7 compatible wheel for version8.1.1
.Environment:
Tests run:
pip install -e .
✔️pip install -e .[tests]
✔️pip install -e .[extras]
✔️pip install -e .[doc]
✔️pip wheel -w dist . --no-deps
✔️nbsite generate-rst --org pyviz --project-name panel && nbsite build --what=html --output=builtdocs
✔️pytest panel/tests/
✔️