- User Since
- Dec 28 2011, 1:51 PM (622 w, 6 d)
Jan 8 2018
Alright! You know where to find me or this patch if interest arises :)
@epriestley is the best course of action for this diff to abandon it?
Feb 14 2017
@chad - How can I be involved with any progress on this front then since now it is planned?
Jan 12 2017
I wasn't trying to create a whole new discussion. I respect your priorities and I also understand the engineers vs. money problem. The only major problem I see with the contribution guidelines is this: to be able to contribute more or affect change, you need clout and to get clout you need to contribute and for those contributions to go into the code or even get some feedback we need time from you guys.
@chad - You know how can you get more developers? Being more open to contributions :)
Jan 9 2017
Oct 21 2016
Oct 4 2016
I'd love to get this merged or concluded instead of trying to keep up with rebases and push it forward. I have been using this branch for quite a while locally and as far as I can tell, there are no problems.
Jul 25 2016
@eadler - oops, sorry about that then :) I'm still behind what I said though ;)
@eadler - I'd like to remind you that there are 3 patches waiting for review right now. They have been sitting there for a few months. That said we are not paying Phabricator. If the only way to get these patches which are already worked on and in pretty good shape merged in is to pay you or maintain a fork I'm really starting to think about crowdsourcing this on Kickstarter or Indiegogo to collect those few hundred bucks to get already working patches landed or simply create a fork and point people there loudly. I also think I almost qualify for "Rise to prominence" sub-section mentioned in https://secure.phabricator.com/book/phabcontrib/article/contributing_code/#alternatives but of course that also relies on you guys letting me contribute more. Making patches sit for months without any feedback and then making me rebase them multiple times is definitely not a great way to encourage people to "Rise to prominence" if you really value that.
Jun 23 2016
Jun 22 2016
I've decided to just drop the CMD.exe support since it is crazy and unreliable when it comes to escaping edge cases. Also it needs double escaping anyways (ArgV + Crazy-ass CMD escaping).
- Remove CMD.exe support and fix edge cases
Jun 21 2016
Jun 20 2016
If there's no way to escape it, we should make it throw, and have a test case to make sure it throws. It's OK if we don't handle it, I just want to have no cases where we silently do the wrong thing.
@epriestley if you are willing to give this another shot I'll try to fix the test error. Otherwise all my hard work seems to be going into a blackhole.
May 16 2016
Apr 29 2016
Apr 22 2016
Apr 15 2016
(I'm looking forward to reviewing this but probably won't get to it for at least a few days since I want to test it thoroughly myself.)
The flag was initially added to minimize the impact of fixing the broken streaming that Windows has by default. Without this flag on, you can't actually stream data from a process's standard output to PHP in realtime; you have to wait for the output to be fully buffered.
Anything I can do to move this one forward?
lgtm (but I haven't tested it)
- Actually add comment about taskkill change
- Safer PHP invocation for edge case test
@epriestley, @hach-que - I think this is in a very good state for a review now. The only thing I'm unsure is whether to enable bypass_shell by default since I don't see a reason to use the cmd.exe proxy. Its commands are not universal and it requires another layer of escaping which is confusing and probably prone to errors, especially when things get nested.
- Add comment about taskkill change
- Slightly better php invocations in tests
- Enable one more test for Windows
Apr 14 2016
Apr 13 2016
Okay, the patch was good. It was arc trying to overcompensate for Windows mania so another patch to arc is needed and it is on its way.
Apr 12 2016
I've noticed an issue with passthru mode. I'll investigate and update the patch accordingly.
I guess the author of the article I referenced is @dancol ?
- Fix broken test case
- Fix Windows escaping once and for all
- Fix test failures
- Add test for edge cases
Apr 11 2016
@epriestley - great test cases. I've already found a great article on MSDN which explains the escaping rules on Windows, both for CreateProcess and CMD.exe and implemented them. They seem to work well. I'm working fixing somewhat unrelated test failures due to stdin pipe getting clogged. I'll add these strings as test cases. Good news is, based on the information I got from the article, there doesn't seem any unescapable inputs.
@hach-que - Okay for the sake of progress I'm gonna leave the specifics of the argument or how these are parsed. I think you basically want me to come up with a "proper CMD escape" (has nothing to do with CreateProcess) instead of reusing PowerShell escape since that is not the correct way and are not asking me to make all instantiations of this class Windows-aware.
Apr 10 2016
But not everything is running through Powershell? When Arcanist runs (which is using this function), it escapes for the Command Prompt, not Powershell.
@hach-que - I strongly disagree since with that solution every single instance of the class needs to to this exact same thing. It doesn't make any sense to me. There's no point using the built in escaping function on Windows because it is broken beyond repair.
- Fix test failures
This should fix T10468.
Mar 11 2016
Mar 9 2016
Dec 31 2015
This is another reason for me to dislike PHP but I really appreciate the time you've taken to explain the issue in addition to a promising fix for this task. Couldn't have asked for a better new year's gift :)
Abandoning per T9724#151731
Oh wow, sorry. I just thought my system was broken and was hoping CI would
run tests. Will see if I can fix this.
Dec 30 2015
Dec 27 2015
If so, I'd generally claim this is an issue with the client you're using: I think clients generally should not do parameter validation themselves. They will always be less able to do validation than the server is, even though server validation is not especially sophisticated right now. If/when we have a formal Python API client, it is very unlikely to perform client validation. Does client validation provide any advantage?
Oct 8 2015
I'm fairly certain since it seems to be using the CreateProcess API as others do. I'm also using xonsh (the project using that Python code) for quite some time now and it works without any issues.
@epriestley - okay, may be the name mislead me. Anyways, I'm basically proposing making bypass_shell on by default in execx and implementing a mechanism that automatically finds the full path of the executable on Windows by implementing that Python function I referred to. Does this make sense?
How does that obsolete PhutilExecPassthru?
Okay, proposal: how about we implement the equivalent of this: https://github.com/scopatz/xonsh/blob/fa356d728b2852274f8aa0260dc7da3b7b0f08ab/xonsh/built_ins.py#L268
Oh god, and I thought Python's subprocess module was pretty bad.
bypass_shell (windows only): bypass cmd.exe shell when set to TRUE
5.2.1 Added the bypass_shell option to the other_options parameter.
Oh, thanks a lot for the pointers. I'll see how I can help with those.
- Fix some tests
This is a "does this really work?" sort of diff. Will make it proper after some feedback. Thanks! :)
Sep 15 2015
Thank you guys for the pointer and quick turn around!
- Do not cache workflow
Sep 11 2015
@epriestley - so are you saying I should simply revert rARC0e0957872083daa38309204ad9774568c9fcdd5c? I mean that looks like a nice patch and given all the context you and @avivey shared (thanks a ton btw for that), I may be able to prevent caching "properly".
Sep 10 2015
@avivey - When arc patch runs itself recursively. The thing I realized is arc patch first tries to get the patch without using credentials (but it still requires a conduit), and then when it realizes the request is rejected due to missing credentials, it manually adds them by calling authenticateConduit. After that it calls run, which in turn calls buildChildWorkflow. Since the parent buildChildWorkflow doesn't know about the already established conduit which has its credentials already set ($workflow->conduit being set) it simply tries to reset it causing $workflow->setConduitCredentials($this->conduitCredentials); call to throw the You may not set new credentials after authenticating conduit. error since credentials were already set manually by arc patch.
Sep 4 2015
Sep 1 2015
- don't skip lint and unit