Skip to content

Fix #191: improve handling of dependencies#201

Open
Mateusz-Grzelinski wants to merge 2 commits into
JacquesLucke:masterfrom
Mateusz-Grzelinski:improve-dependencies
Open

Fix #191: improve handling of dependencies#201
Mateusz-Grzelinski wants to merge 2 commits into
JacquesLucke:masterfrom
Mateusz-Grzelinski:improve-dependencies

Conversation

@Mateusz-Grzelinski

@Mateusz-Grzelinski Mateusz-Grzelinski commented Sep 11, 2024

Copy link
Copy Markdown
Collaborator

This PR will allow for update/downgrade of dependencies when needed.

  • do not use import as a way of checking for version or checking if module can be imported. When Windows opens file it can not longer be removed! The same problem is being faced by blender developers: https://projects.blender.org/blender/blender/commit/b38439db99bd3791ff1dc55ddfaa9d227b1bad87
  • use one pip call to install dependencies. It allows to correctly handle constraints.
  • when package is outside of constraints, perform clean installation. Clean installation in most cases is not necessary but with --target it has strange corner cases.
    • for example when you are not doing clean install, but only upgrader with pip install --upgrade you can only downgrade package...
  • commit semver dependency with code

This PR should be pretty complete, can not think of anything specific to test.

- use one pip call to install dependencies. It allows to correctly handle constraints.
- when package is outside of constraints, perform clean installation. Clean installation in most cases is not necessary but with --target it has strange corner cases.
- commit semver dependency with code
@Mateusz-Grzelinski Mateusz-Grzelinski marked this pull request as ready for review September 11, 2024 17:51
if requires_reinstall:
print(f"INFO: dependencies reuqire update because of {requires_reinstall}, reinstalling...")
print(f'INFO: removing "{bpy.utils.user_resource("SCRIPTS", path="modules")}"')
shutil.rmtree(bpy.utils.user_resource("SCRIPTS", path="modules"))

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This carries a low risk of removing user data... The only think that can be done is to move the dir to backup location or track exactly what files are being installed.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What makes you think that the risk is low? Sounds like removing a folder that is not specific to the extension is quite risky.
May be better to tell the user in some way that the folder should be removed instead of just doing it.

@Mateusz-Grzelinski Mateusz-Grzelinski Sep 30, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

i don't think this folder is used often.
The alternative is to track all names of dependencies and remove folder and .dist-info folder. Like jinja and ^jinja.*-.*\.dist-info$. A static list of dependencies is easiest to write.
just a note for myself: I need to further reduce version of wergzeug as 3.0.3 supports python >=3.8

@Mateusz-Grzelinski Mateusz-Grzelinski Sep 30, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

made a quick implementation, might still have mistakes

Some snipet with alternative:

def list_installed_packages():
    target = get_package_install_directory()
    command = [str(python_path), "-m", "pip", "freeze", "--disable-pip-version-check", "--exclude-editable", "---path", target]
    print("INFO: execute: ", " ".join(_escape_space(c) for c in command))
    output = subprocess.check_output(command, cwd=_CWD_FOR_SUBPROCESSES).decode()
    for package in output.splitlines():
        yield package.split("==")[-1]

if requires_reinstall:
print(f"INFO: dependencies reuqire update because of {requires_reinstall}, reinstalling...")
print(f'INFO: removing "{bpy.utils.user_resource("SCRIPTS", path="modules")}"')
shutil.rmtree(bpy.utils.user_resource("SCRIPTS", path="modules"))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What makes you think that the risk is low? Sounds like removing a folder that is not specific to the extension is quite risky.
May be better to tell the user in some way that the folder should be removed instead of just doing it.

real_version = Version.parse(package_version(name))
assert isinstance(requested_version, Version), requested_version
assert isinstance(real_version, Version), real_version
if "==" in name and not (real_version == requested_version):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is there no builtin library that helps with these kinds of checks? It feels a bit weird to have to implement it manually.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

we are stepping into the boots of pip. I am forcing pip to do very specific things to my dependencies:

  • dont touch deps when they are in acceptable range and
  • oh, thats it, nothing more.

If you read about the --target option you will see that it is not well supported...

The typical way of using pip is lock file or requirement file. Lock file is way to restrictive in our case, and if you use requirement file with -U you will force everyone to upgrade.

Sooo... yes, it is messy. I am not saying it is the best, this is what I came up with

Comment thread pythonFiles/include/blender_vscode/installation.py Outdated
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.

2 participants