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

declare -path & -lib flags now search user search paths #205

Closed
wants to merge 12 commits into from

Conversation

danomatika
Copy link
Contributor

@danomatika danomatika commented Aug 21, 2017

This PR adds adds st_searchpath checking for the [declare] -path & -lib flags. This provides similar functionality to how -stdpath & -stdlib work, but searches one level into st_searchpath instead of st_staticpath.

I'm not sure if this is a good fix or opens a host of new problems. It was, however, easy to implement (mostly via copy/paste/edit), so here you go.

EDIT: This update favors relative paths by first checking for a preceding "./" and then checking if a subdirectory of the canvas dir exists or the library was loaded. It then falls back to checking st_searchpath. If the path/lib is an explicit relative path (starts with "./"), the st_searchpath checks are omitted.

This is in response to #184 and would negate the need for deken to suggest adding to search paths.

@danomatika
Copy link
Contributor Author

Just tested this with Gem installed via deken to ~/Documents/Pd/externals and this path added to the user search paths. Using [declare -lib Gem], I can create Gem objects.

@porres
Copy link
Contributor

porres commented Aug 21, 2017

Just tested this with Gem installed via deken to ~/Documents/Pd/externals and this path added to the user search paths. Using [declare -lib Gem], I can create Gem objects.

Hi, that is not a good test, as this was already happening before! Even if the path for Gem is not added - as in ~/Documents/Pd/externals/Gem - but the parent folder is ( ~/Documents/Pd/externals), calling [declare -lib Gem] would look for a Gem binary in the user added paths... Now, I know the binary is not available in that folder, but in the case the binary has the same name as the folder, voilà... it works!!! So it finds Gem, and it used to find before.

So I don't think the changes may have affected it or made it happen. It's good to test though so we can see at least it didn't ruin it :)


Now, for an actual test... I did it, and it WORKS!!! :D

What we needed to test is the -path flag indeed. So now I just have ~/Documents/Pd/externals/ and not the cyclone path added, which is in a subfolder to that path. By now using [declare -path cyclone] I can call the externals without the name prefix ;)

Hurray....

screen shot 2017-08-21 at 20 46 13

@porres
Copy link
Contributor

porres commented Aug 21, 2017

Well, just to be clear, that fixes issues I was raising in this old and long discussion #139

@porres
Copy link
Contributor

porres commented Aug 22, 2017

Hi, that is not a good test, as this was already happening before! Even if the path for Gem is not added - as in ~/Documents/Pd/externals/Gem - but the parent folder is ( ~/Documents/Pd/externals), callinf [declare -lib Gem] would look for a Gem binary in the user added paths... Now, I know the binary is not available in that folder, but in the case the binary has the same name as the folder, voilà... it works!!! So it finds Gem, and it used to find before.

Maybe this was all unintentional. Seems that the case with -lib is that it actually searches anywhere! So I'm testing it and finding that if you have Gem or whatever in a standard path and use [declare -lib Gem] instead of [declare -stdlib Gem], it still finds the thing!

This seems wrong and is conflicting with the whole point of having "-stdlib" and just "-lib". So I propose we also fix this and prevent -lib from searching the 'standard paths'

@umlaeute
Copy link
Contributor

i'd argue to the opposite: -stdlib vs -lib is just confusing everybody; so rather than prevent -lib from searching the 'standard paths', we should question whether the concept of standard paths should be exposed at the patch level at all.

@danomatika
Copy link
Contributor Author

danomatika commented Aug 22, 2017

That's the thing: what behavior do we want? I think I've proven we can add behavior to the -path & -lib flags, but does this make sense so far?

In some ways, I think it's reasonable to allow both deep search and shallow searching, which is why the check for "./" simply adds the paths as before without looking deeper. After a certain point, there can always be conflicts and that is on the user. We should decide where that point lies.

Another point of reference, is how the Lua require command looks for Lua files or libraries: https://www.lua.org/pil/8.1.html

EDIT: Of course, I am not proposing changes which break current behavior or compatibility.

@danomatika
Copy link
Contributor Author

We should question whether the concept of standard paths should be exposed at the patch level at all.

Right. Falling back to "extra" relies too much on the Pd-extended model. A smarter -path and -lib could help in this regard.

@porres
Copy link
Contributor

porres commented Aug 22, 2017

i'd argue to the opposite: -stdlib vs -lib is just confusing everybody; so rather than prevent -lib from searching the 'standard paths', we should question whether the concept of standard paths should be exposed at the patch level at all.

I'd say it's probably unnecessarily complicated having both kinds of flags aroung, for standard or not. I could live with a simpler -path/-lib flag that would search everywhere (in extra or not). But what are we suggesting? Getting rid of -stdpath/-stdlib? I, for one, would be totally fine with it... but I know this has 0 chance to be done for concerns on backwards compatibility and all... so what are we talking about?

Oh... maybe you mean -path and -lib look EVERYWHERE, whereas -stdpath and -stdlib does a restrictive search.

Hmmm, that would make sense...

@porres
Copy link
Contributor

porres commented Aug 22, 2017

funny thing about "-lib" is that it's basically unnecessary, as you should be able to load the libraries by calling the external binary. Like, creating the [zexy] external loads the library. And if it is in the extra folder, it's automatically searched for... same if it's in the relative patch directory, or just any path that is added. So I can only see an use for "path" flags :/

@umlaeute
Copy link
Contributor

sigh.

while this is technically true, [declare -lib foo] guarantees that the library is loaded when the patch is opened, whereas [foo] only guarantees that the library is loaded once the object is instantiated.
also, there are a number of libraries that do not provide a boilerplate object, in which case the library loads but the object fails to instantiate.

but you are of course free to just not use [declare] if that fits your bill.

@porres
Copy link
Contributor

porres commented Aug 22, 2017

well, good to know, I'm gonna document this in an update of my manual on how to load and manage externals.

Now, what exactly do you guys mean with -path/-lib vs -stdpath/-stdlib? Am I right to assume the idea is that -stdpath/-stdlib would be restrictive while the others should look everywhere?

@danomatika
Copy link
Contributor Author

whereas -stdpath and -stdlib does a restrictive search

It already does.

@porres
Copy link
Contributor

porres commented Aug 22, 2017

It already does.

I know, but -path still does not search inside -stdpath, while -lib does search in -stdlib. Seems the original design was explicitly not allowing -lib to search in -stdlib, but it does, so I raised that as a bug, asking that -lib wouldn't search in -stdlib. Now, I'm not 100% sure what you guys are proposing. Is it that both -path and -lib would also search in -stdpath and -stdlib?

If yes, cool, I like the idea, and all we need to do is make sure -path also searches inside -stdpath

@danomatika
Copy link
Contributor Author

danomatika commented Aug 22, 2017

Seems the original design was explicitly not allowing -lib to search in -stdlib, but it does, so I raised that as a bug, asking that -lib wouldn't search in -stdlib.

Yes, IMO it should not. I would see that as a bug.

Now, I'm not 100% sure what you guys are proposing.

I don't know either as you're often confusing me by (seemingly) going in circles.

Is it that both -path and -lib would also search in -stdpath and -stdlib?

No.

If yes, cool, I like the idea, and all we need to do is make sure -path also searches inside -stdpath.

What? No. We are trying to avoid people installing stuff to "extra", not encourage it, AFAICT.

@porres
Copy link
Contributor

porres commented Aug 22, 2017

I don't know either as you're often confusing me by (seemingly) going in circles.

well, I see I'm repeating the same question until I have clear answer

What? No. We are trying to avoid people installing stuff to "extra", not encourage it, AFAICT.

ok, fine by me, but I'm still confused because of this quote...

i'd argue to the opposite: -stdlib vs -lib is just confusing everybody; so rather than prevent -lib from searching the 'standard paths', we should question whether the concept of standard paths should be exposed at the patch level at all.

see, that's what's troubling me, this seems to be an argument that -lib should in fact search in the same place as -stdlib... because I raised the bug and said -lib shouldn't search in it, and this says "i'd argue to the opposite (...) rather than prevent -lib from searching the 'standard paths', we should ..."

I'm fine either way, make -path and -lib search in the same place as -stdpath or -stdlib, or not, I don't care. I just wanna clearly understand where we're going. All I know and care is that now we clearly have a conflicting behaviour with -lib searching in the same place as -stdlib, and we need to sort this one way or the other.

cheers

@danomatika
Copy link
Contributor Author

danomatika commented Aug 22, 2017

It should work as it's described in the help patch:

  • -path: adds a path to the patch's search path relative to patch (and looks in user search paths if nothing is found with my addition)
  • -lib: tries to load a lib relative to the patch (and looks in user search paths if nothing is found with my addition)
  • -stdpath: adds a path to the patch's search path relative to Pd (aka extra)
  • -lib: tries to load a lib relative to Pd (aka extra)

From looking in the code, the behavior above is what we have. I think the issue comes from the sys_loadlib() mechanism which, once a library is loaded, will keep allowing instantiation for objects from that library even if there is no [declare] in the patch. I do not think we can change this (or should).

@danomatika
Copy link
Contributor Author

danomatika commented Aug 22, 2017

Maybe this was all unintentional. Seems that the case with -lib is that it actually searches anywhere! So I'm testing it and finding that if you have Gem or whatever in a standard path and use [declare -lib Gem] instead of [declare -stdlib Gem], it still finds the thing!

Can you test this again, but close Pd, then load the patch with [declare -lib]? As stated above, if you load the patch with [declare -stdlib] and cyclone is loaded, it's basically within Pd and objects will always be created afterwards, as far as I know.

@porres
Copy link
Contributor

porres commented Aug 22, 2017

well, in any case, I opened a new issue about it in #206 - to keep things more objective and focused, so we can treat that separately

@porres
Copy link
Contributor

porres commented Aug 22, 2017

Can you test this again, but close Pd, then laid the patch with [declare -lib]?

I've tested it many times, here's a screenshot from a new test, from scratch...

screen shot 2017-08-22 at 13 36 59

@Ant1r
Copy link
Contributor

Ant1r commented Sep 2, 2017

So you're suggesting -path and -lib should always end up looking relative to Pd as well if all else fails?

I also think that it would be a valuable simplification:

  • 1: search relatively to the calling patch
  • 2: search the user's global paths
  • 3: search Pd's system paths

@danomatika
Copy link
Contributor Author

danomatika commented Sep 2, 2017 via email

@danomatika
Copy link
Contributor Author

Ok, st_staticpath is now searched after all else when using [declare -path].

@danomatika danomatika added the bug/fix either a bug (for issues) or a bugfix (for pull-requests) label Nov 30, 2017
@porres
Copy link
Contributor

porres commented Mar 8, 2018

Ok, st_staticpath is now searched after all else when using [declare -path].

I tested it, and it works!

Let us just remember to update the help file of [declare] when this gets merged. As I see it, the text should now say:

-path add to search path, relative to Pd, to the patch or to the user paths
-stdpath add to search path, but only relative to Pd
-lib load a library, relative to Pd, to the patch or to the user paths
-stdlib load a library, but only relative to Pd

And by "relative to Pd", it should say somewhere that this means the "extra" folder.

But then, currently Pd has additional Standard Paths, which I'm suggesting we just get rid of in here #321

@danomatika
Copy link
Contributor Author

Before you write more and more covering the same thing, why not change the help file and either paste it here or make a PR to this branch: feature/declare-path. I honestly can't follow this anymore even if it makes sense to you.

@porres
Copy link
Contributor

porres commented Mar 9, 2018

Made the Pull Request to feature/declare-path to update the help file, which only reflects what was discussed and made here. Sorry, didn't want to impose that so I just suggested first.

What could be still confusing about this Pull Request to master is the name, instead of "declare -path & -lib flags now search user search paths" we could say "declare -path & -lib flags now search user search paths and standard path", cause that's where we ended up and it helps making sense ;)

cheers

@umlaeute umlaeute added this to the 0.49 milestone Sep 3, 2018
@danomatika
Copy link
Contributor Author

This branch is now up to date.

src/g_canvas.c Outdated Show resolved Hide resolved
@danomatika
Copy link
Contributor Author

This branch is now up to date.

@danomatika
Copy link
Contributor Author

In looking at the commits on the master branch, it looks like some of this is already there? It's hard to tell what is merged if this PR is still open. If we're doing a running/manual merge, it might be good to add a post here, otherwise I might still be working on this branch while it's possibly being worked on in master upstream...

@danomatika
Copy link
Contributor Author

As this is basically in master, I'm closing this PR.

@danomatika danomatika closed this Sep 10, 2018
@umlaeute
Copy link
Contributor

umlaeute commented Oct 2, 2018

can we also remove the feature/declare-path branch?

@danomatika
Copy link
Contributor Author

danomatika commented Oct 3, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/fix either a bug (for issues) or a bugfix (for pull-requests) feature suggestion for an enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants