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

Fix FIFO for userscripts on Windows #1927

Merged
merged 5 commits into from Sep 11, 2016

Conversation

Kingdread
Copy link
Contributor

@Kingdread Kingdread commented Sep 6, 2016

Due to the way Windows handles files, we couldn't actually write to the "FIFO" file from the userscript (same issue as #1767). This patch should fix this problem with the patch from the mailinglist, and it adds a simple test for running a userscript on Windows.

On a somewhat related note: I'm wondering how those tests even pass on AppVeyor - echo is not an external command on Windows (there is no echo.exe), and you can't call echo via subprocess. The tests fail locally, but seem to work fine on AppVeyor... Any idea what magic you used to get it to work? 😉


This change is Reviewable

The userscript FIFO on Windows suffered the same problem that open-editor
once did, because files on Windows can't be opened with write access by
two different processes. We kept the oshandle around and only closed it
when the process exited, which means that userscripts could not actually
write any commands to the FIFO.

This patch closes the file earlier, allowing the userscript to actually
write commands to it.

See also
https://lists.schokokeks.org/pipermail/qutebrowser/2016-September/000256.html
"""

def __init__(self, win_id, parent=None):
super().__init__(win_id, parent)
self._oshandle = None
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the overridden __init__now.

@The-Compiler
Copy link
Member

Seems like some freshly added tests are flaky, I'll take a look.

@codecov-io
Copy link

codecov-io commented Sep 6, 2016

Current coverage is 81.63% (diff: 100%)

Merging #1927 into master will increase coverage by <.01%

@@             master      #1927   diff @@
==========================================
  Files           121        121          
  Lines         16918      16912     -6   
  Methods           0          0          
  Messages          0          0          
  Branches       2589       2589          
==========================================
- Hits          13810      13806     -4   
+ Misses         2640       2638     -2   
  Partials        468        468          

Powered by Codecov. Last update 29715ae...22ac19b

@The-Compiler
Copy link
Member

After some fixes and librally skipping everything broken for now, things are back to normal again 😆

@The-Compiler
Copy link
Member

On a somewhat related note: I'm wondering how those tests even pass on AppVeyor - echo is not an external command on Windows (there is no echo.exe), and you can't call echo via subprocess. The tests fail locally, but seem to work fine on AppVeyor... Any idea what magic you used to get it to work? 😉

Right, they are broken here too... My best guess would be that AppVeyor has some cygwin echo.exe in %PATH%? 😆

I wonder what the best way to fix them would be - is there an executable which is there and does nothing on Windows/Linux/OS X? Or do we need a (python) replacement there which expands to sys.executable and work with that?

@Kingdread
Copy link
Contributor Author

My best guess would be that AppVeyor has some cygwin echo.exe in %PATH%?

Seems quite possible, so I'll accept that as an explanation 😜

C:\Users\Daniel>C:\cygwin64\bin\echo.exe "foobar"
foobar

is there an executable which is there and does nothing on Windows/Linux/OS X?

One for all platforms? Probably not. But if the name can vary, it should be easy to find something for each one separately. For Windows, see this question, or we could drop a simple .bat into the userscript dir.

Or we use some (python) thing, as you suggested, as we're going to need a platform-dependent variable anyway.

On Windows, no echo.exe exists normally, so calling echo from the tests
is no good idea, since it relies on Cygwin to be installed and in %PATH%
(so that echo.exe is available).

This fixes this by providing a small echo.bat which is callable from the
tests, and then using a platform-specific path to the executable instead
of the hardcoded "echo". This should ensure that the tests pass even on
systems where echo.exe is not installed.

Note that we can't simply use a do-nothing exe (like rundll or hh.exe),
as we're passing parameters, and those executables may behave
differently in the presence of those parameters.
@Kingdread
Copy link
Contributor Author

FAIL tests/end2end/features/test_tabs_bdd.py::test_buffer_with_a_matching_title

Seems unrelated to my changes and fixed in master, so I've restarted the Travis build to see if everything passes now.

@The-Compiler The-Compiler merged commit b011476 into qutebrowser:master Sep 11, 2016
@The-Compiler
Copy link
Member

Thanks!

@Kingdread Kingdread deleted the windows-userscripts branch September 11, 2016 15:59
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.

None yet

4 participants