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

Allow loading plugins from either scratchabit dir or working directory #7

Closed
wants to merge 1 commit into from
Closed

Conversation

projectgus
Copy link
Contributor

Hi Paul,

Small change I found useful when working outside the scratchabit directory itself.

Cheers,

Angus

@pfalcon
Copy link
Owner

pfalcon commented Jul 23, 2015

So, this patch tries to add loading plugins from a directory where scratchabit.py resides. Is that what you call "working directory"? I wouldn't call it such ;-). And funny thing, Python already should support that. I don't know how that works in presence of symlinks though. So, let's approach it from the other end - please describe the setup you have in mind, let's discuss whether it's the best setup, and then see how to support it. (Because loading executable stuff from arbitrary (or just too many) places is not source of confusion and security issues.)

@projectgus
Copy link
Contributor Author

Hi Paul,

By working directory I mean current working directory, os.getcwd().

What I'm trying to do: I'd like to keep my plugins dir in the same dir I'm using to keep scratchabit.py, then put that directory on my PATH. Then run scratchabit.py from anywhere on my system and have it load plugins correctly.

At the moment I find I need to keep my .def files in the same directory as scratchabit.

The patch I sent tries to keep the current behaviour as well (which I saw as finding plugins under cwd), so it looks in both places. The set(), abspath(), stuff is to remove duplicates - so not really necessary. It would be a much simpler patch without those two things.

Angus

-------- Original Message --------
From: Paul Sokolovsky notifications@github.com
Sent: 23 July 2015 6:02:39 pm AEST
To: pfalcon/ScratchABit ScratchABit@noreply.github.com
Cc: Angus Gratton gus@projectgus.com
Subject: Re: [ScratchABit] Allow loading plugins from either scratchabit dir or working directory (#7)

So, this patch tries to add loading plugins from a directory where scratchabit.py resides. Is that what you call "working directory"? I wouldn't call it such ;-). And funny thing, Python already should support that. I don't know how that works in presence of symlinks though. So, let's approach it from the other end - please describe the setup you have in mind, let's discuss whether it's the best setup, and then see how to support it. (Because loading executable stuff from arbitrary (or just too many) places is not source of confusion and security issues.)


Reply to this email directly or view it on GitHub:

#7 (comment)

Sent from my Android phone with K-9 Mail. Please excuse my brevity.

@pfalcon
Copy link
Owner

pfalcon commented Jul 23, 2015

Ok, so here's my userstory:

  1. A user checks out ScratchABit repository to any directory they like, there can't be univeral conventions/recommendations.
  2. To work with it in standard (no-litter-in-random-dirs) way, scratchabit.py needs to be accessible from PATH.
  3. A careful user already has one's "personal" bin dir (usually ~/bin/), and is reluctant to add more stuff to PATH.
  4. Consequently, a user makes a symlink to repo checkout's scratchabit.py from ~/bin/scratchabit.py .
  5. User keeps plugins in standard locations, which is either system-wide plugins/ subdir(s) of SAB checkout, or just any directory on PYTHONPATH (configured per user's likes and knowledge).
  6. Then user can have "project dirs" anywhere on filesystem, and everything works as expected.
  7. At this time, there's no per-project plugin support, as that doesn't scale in terms of management, usability, and security.

@pfalcon
Copy link
Owner

pfalcon commented Jul 23, 2015

By working directory I mean current working directory, os.getcwd().

module.__file__ in Python refers to location of module file itself (and even symlinks are resolved, per above). The only way module.__file__ can same as os.getcwd() is when module is in current dir. As we talk about scratchabit.py, that would mean it's copied into current (i.e. particular project's) dir. I obviously don't endorse such copying. Nor I can endorse loading plugins from a random current dir, that's as helpful as telling user that they have to have PATH=. to work with some app. They can if they want - on their own, forcing them to have that is against any logic.

Python's analog of PATH is PYTHONPATH, and you can have the same behavior with setting that in environment. E.g., if you have scratchabit.py (somewhere else) in your PATH, and want to load plugins from current dir, run in it as:

PYTHONPATH=. scratchabit.py some.def

No patching necessary. Not endorsed. Used at your own risk.

@projectgus
Copy link
Contributor Author

I don't think I explained the reason for the pull request well enough.

The behaviour of scratchabit without this patch is that any directory named 'plugins' has to be relative to cwd (or the plugin module has to live elsewhere on the python path to be found). At least that's been my experience.

The only reason the original version of the patch supported looking for a plugins dir inside cwd is that I didn't want to break the existing behaviour, in case you wanted it that way.

The user story you outlined matches my use case perfectly. I just pushed a new version of the patch, which only supports that use case.

@pfalcon
Copy link
Owner

pfalcon commented Jul 23, 2015

The behaviour of scratchabit without this patch is that any directory named 'plugins' has to be relative to cwd

Right, now I got it. Yes, that's bug.

# plugin dirs found in same directory as scratchabit
plugin_dirs = [ "plugins", "plugins/cpu", "plugins/loader" ]
for d in plugin_dirs:
sys.path.append(os.path.join(sys.path[0], d))
Copy link
Owner

Choose a reason for hiding this comment

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

Using sys.path[0] looks a bit hacky, but __file__ isn't resolved for symlinks, so it would need to be done manually, which is boring. So, ok ;-).

@pfalcon
Copy link
Owner

pfalcon commented Jul 23, 2015

Merged with minor changes, thanks!

@pfalcon pfalcon closed this Jul 23, 2015
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