Skip to content

Conversation

@tomvanmele
Copy link
Member

What type of change is this?

  • Bug fix in a backwards-compatible manner.
  • New feature in a backwards-compatible manner.
  • Breaking change: bug fix or new feature that involve incompatible API changes.
  • Other (e.g. doc update, configuration, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
  • I ran all tests on my computer and it's all green (i.e. invoke test).
  • I ran lint on my computer and there are no errors (i.e. invoke lint).
  • I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

@tomvanmele
Copy link
Member Author

If a user install a bunch of packages from an environment, and then removes the environment without uninstalling the packages this can create problems when trying to install again later on.

The uninstall process now also removes all broken symlinks...

Copy link
Contributor

@Licini Licini left a comment

Choose a reason for hiding this comment

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

Looks good!

@tomvanmele
Copy link
Member Author

the following will now also happen during the install procedure:

  1. all installed packages that are not available in the current env will be removed (albeit quietly)
  2. all installed packages that are available will be reinstalled

@tomvanmele
Copy link
Member Author

not sure if this is what we mean to happen though.
however, it seems to me that packages from a different env should not be allowed to float around in a new install...

exit_code = 0

for name in os.listdir(scripts_path):
if name.startswith('compas') and not name.endswith('.py'):
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense for this to also include all installable_rhino_packages? In the case that packages is provided by the user, then something like roslibpy (which is a default installable package for compas_fab) would be missed by this check and also would not be added to packages by _filter_installable_packages to be removed and added again.

CHANGELOG.md Outdated
### Changed

* Changed `compas_rhino.uninstall` to also remove broken symlinks if no specific packages are provided for installation.
* Changed `compas_rhino.install` to also remove broken symlinks if no specific packages are provided for installation.
Copy link
Member

Choose a reason for hiding this comment

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

It looks like broken symlinks are removed whether or not packages are specified on install.

if not packages:
packages = []
for name in os.listdir(scripts_path):
if name.startswith('compas') and not name.endswith('.py'):
Copy link
Member

Choose a reason for hiding this comment

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

again, roslibpy or other default compas-installed packages?

Copy link
Member Author

Choose a reason for hiding this comment

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

is this also the case here?

Copy link
Member

Choose a reason for hiding this comment

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

yes, i think so.

CHANGELOG.md Outdated

### Changed

* Changed `compas_rhino.uninstall` to also remove broken symlinks if no specific packages are provided for installation.
Copy link
Member

Choose a reason for hiding this comment

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

'for un-installation'?

# add the packages that can't be imported from the current env to the list of symlinks to uninstall
# and remove the package name from the list of installable packages
# make a copy of the list to avoid problems with removing items
# note: perhaps this should already happen in the filter function...
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense to me.

@tomvanmele tomvanmele merged commit 08f76e9 into main Nov 19, 2021
@tomvanmele tomvanmele deleted the remove-broken-links branch November 20, 2021 07:41
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.

4 participants