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

Improved manifest #11

Merged
merged 39 commits into from
Jun 13, 2020
Merged

Improved manifest #11

merged 39 commits into from
Jun 13, 2020

Conversation

haz
Copy link
Contributor

@haz haz commented May 26, 2020

Closes #4

packages/TEMPLATE/install Outdated Show resolved Hide resolved
packages/TEMPLATE/install Outdated Show resolved Hide resolved
packages/TEMPLATE/install Outdated Show resolved Hide resolved
packages/TEMPLATE/install Outdated Show resolved Hide resolved
packages/TEMPLATE/install Outdated Show resolved Hide resolved
packages/TEMPLATE/uninstall Outdated Show resolved Hide resolved
packages/TEMPLATE/uninstall Outdated Show resolved Hide resolved
packages/fd/manifest.json Outdated Show resolved Hide resolved
packages/fd/manifest.json Outdated Show resolved Hide resolved
packages/fd/run Outdated Show resolved Hide resolved
@haz
Copy link
Contributor Author

haz commented May 27, 2020

I still need to re-work the installation, but I'm getting closer. Thanks for the feedback!

@haz
Copy link
Contributor Author

haz commented May 30, 2020

@FlorianPommerening Want to have another look?

I put in a dependency crawler to remove the dangling ones, but then couldn't conclude how it should work. If I install lama, which installs downward, then uninstalling downward is forbidden. That makes sense. But should uninstalling lama also nix downward?

Perhaps I keep a list of those packages explicitly installed, and detect "no longer needed packages" from those that are installed and not on "the list"?

@haz haz changed the title [WIP] Improved manifest Improved manifest May 30, 2020
Copy link
Collaborator

@FlorianPommerening FlorianPommerening left a comment

Choose a reason for hiding this comment

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

Looks much better to me now. I added more comments (let me know if I'm being to nit-picky).

About your question on how to deal with dependencies when uninstalling. I think we had some discussion on this already but I don't remember where (in the other pull request maybe?) IIRC the final suggestion was to recursively look up dangling dependencies and ask the user if they want to get rid of them.

planutils/__init__.py Outdated Show resolved Hide resolved
planutils/__init__.py Outdated Show resolved Hide resolved
planutils/__init__.py Outdated Show resolved Hide resolved
planutils/__init__.py Outdated Show resolved Hide resolved
planutils/package_installation.py Outdated Show resolved Hide resolved
planutils/package_installation.py Outdated Show resolved Hide resolved
planutils/package_installation.py Outdated Show resolved Hide resolved
planutils/package_installation.py Outdated Show resolved Hide resolved
planutils/packages/lama/run Outdated Show resolved Hide resolved
planutils/packages/TEMPLATE/run Outdated Show resolved Hide resolved
@haz
Copy link
Contributor Author

haz commented Jun 5, 2020

Ok, removing the WIP. If you're up for it, have another go with a focus on bugs. Major feature requests / enhancements we can divert to issues...I'd like to open it up for others to start writing packages.

Two key things to note:

  1. Upgrading is absurdly naive atm. For those packages installed, uninstall/reinstall. This presumes no settings, persistent data for the packages, etc. I think we'd need to add versioning to the manifest in order to cross-check, and perhaps even have an upgrade script mandatory for each package. Advanced upgrade functionality #17

  2. The dependency nest on uninstalling is still nagging me. If you install lama, downward also comes. If you then uninstall downward, it confirms to remove both it and lama. This makes sense. But what about this situation?

  • Install awesome-planner-set
  • Bunch of planners are installed, including XYZ
  • Uninstall XYZ
  • This removes awesome-planner-set since XYZ was a dependency
  • What happens to all the other planners that came along with XYZ? What of the things those planners required? What of some-other-planner-set which also required some of those planners?

I think it works ok as it's currently implemented, but like I said, still bugs me...

@FlorianPommerening
Copy link
Collaborator

What about this?

def uninstall(packages):
    ancestors = recursively_collect_ancestors(packages)
    to_uninstall = ancestors + packages
    if confirm("The following packages will be uninstalled:" + to_uninstall):
        for p in to_uninstall:
            uninstall_package(p)

    # At this point, we satisfied the user request in a minimal way
    # (uninstalling only the necessary parts). Now let's see what else we should remove.

    descendants = [d for p in to_uninstall for d in p.dependencies]
    # is_required(p) should check if there is an installed package that has p as a dependency.
    dangling_packages = [p for p in descendants if is_installed(p) and not is_required(p)]
    for p in dangling_packages:
        if confirm("The package '%s' is a dependency of an uninstalled package. "
                   "Should it be removed as well?" % p):
            uninstall([p])

In you use case this would look like this

$ planutils uninstall XYZ
The following packages will be uninstalled: awesome-planner-set XYZ
continue? [y/N] y
uninstalling awesome-planner-set ... done
uninstalling XYZ ... done
The package 'ABC' is a dependency of an uninstalled package. Should it be removed as well?
continue? [y/N] y
The following packages will be uninstalled: ABC
continue? [y/N] y
uninstalling ABC ... done

If the dependency structure is complicated, there might be a lot of questions to the user, so maybe showing all dangling packages and giving the options "yes to all", "no to all", "let me decide for each one" would be better.

Copy link
Collaborator

@FlorianPommerening FlorianPommerening left a comment

Choose a reason for hiding this comment

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

I tried to focus on potential bugs but a few feature requests and style comments snuck in. Feel free to ignore them or move them to a new issue, of course.

planutils/__init__.py Outdated Show resolved Hide resolved
planutils/__init__.py Outdated Show resolved Hide resolved
planutils/__init__.py Outdated Show resolved Hide resolved
planutils/__init__.py Show resolved Hide resolved
planutils/__init__.py Outdated Show resolved Hide resolved
planutils/__init__.py Outdated Show resolved Hide resolved
planutils/package_installation.py Show resolved Hide resolved
planutils/package_installation.py Show resolved Hide resolved
planutils/packages/TEMPLATE/manifest.json Show resolved Hide resolved
planutils/package_installation.py Show resolved Hide resolved
@haz
Copy link
Contributor Author

haz commented Jun 6, 2020

Will take a look at the more advanced uninstallation you suggested, but snagged the rest. How best to get at the sizing remains open ( #19 ), but at least it's a start.

@haz
Copy link
Contributor Author

haz commented Jun 7, 2020

Ok, included the optional crawl. Didn't do a yes-to-all, as a "no" to one branch could cut off a chain of dependencies that you don't necessarily want to consider.

Tested and ready.

@haz
Copy link
Contributor Author

haz commented Jun 11, 2020

@FlorianPommerening Lack of response an approval? :P

Copy link
Collaborator

@FlorianPommerening FlorianPommerening 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 to me.

planutils/__init__.py Outdated Show resolved Hide resolved
planutils/package_installation.py Show resolved Hide resolved
planutils/package_installation.py Outdated Show resolved Hide resolved
@haz haz merged commit ec45c2a into master Jun 13, 2020
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.

Uninstall/upgrade functionality
2 participants