facelessuser / soupsieve

A modern CSS selector implementation for BeautifulSoup
https://facelessuser.github.io/soupsieve/
MIT License
205 stars 38 forks source link

circular dependency /bs4 #231

Closed jschueller closed 2 years ago

jschueller commented 3 years ago

since #229 you depend on bs4, but the bs4 package already depends on soupsieve

this prevents from building the package

facelessuser commented 2 years ago

Can you describe how this is preventing package build? Can you give context? Is it failing in pip or some specialized build process?

jschueller commented 2 years ago

in conda-forge, this it is trigerred by conda-build if I add beatifulsoup to the dependencies list (first I got an import error for bs4): https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=402948&view=logs&jobId=656edd35-690f-5c53-9ba3-09c10d0bea97&j=656edd35-690f-5c53-9ba3-09c10d0bea97&t=e5c8ab1d-8ff9-5cae-b332-e15ae582ed2d conda says: RuntimeError: soupsieve cannot depend on itself that's when I noticed it was a circular dependency problem

facelessuser commented 2 years ago

This may just be a conda-forge problem.

  1. SoupSieve works fine in pip.
  2. SoupSieve has always, even before adding static typing, had bs4 as a dependency.
  3. SoupSieve did not change anything in regard to dependencies with the addition of static types and made sure to only reference the static types as strings to ensure they are only evaluated once everything is loaded and compiled.
  4. As far as runtime, SoupSieve only references bs4 inside functions and does not reference it globally to prevent issues at runtime when importing. This has not changed.
jschueller commented 2 years ago

That's not what I'm seing here: https://github.com/facelessuser/soupsieve/blob/main/soupsieve/__init__.py#L33

facelessuser commented 2 years ago

That's not what I'm seing here: https://github.com/facelessuser/soupsieve/blob/main/soupsieve/__init__.py#L33

Using and importing are very different things. We've been importing in css_match.py prior to static typing. Yes it is now imported in __init__.py now as well, but again, not used globally. Typing is defined in strings.

Here we install in a fresh virtual environment:

(test) ➜  souptest pip install beautifulsoup4
Collecting beautifulsoup4
  Using cached beautifulsoup4-4.10.0-py3-none-any.whl (97 kB)
Collecting soupsieve>1.2
  Using cached soupsieve-2.3-py3-none-any.whl (37 kB)
Installing collected packages: soupsieve, beautifulsoup4
Successfully installed beautifulsoup4-4.10.0 soupsieve-2.3
WARNING: You are using pip version 21.2.4; however, version 21.3.1 is available.
You should consider upgrading via the '/Users/facelessuser/Code/souptest/test/bin/python3.9 -m pip install --upgrade pip' command.

No problems

facelessuser commented 2 years ago

I am open to possible suggestions, but I do not have a way right now to test conda-forge.

facelessuser commented 2 years ago

Is there a way for you to try out this branch? https://github.com/facelessuser/soupsieve/pull/232

Here we only do the new import if we are running typing. We still import it in css_match.py as we've done for a while now, but we don't do it in __init__.py except when typing

I need to know if the above alleviates the issue before pushing it through.

facelessuser commented 2 years ago

@jschueller considering conda-forge hasn't choked on soupsieve since 2.1.0+, I'm pretty sure that #232 will fix the issue as it seemed to never have an issue with soupsieve importing bs4 in css_match.py. If for some reason there is something in conda-forge that requires css_match.py to also be patched suddenly, I think I could alleviate that as well, but I'd be surprised if it was necessary considering it accepting past versions without issues.

If/when you can confirm whether #232 fixes the issue or not, I can get the change out promptly.

jschueller commented 2 years ago

indeed, now the error is in css_match.py instead of init.py with your patch

facelessuser commented 2 years ago

Okay, so you guys haven't updated since before 2.1.0 then. I have pushed another commit which should hopefully get past whatever conda-forge is checking.

As far as Python is concerned, there is nothing specifically wrong with what we are doing. This is specifically a conda-forge quirk, but hopefully, the last commit works around this quirk.

facelessuser commented 2 years ago

Just ping me if commit 403f3d8 fixes things

facelessuser commented 2 years ago

@jschueller I'd love to get this closed for you. Have you had a chance to test the latest commit of the bugfix branch? The only thing waiting is confirmation that css_match no longer throws an error in conda-forge.

jschueller commented 2 years ago

no luck either, but actually I can work around this if I add bs4 into the test dependencies

facelessuser commented 2 years ago

Yes, bs4 is a test dependency as SoupSieve works on an instantiated BeautifulSoup object. I thought we were dealing with some Conda Forge check of avoiding circular dependencies, not an actual runtime failure.

jschueller commented 2 years ago

me too