Skip to content
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

Isolate dependencies from other add-ons #284

Merged
merged 5 commits into from
Jun 5, 2024
Merged

Conversation

keianhzo
Copy link
Contributor

@keianhzo keianhzo commented May 8, 2024

This PR isolates selenium from the Blender python packages to avoid conflicts. We still do it using pip but using a specific target directory and now the modules are loaded in isolation using importlib.

Copy link
Contributor

@Exairnous Exairnous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a nice improvement. I have a few questions though:

  • Why have the install/uninstall deps operators been changed to only work with a single module?
  • Why did you create a dependencies folder with an __init__.py instead of just creating dependencies.py?
  • Why has the check to ensure pip is present been removed from the install deps operator?

addons/io_hubs_addon/preferences.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/preferences.py Outdated Show resolved Hide resolved
@Exairnous
Copy link
Contributor

Oh, also, I think this will leave the old selenium module installed. It would be nice to remove it, if that's not too hard.

@keianhzo
Copy link
Contributor Author

Oh, also, I think this will leave the old selenium module installed. It would be nice to remove it, if that's not too hard.

Yeah that's a problem with the way we were doing things before we can't really remove it as we don't know if any other add-on might have installed or is using it.

@Exairnous
Copy link
Contributor

Thank you for the updates.

Yeah that's a problem with the way we were doing things before we can't really remove it as we don't know if any other add-on might have installed or is using it.

Good point. That makes sense.

Any update on my questions from above?

Why have the install/uninstall deps operators been changed to only work with a single module?
Why did you create a dependencies folder with an init.py instead of just creating dependencies.py?
Why has the check to ensure pip is present been removed from the install deps operator?

@keianhzo
Copy link
Contributor Author

keianhzo commented May 15, 2024

Why have the install/uninstall deps operators been changed to only work with a single module?

We were only using these operators to install one dependency so I simplified the code to just get one dep.

Why did you create a dependencies folder with an init.py instead of just creating dependencies.py?

No strong reason, I guess it makes it simple to do from .dependencies import get_XXX

Why has the check to ensure pip is present been removed from the install deps operator?

I think we can safely assume that pip is always present in Blender installs

Copy link
Contributor

@Exairnous Exairnous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were only using these operators to install one dependency so I simplified the code to just get one dep.

I figured it might be nice to have one button to install all dependencies and that supporting multiple would future proof that, but one button per dependency works too and is simpler. So, I guess I don't have a strong opinion either way.

No strong reason, I guess it makes it simple to do from .dependencies import get_XXX

I believe you can do that with a regular .py file, but this works fine too; just seems a little weird to me, that's all.

I think we can safely assume that pip is always present in Blender installs

I just tried with Blender-3.3.2 on my system and got this error:

01   Dependencies install has failed
02   /home/<user>/Workshop/CustomApps/blender-3.3.2/3.3/python/bin/python3.10: No module named pip

So I think ensure pip needs to remain. Some people recommend upgrading pip as well, but I'm not entirely sure if we need to: https://blender.stackexchange.com/questions/56011/how-to-install-pip-for-blenders-bundled-python

addons/io_hubs_addon/preferences.py Outdated Show resolved Hide resolved
@keianhzo
Copy link
Contributor Author

keianhzo commented Jun 4, 2024

I've addressed the feedback and added back pip and added pip upgrade.

@keianhzo keianhzo requested a review from Exairnous June 4, 2024 08:14
Copy link
Contributor

@Exairnous Exairnous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@keianhzo keianhzo merged commit 95cea92 into master Jun 5, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants