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

Namespace for Packages and Dependencies Conflict #1028

Closed
princemaple opened this issue Nov 17, 2015 · 25 comments
Closed

Namespace for Packages and Dependencies Conflict #1028

princemaple opened this issue Nov 17, 2015 · 25 comments

Comments

@princemaple
Copy link

Context

I had jinjia2 package installed

Problem

I installed the shiny https://github.com/facelessuser/ColorHelper which depends on jinjia2, https://github.com/facelessuser/ColorHelper/blob/master/dependencies.json, and facelessuser/ColorHelper#12 happens

Copying my comment on the above issue

Looks like there's a bug in package dependencies with existing packages.. I had jinja installed for my python projects, and it's the only package that's in the dependencies and is already installed. After I removed it, the problem goes away, but I can't use it anymore.

Copying log over

Package Control: Removed directory for orphaned package jinja2
Package Control: Installing 1 missing dependency
ignored packages updated to: ["0_package_control_loader", "Anaconda", "Behave Color Scheme", "Color Highlighter", "ColorSchemeEditor", "DrSync", "FindKeyConflicts", "Git", "Project Manager", "Sublime Files", "SublimeHaskell", "SublimeLinter", "SublimeLinter-pylint", "Terminal", "Vintage"]
unloading plugin 0_package_control_loader.00-package_control
unloading plugin 0_package_control_loader.01-pygments
unloading plugin 0_package_control_loader.02-bz2
unloading plugin 0_package_control_loader.50-markupsafe
unloading plugin 0_package_control_loader.50-python-markdown
unloading plugin 0_package_control_loader.51-jinja2
unloading plugin 0_package_control_loader.55-mdpopups
reloading Packages/User/Package Control.sublime-settings
Package Control: Installed missing dependency jinja2
Package Control: Skipping automatic upgrade, last run at 2015-11-17 13:33:51, next run at 2015-11-17 14:33:51 or after
reloading Packages/User/Preferences.sublime-settings
ignored packages updated to: ["Anaconda", "Behave Color Scheme", "Color Highlighter", "ColorSchemeEditor", "DrSync", "FindKeyConflicts", "Git", "Project Manager", "Sublime Files", "SublimeHaskell", "SublimeLinter", "SublimeLinter-pylint", "Terminal", "Vintage"]
reloading plugin 0_package_control_loader.00-package_control
reloading plugin 0_package_control_loader.01-pygments
reloading plugin 0_package_control_loader.02-bz2
reloading plugin 0_package_control_loader.50-markupsafe
reloading plugin 0_package_control_loader.50-python-markdown
reloading plugin 0_package_control_loader.51-jinja2
reloading plugin 0_package_control_loader.55-mdpopups
error: Package Control

1 missing dependency was just installed. Sublime Text should be restarted, otherwise one or more of the installed packages may not function properly.
reloading Packages/User/Package Control.sublime-settings
reloading Packages/User/Preferences.sublime-settings
@facelessuser
Copy link

Ahh! It's a namespace thing. Windows is case insensitive. So jinja2 and Jinja2 clash (but only on Windows). It's funny, @FichteFoll and I just today acknowledge this issue here: wbond/package_control_channel#4986. I almost submitted markdown which would have clashed with Markdown.

@princemaple
Copy link
Author

facepalm

@FichteFoll
Copy link
Collaborator

Yeah, that sucks ... I'll update the tests today evening (didn't have time to do that yesterday) so this won't happen again.

@facelessuser
Copy link

That is cool and will really help, but how do you propose we resolve the current conflict of the jinja2 dependency and the Jinja2 package?

I guess one solution could be to relocate all the dependencies so their folder name wouldn't clash as there actual name should import fine in Python.

The easier solution is change the name by which one of them are known. Neither are owned by me, and I am not sure how many people use the jinja2 dependency. I am not sure which is easiest to do.

@wbond
Copy link
Owner

wbond commented Nov 17, 2015

@facelessuser I'd prefer not to relocate dependencies because it is useful to have common settings files and menus in dependencies.

I've actually already launched a dependency utilizing that, and it is kind of the point – the dependency is a common component for loading settings for Go-related packages, and includes a single menu entry.

@facelessuser
Copy link

@wbond Understood.

@kudago your input would be helpful. I have also created an issue on teddy_bear_maniac's bitbucket repo pointing back here.

@facelessuser
Copy link

Maybe having the Jinja2 package renamed via:

        // If you rename a package, you can provide the previous name(s) so
        // that users with the old package name can be automatically upgraded
        // to the new one.
        {
            "name": "Alignment",
            "details": "https://github.com/wbond/sublime_alignment",
            "previous_names": ["sublime_alignment"],
            "releases": [
                {
                    "sublime_text": "*",
                    "tags": true
                }
            ]
        },

Maybe this would be easiest. That, and we would need better conflict detection.

  1. There should be a case insensitive folder name check.
  2. This might already exist, but dependency library folder names (the actual library to be imported) should also be checked against with package folder names (case sensitive) to avoid import issues.

@FichteFoll
Copy link
Collaborator

@wbond, dependencies could still go into a sub-directory under Packages, e.g. .deps or _dependencies or whatever convention you like, which still allows all non-Python resources to be loaded while separating them physically from actual packages. But that should go to to #800.

Regarding the issue at hand, I'm not sure if renaming is even supported for dependencies, so @wbond should comment on that.
Besides, moving dependencies to a different location physically still does not solve name conflicts because, on ST3, all packages are exposed to being imported anyway (since packages dir is in sys.path). Since dependencies, by default iirc, get appended to the path, you won't be able to import the dependency over the package with the same name.

@facelessuser
Copy link

Regarding the issue at hand, I'm not sure if renaming is even supported for dependencies, so @wbond should comment on that.

That is why I was thinking the Package, not the dependency could rename.

Since dependencies, by default iirc, get appended to the path, you won't be able to import the dependency over the package with the same name.

Yes, but it would be nice to have a validation test when people are submitting to Package Control so they don't unexpectedly run into a situation where user X can't import dependency because they use Package Y.

@FichteFoll
Copy link
Collaborator

That is why I was thinking the Package, not the dependency could rename.

Just debating whether the option is even available.

Yes, but it would be nice to have a validation test when people are submitting to Package Control so they don't unexpectedly run into a situation where user X can't import dependency because they use Package Y.

I know, I know. I just got home.

@facelessuser
Copy link

@FichteFoll understood :).

@wbond
Copy link
Owner

wbond commented Nov 17, 2015

@FichteFoll Does Sublime Text scan subfolders for menus and settings files? Currently I don't have the time or interest in mucking about with how dependencies work.

I don't specifically recall implementing renaming of dependencies. Instead I would think you should release a new version of your package with a dependency having a different name. Package Control will first install the new dependency, then detect the old dependency is unused, and remove it. It may end up removing the package that has the same name as the dependency. Can't say I know for sure without testing it all out, and unfortunately I don't really have the time to dig into it in the near future.

@facelessuser Does jinja2 include shared libraries or any sort of binary? If not, it may be simpler to just embed it in your package. Then you don't need to deal with the namespace issues at all, you can just do relative imports.

Sorry that I don't have a better answer for all of this. Dependencies ended up being one of those types of projects where you put a ton of work into implementing something that seems really useful, only to find they aren't used all that much, and instead just lead to more "complicated" questions that have to be answered.

Maybe someday Jon will release Sublime Text 3 and it will have a built-in package manager that solves all of these issues. 😄

@facelessuser
Copy link

I don't specifically recall implementing renaming of dependencies. Instead I would think you should release a new version of your package with a dependency having a different name. Package Control will first install the new dependency, then detect the old dependency is unused, and remove it. It may end up removing the package that has the same name as the dependency. Can't say I know for sure without testing it all out, and unfortunately I don't really have the time to dig into it in the near future.

I have no problem providing another jinja2 dependency with a different name if that is how we want to go.

Does jinja2 include shared libraries or any sort of binary? If not, it may be simpler to just embed it in your package. Then you don't need to deal with the namespace issues at all, you can just do relative imports.

That is a possibility, I'd have to see how well it works, but it was also what I was trying to avoid by using dependencies. I originally pulled it in to take advantage of the usefulness of Package Control dependencies...looks like I got burned 😄 . I think with a couple of little Travis-CI tests I think these issues can be avoided. It is mainly just aggregating the package names, dependency folder names, and dependency root level import names and doing a case insensitive folder name check and a case sensitive import name check.

@FichteFoll
Copy link
Collaborator

@wbond, yes, it does scan subfolders for everything, besides Python plugins.

I suppose the issue with jinja2 is that other modules/libraries/dependency might rely on jinja2 being available via import jinja2 and would require a rewrite (i.e. slight adjustments) if it had a different name.
Other than that, we should probably find the other package (don't think it's more than one) that's using jinja2 and just rename the dependency in both the channel and the packages' dependencies. PC should fix most issues on the next restart. That is, if it doesn't uninstall the Jinja2 package.

@facelessuser
Copy link

From what I can tell, teddy_bear_maniac added jinja2 for his package: https://bitbucket.org/teddy_beer_maniac/sublime-text-glorified-copy-n-paste. So that shouldn't be a problem if he is actively maintaining his packages and is readily available.

@FichteFoll
Copy link
Collaborator

@teddybeermaniac
Copy link

Hello, I read all of this after one of you wrote to me on bitbucket, but I couldn't respond earlier.

At first I had jinja2 embedded in GCnP, but after discussion with @FichteFoll on my pr to include it in the repository I moved it out into a dependency. As you can see in comments, jinja2 uses absolute imports so it itself expects jinja2 to be on path, but I found a workaround for this which doesn't require patching it (also in comments), so there's no problem in moving it back if that turns be the best way to work around this issue.

@facelessuser
Copy link

I don't think moving it to the package is necessary. Just changing the PC alias name. For instance, I use python-markdown for the markdown dependency, and I can still use import markdown. Other packages would list the alias as the dependency, but still import it as expected.

@teddybeermaniac
Copy link

And it doesn't conflict with Markdown, the package, on Windows? They still both are on sys.path, aren't they? How does python work in this case (or lack thereof ;)) on Windows?

EDIT:
I'm asking because @FichteFoll wrote

on ST3, all packages are exposed to being imported anyway (since packages dir is in sys.path). Since dependencies, by default iirc, get appended to the path, you won't be able to import the dependency over the package with the same name.

FichteFoll added a commit to wbond/package_control_channel that referenced this issue Nov 17, 2015
Some file systems are case-insensitive, thus making differently cased
package names non-unique.

Also made me aware of the issue fixed in
fc6448f.

Partly addresses wbond/package_control#1028.
@facelessuser
Copy link

So we all know that the following conflicts on windows: markdown and Markdown.

And package control unpacks dependencies like this:

alias
    all
        markdown

So the following conflicts:

markdown
     all
         markdown
Markdown

But if we rename the alias we have no path conflict

python-markdown
    all
        markdown
Markdown
sys.path = ['Packages', 'Packages/python-markdown/all']

Python imports are case sensitive, so there is no conflict with:

import markdown
import Markdown

@FichteFoll
Copy link
Collaborator

In that case we might actually just rename the dependency name and bank on Python's case-sensitive imports (for both markdown and jinja2). I think that sounds like a good idea, although not inherently transparent. But I don't think this is not something that needs to be resolved by Package control itself.

I got the tests updated in wbond/package_control_channel@8fa08bd and also added checks for dependency<->package name checks locally but can't push them yet since they would break obviously.

Thus, I propose you two change your dependency specification to "python-jinja2" and I rename the dependency in dependencies.json (preferredly within a short time frame).

@teddybeermaniac
Copy link

I'm on my phone now, away from my computer, I will update GCnP tomorrow (20:30 where I sit) and ping you in here.

@facelessuser
Copy link

@FichteFoll , so does your test make sure that the dependency root level imports don't conflict with Package folder names (case sensitive)? If so, we should be pretty good (obviously you can't protect against others adding stuff to sys.path).

@FichteFoll
Copy link
Collaborator

so does your test make sure that the dependency root level imports don't conflict with Package folder names (case sensitive)?

No, that's not possible.

Or well, it might be possible but extremely infeasable.

@facelessuser
Copy link

Yeah, that's right, you would have to poll each package...I see that now. Oh well.

facelessuser added a commit to facelessuser/ColorHelper that referenced this issue Nov 17, 2015
facelessuser added a commit to facelessuser/QuickCal that referenced this issue Nov 17, 2015
facelessuser added a commit to facelessuser/ScopeHunter that referenced this issue Nov 17, 2015
FichteFoll added a commit to wbond/package_control_channel that referenced this issue Nov 17, 2015
It collided with the Jinja2 package previously.

ref: wbond/package_control#1028
FichteFoll added a commit to wbond/package_control_channel that referenced this issue Nov 17, 2015
@wbond wbond changed the title Package dependeny bug with existing packages Namespace for Packages and Dependencies Conflict Aug 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants