sandboxes and composer installations #152

Markcial · 2015-02-16T22:08:29Z

If you want to test only a composer library without storing it, you can call the sandbox command, so when the shell closes it removes all in the end, if the composer command is called without sandbox the files are kept.

bobthecow · 2015-02-17T00:09:58Z

Looks good :)

bobthecow · 2015-02-17T00:10:09Z

src/Psy/Command/SandboxCommand.php

+     */
+    protected function getSandboxPath($name)
+    {
+        return sys_get_temp_dir() . DIRECTORY_SEPARATOR . self::SANDBOX_FOLDER . DIRECTORY_SEPARATOR . $name;


Let's use Configuration::getRuntimeDir instead of sys_get_temp_dir for the base directory.

bobthecow · 2015-02-17T01:11:52Z

The sandbox command (I like the name, btw) is a bit unwieldy. I think it would probably make sense to split out subcommands and run them from the parent command, a bit like what composer global does:

https://github.com/composer/composer/blob/master/src/Composer/Command/GlobalCommand.php#L53-L81

bobthecow · 2015-02-17T01:13:40Z

For the sandbox subcommands:

sandbox create (alias: add)
sandbox create bananas
sandbox list
sandbox list --grep nana
sandbox which
sandbox use bananas (alias: switch)
sandbox clear
sandbox clear foo
sandbox delete
sandbox delete foo
sandbox exit (not sure about this one. maybe reset?)

bobthecow · 2015-02-17T01:19:31Z

Additionally, I feel like there's room for proxying more than just composer's require...

browse, clear-cache, depends, dump-autoload, home, info, remove, require, run-script, and search all seem like they could at least be useful inside PsySH, if not 100% essential to have :)

So given that, it might make sense to have composer be a general command that proxies to (a whitelisted subset of) composer calls.

If we go that route, I'd make composer be the command for dealing with the local project, and add some of the composer subcommands to sandbox:

sandbox require mustache/mustache
sandbox remove mustache/mustache

That would make the two commands (somewhat) analogous, but have a clear distinction between "this is going to be used for temporary stuff" vs "this is becoming a project dependency".

bobthecow · 2015-02-17T01:25:33Z

src/Psy/Command/SandboxCommand.php

+    {
+        $name = !is_null($name) ? $name : uniqid();
+        $path = $this->getSandboxPath($name);
+        if (file_exists($path)) {


this will need to be @file_exists because PHP triggers errors if you check a file outside of open_basedir restrictions.

Markcial · 2015-02-17T10:15:22Z

About this comment #152 (comment), it would make sense if we has a base library for sandbox, i tried to make it all encapsed on a single command file. But if you think that it is necessary to have a wider range of use for the functions it can be moved as a service.

And about this one, #152 (comment) i will try to make some experiments, because the proc_open way i solved the composer related thingies maybe is not the best solution. Because of different systems like windows, and because the parameters as you said for sandbox. I will post the updated PR if i get with something interesting.

Markcial · 2015-02-17T12:14:47Z

Hi there! i finally managed to add a lot more potential to the composer shell, i include the phar file and create a composer application that receives the composer calls, in other words, right now the composer shell is freaking awesome, it accepts completely all the syntax that it uses via CLI, i still have to create the methods for, sandbox require and sandbox remove, but the version we have right now is pretty decent IMHO. 😎

Taluu · 2015-02-17T12:22:09Z

src/Psy/Command/ComposerCommand.php

+        }
+
+        return false;
+    }


why don't you use Symfony's Process component instead ?

I will check for that, didn't knew that symfony had one.

https://github.com/symfony/Process :)

with this, you do not have to bother anymore with the proc_open functions, unix, windows, etc

Ok, i will check that, maybe today i will update the PR

bobthecow · 2015-02-17T15:26:54Z

If composer install fails, we could tell the user to just download the phar themselves and put it in the current directory :)

bobthecow · 2015-02-17T15:29:01Z

(not at a computer now so I can't check this myself, but does composer install let you specify an install directory? If so, it might make sense for us to install it in one of the psysh config or data directories.

Taluu · 2015-02-17T15:29:49Z

On the CLI, yes, through a -d argument

bobthecow · 2015-02-17T15:37:00Z

The composer sub-application approach is nice. My one concern would be that composer might not have as flexible dependency version requirements as we do. That's fine for them, because they ship as a phar, but for us, we have to be pretty forgiving, since we often run inside other symfony apps. So if the composer phar expects one version of a symfony component, and the current project (and thus psysh) is stuck on a different one, there is no real way to enforce the dependency versions and make sure things stay sane.

bobthecow · 2015-02-17T15:39:49Z

@Taluu things that start with -- are options not arguments, so they're eaten by the original command. This is how it needs to be done. See composer global, they have to do it there too.

Taluu · 2015-02-17T15:39:49Z

Then maybe we should discard composer's cli, and require it directly in the json file ? And then be able to use directly composer from its source ?

bobthecow · 2015-02-17T15:41:00Z

But then psysh becomes dependant on all of composer, and all of their dependencies. And psysh's phar becomes huge and bloated.

Taluu · 2015-02-17T15:41:00Z

OK now I get why the input is kinda reparsed... Too bad :(

Taluu · 2015-02-17T15:41:23Z

@bobthecow yes, that's also what I thought

Markcial · 2015-02-17T15:41:50Z

@bobthecow Yes you can specify the path to the phar file, if it is not specified it downloads the phar locally via a process through php, which might surely be installed because of all the code is made in php obviously.

@Taluu The CLI has nothing to do with this, i include the phar as a library, so i am not 100% sure that setting the phar path via the CLI call will affect to the command.

About the composer dependencies, if the end user is concerned about this, he can set the phar path in the config as you said, or maybe like @Taluu said, include the library in the composer.json file. but having it as a dependency for the library only keeps the library up to date whenever the user runs update on the psysh project, but if the user sets the configuration it will use the selfupdated phar, well so far, i guess that a configured path to the phar might be ok, my point.

bobthecow · 2015-02-17T15:42:42Z

We can't make psysh depend on something like composer for just one command, which many users will likely never even need. That's just too much.

GrahamCampbell · 2015-02-17T15:43:21Z

👎 for dependence on composer. I don't want every laravel app to be download composer...

Markcial · 2015-02-17T15:45:37Z

maybe the composer folks can provide a way to do a "discovery mode" for the composer.phar, so 3rd parties like psysh could ask something like : composer phar-location, this way the calls will be pretty straightforward.

bobthecow · 2015-02-17T15:46:53Z

It won't, I promise :)

I don't take adding dependencies lightly, since every one adds a potential for conflict and incompatibility with a user project.

bobthecow · 2015-02-17T15:48:32Z

Also, every psysh dependency has to be shoehorned into the phar, and that doesn't need to be any bigger than it has to be :)

Markcial · 2015-02-17T15:51:41Z

@bobthecow Ok to the process dependency from symfony or should i rollback to the proc_open solution?

bobthecow · 2015-02-17T16:07:01Z

@Markcial leave it for now, and I'll look into it. My rule of thumb is that if it's just a convenience wrapper, I leave it out and handle the inconvenience myself. If the dependency does something material, and we use a decent amount of the library's features, and the library doesn't introduce onerous dependencies or version requirements, then it's probably okay. Symfony components usually meet most of these criteria though.

bobthecow · 2015-02-23T18:58:29Z

The Symfony Process component is 3200 lines of code. Only 1400 if you exclude the comments. To put that in perspective, all of PsySH is 6000 lines of code :)

Markcial · 2015-02-24T14:09:56Z

i rolled back to the previous command without the symfony process dependency.

Taluu · 2015-02-24T14:45:11Z

I think this is really a wrong argument against the process. Specially if you base yourself on the number of code lines, specially as the process component does all the needed abstraction on linux, mac, windows or whatever (it is tested against those environment, etc, etc)

BTW, the "1400 lines with the comments", you also take into account the lines which are not used (such as the PhpProcess, the ProcessBuilder, ...). All in all, with the comments and if you use the thing (because you won't load it until necessary), it is only a matter of 1000 lines (with comments) tops.

I really think this is a bullshit argument, which is clearly a NIH syndrome. And also because this feature will need some maintainance... Good luck with that.

I'm still gonna use the feature if it is released (I suggested it after all), but meh, you now know my point of view.

…to the composer shell

…deferral for process symfony library

Markcial · 2015-02-24T15:10:29Z

the proc_open is supposed to be a little help, not a core feature. We do not rely on it that heavily.

Taluu · 2015-02-24T15:21:10Z

Just to run the composer install, which is I think the whole concept behind that feature....

Markcial · 2015-02-24T15:23:47Z

if you already have it installed, which might be the 90% of the people using it, you only need to set the parameter in the config and it will use that phar instead.

Taluu · 2015-02-24T15:28:59Z

Fair enough, just saw where and why the Proc_open is called. But I still think the LOC argument is not the right one. And that the NIH syndrome should be avoided, even if it's for something minor such as the composer installation

bobthecow · 2015-02-24T16:04:00Z

Except all the lines get bundled into the psysh phar, so at some level they do matter.

Lines of code is not the only reason for using (or not using) a package. It's just one data point in weighing whether to add another dependency. But they are indicative of the relative complexity and feature set of this package. Since those 1500 lines of code are replacing approximately 5 lines of code that we write ourselves, it seems prudent to actually consider them rather than just blindly tacking on another dependency.

Dependencies themselves aren't bad, per se. But they definitely aren't without cost, either. Every dependency we add, and every version constraint we require, excludes some number of potential users. Because in real life, not every project keeps up with the latest releases of all their dependencies. In the interest of not adding to the inevitable dependency hell, I prefer to weigh each one on its merits before deciding to accept its cost.

Additionally, each dependency adds some amount of work for the maintainers of this package. SemVer helps mitigate this cost, but Symfony doesn't actually follow SemVer. So every release of every Symfony component adds the potential for BC breaks and a drain on the contributors of this library and all projects that depend on it.

Note that this isn't theoretical. It's actually a problem, and actually happens in real life. See the Table(Helper) BC break we had to work around in the latest release.

So is it worth adding this dependency? Maybe.

But it has to bring something material to the table. And I'm not convinced this particular dependency does. For our needs. For this feature. At this time. All of those factors may change, and I'm happy to revisit this decision.

Since you weren't in my head during the process of weighing the options, I'll give you the benefit of the doubt.

For the record, though, what's actually bullshit is getting worked up over how an open source project comes to a different conclusion than you when weighing costs and benefits of a decision such as this.

You're still gonna use the feature, even though it might be implemented in a way that you disagree with, despite the fact that those implementation details are just that, implementation details, and you won't even be able to tell the difference between the two when using the product? Despite the fact that the project was given to you for free, and then you asked for another feature and someone else took their own time to write it and give that to you for free as well? How gracious of you.

Don't be a dick about open source.

Taluu · 2015-02-24T16:16:52Z

Let me then answer on some points. :)

Lines of code is not the only reason for using (or not using) a package. It's just one data point in weighing whether to add another dependency. But they are indicative of the relative complexity and feature set of this package. Since those 1500 lines of code are replacing approximately 5 lines of code that we write ourselves, it seems prudent to actually consider them rather than just blindly tacking on another dependency.

Dependencies themselves aren't bad, per se. But they definitely aren't without cost, either. Every dependency we add, and every version constraint we require, excludes some number of potential users. Because in real life, not every project keeps up with the latest releases of all their dependencies. In the interest of not adding to the inevitable dependency hell, I prefer to weigh each one on its merits before deciding to accept its cost.

Fair enough, I'm not really developping anything on PHARs (...just using them), so you may have a point here. As I already said earlier, I saw that the proc_open is not used on a critical path for this feature, so "meh, fine".

Additionally, each dependency adds some amount of work for the maintainers of this package. SemVer helps mitigate this cost, but Symfony doesn't actually follow SemVer. So every release of every Symfony component adds the potential for BC breaks and a drain on the contributors of this library and all projects that depend on it.

Note that this isn't theoretical. It's actually a problem, and actually happens in real life. See the Table(Helper) BC break we had to work around in the latest release.

It does actually follow semver, since the 2.3. The tablehelper you're quoting, for example, is only deprecated till the 3.0... they removed the deprecation notice because it was bothering everybody else, which is understandable. But on a general point of view, yes, it is also understandable to be careful against library that does not follow exactly the semver standard. :)

You're still gonna use the feature, even though it might be implemented in a way that you disagree with, despite the fact that those implementation details are just that, implementation details, and you won't even be able to tell the difference between the two when using the product? Despite the fact that the project was given to you for free, and then you asked for another feature and someone else took their own time to write it and give that to you for free as well? How gracious of you.

Actually, even though it is not implemented in a way I do agree with, I still think the project worthwhile to be used, and I'm even thankful (truly !) that there is something that can ease my development workflow.

It was just that I had the impression that the LOC argument was the only thing taken into account (and not all the dependency stuff, which means at each release you must indeed be careful, yadi yadi yada), which is AFAIC not enough for me something to agree with. But, with the other arguments you just enounced right there, I am really fine with those.

BTW, I wasn't talking about this feature en particular (I wanted to do it myself to contribute but meh, @Markcial was faster :D), but I was really just suprised on the sole argument before the whole speech.

If I offended you (or @Markcial), then I'm really sorry, and again, thanks for the work you invested in this application. :)

Markcial · 2015-02-24T16:24:17Z

Not offended, but saying thanks is nicer than the comments before. 😃

bobthecow · 2015-02-24T18:57:31Z

I'm not offended. I just don't think it's appropriate to direct abuse at open source projects or contributors.

Given the context of the entire thread, e.g.:

I don't take adding dependencies lightly, since every one adds a potential for conflict and incompatibility with a user project.

… and:

Also, every psysh dependency has to be shoehorned into the phar, and that doesn't need to be any bigger than it has to be :)

… and:

My rule of thumb is that if it's just a convenience wrapper, I leave it out and handle the inconvenience myself. If the dependency does something material, and we use a decent amount of the library's features, and the library doesn't introduce onerous dependencies or version requirements, then it's probably okay.

… my comment was shorthand for "This is a relatively big library, of which we're using a very very small part. We don't really need it, since this is a fairly insignificant sub-feature of a feature — which approximately 0% of our users will need. So it would probably be best if we didn't force everyone else take on the overhead and potential headache of the additional dependency."

I thought it was clear from the context, but it obviously wasn't. If you'd asked, I would have pointed these things out :)

Taluu · 2015-02-24T22:27:21Z

Once again, if I seemed rash, I'm sorry, as this was far from my intentions (I do have to fix that).

But then again, maybe thinking of a solution to be able to register plugins would be great, so that anybody could actually register its own command (or whatever) with its own dependencies would be great... Should I open a ticket ? Or I could have missed something in Psysh which already allows that...

bobthecow · 2015-02-25T01:06:38Z

I have been thinking about extensibility, but I haven't come up with anything I love yet. Open an issue and let's talk :)

Taluu · 2015-02-26T10:36:42Z

done : #166

castarco · 2017-05-20T20:24:16Z

Hi, what's the state of this PR? I think that reading the complete thread is not enough to completely understand the current situation.

I'm interested on this feature since I'm maintaining the PHP kernel for Jupyter and packages isolation is very interesting for this usage case.

Best regards.

bobthecow reviewed Feb 17, 2015
View reviewed changes

Markcial mentioned this pull request Feb 17, 2015

Load dynamically a composer dependency #146

Open

Taluu reviewed Feb 17, 2015
View reviewed changes

Markcial force-pushed the sandboxed_composer branch from 132e18a to 72ea2e6 Compare February 17, 2015 13:56

Markcial added 4 commits February 24, 2015 16:01

sandboxes and composer installations

869aa5a

silenced some verbose functions, code cleaning, added more functions …

e3dc466

…to the composer shell

refactoring of the arguments for the commands, replaced proc_open in …

dbf3d7c

…deferral for process symfony library

removed dependency on symfony process, replaced with proc_open function

74732a1

Markcial force-pushed the sandboxed_composer branch from 0082ced to 74732a1 Compare February 24, 2015 15:08

Base automatically changed from master to main January 17, 2021 15:49

sandboxes and composer installations #152

Are you sure you want to change the base?

sandboxes and composer installations #152

Conversation

Markcial commented Feb 16, 2015

bobthecow commented Feb 17, 2015

bobthecow Feb 17, 2015

Choose a reason for hiding this comment

bobthecow commented Feb 17, 2015

bobthecow commented Feb 17, 2015

bobthecow commented Feb 17, 2015

bobthecow Feb 17, 2015

Choose a reason for hiding this comment

Markcial commented Feb 17, 2015

Markcial commented Feb 17, 2015

Taluu Feb 17, 2015

Choose a reason for hiding this comment

Markcial Feb 17, 2015

Choose a reason for hiding this comment

Taluu Feb 17, 2015

Choose a reason for hiding this comment

Markcial Feb 17, 2015

Choose a reason for hiding this comment

bobthecow commented Feb 17, 2015

bobthecow commented Feb 17, 2015

Taluu commented Feb 17, 2015

bobthecow commented Feb 17, 2015

bobthecow commented Feb 17, 2015

Taluu commented Feb 17, 2015

bobthecow commented Feb 17, 2015

Taluu commented Feb 17, 2015

Taluu commented Feb 17, 2015

Markcial commented Feb 17, 2015

bobthecow commented Feb 17, 2015

GrahamCampbell commented Feb 17, 2015

Markcial commented Feb 17, 2015

bobthecow commented Feb 17, 2015

bobthecow commented Feb 17, 2015

Markcial commented Feb 17, 2015

bobthecow commented Feb 17, 2015

bobthecow commented Feb 23, 2015

Markcial commented Feb 24, 2015

Taluu commented Feb 24, 2015

Markcial commented Feb 24, 2015

Taluu commented Feb 24, 2015

Markcial commented Feb 24, 2015

Taluu commented Feb 24, 2015

bobthecow commented Feb 24, 2015

Taluu commented Feb 24, 2015

Markcial commented Feb 24, 2015

bobthecow commented Feb 24, 2015

Taluu commented Feb 24, 2015

bobthecow commented Feb 25, 2015

Taluu commented Feb 26, 2015

castarco commented May 20, 2017