-
Notifications
You must be signed in to change notification settings - Fork 236
Used env vars to search for packages in PYTHONPATH #1
Conversation
711c031
to
8fde007
Compare
differs/pipDiff.go
Outdated
for _, pythonPath := range pythonPaths { | ||
contents, err := ioutil.ReadDir(pythonPath) | ||
if err != nil { | ||
// layer's Python folder doesn't have a site-packages folder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this no longer true in your implementation or did you just find the comment superfluous?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh hmm, you're right, if I'm preemptively building those "site-packages" paths without knowing for sure if they exist, I should just skip over them if they don't exist... I mostly deleted it because it referenced layers and presumably, there should be no layers once we address #11
|
||
func (d PipDiffer) getPackages(path string) (map[string]map[string]utils.PackageInfo, error) { | ||
packages := make(map[string]map[string]utils.PackageInfo) | ||
func getPythonPaths(vars []string) []string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, is every path where python packages could be installed included in the environment variables? Or is it only the path set on exit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean the path set on exit?
But also, now that you mention it, I've been obtaining packages from the PYTHONPATH in addition to the default "site-packages" path, but perhaps this is wrong... would no packages be installed in that default location if the user explicitly sets their PYTHONPATH?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern was that if a user changes their PYTHONPATH many different times and installs things in many different places, would we be finding all of those packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I believe this would be the PYTHONPATH set at the time the container is exported into an image, so yes, if it's been overwritten and not merely added to while the container is running, you wouldn't have all the old ones... @nkubala thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's safe to assume that all directories where packages are installed will be on the PYTHONPATH
. If they're not, then Python won't know how to find them, and in that case they're basically as good as uninstalled from our point of view. Usually, when you install packages to a different directory, you append it to your path, not reset it.
.gofmt.sh
Outdated
|
||
set -e | ||
|
||
files=$(find . -name "*.go" | grep -v iDiff/vendor | grep -v vendor/ | xargs gofmt -l -s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove grep -v iDiff/vendor
120185c
to
fa1fd61
Compare
#3 @aaron-prindle @nkubala @abbytiz