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

PR : Spyder 6 compatibility #349

Open
wants to merge 39 commits into
base: master
Choose a base branch
from
Open

Conversation

Zybulon
Copy link

@Zybulon Zybulon commented Sep 7, 2024

Hello,

I have updated the code to be compatible with Spyder 6.

@Zybulon Zybulon marked this pull request as draft September 7, 2024 14:48
@Zybulon Zybulon marked this pull request as ready for review September 7, 2024 14:51
@Zybulon Zybulon marked this pull request as draft September 7, 2024 14:54
@Zybulon Zybulon marked this pull request as ready for review September 7, 2024 15:19
@Zybulon Zybulon marked this pull request as draft September 7, 2024 15:20
@Zybulon Zybulon marked this pull request as ready for review September 7, 2024 15:26
@Zybulon Zybulon marked this pull request as draft September 8, 2024 09:27
@Zybulon Zybulon mentioned this pull request Sep 21, 2024
@Zybulon
Copy link
Author

Zybulon commented Sep 21, 2024

The tests do not pass. They failed not because of the corrections I made but because of the dependencies are too old.
Honestly I don't think I would have time to make the tests pass. So I think I'll stop here, unless someone update the tests.

@Zybulon Zybulon marked this pull request as ready for review September 22, 2024 06:08
@Zybulon
Copy link
Author

Zybulon commented Sep 22, 2024

All the test are ok except for one. The test behaviour is not reproductible, if you run it enough time it will eventually pass. I would say that it is not a real issue but the tests needs to be improved.

@Zybulon Zybulon changed the title Spyder 6 compatibility PR : Spyder 6 compatibility Sep 28, 2024
@Zybulon
Copy link
Author

Zybulon commented Sep 28, 2024

@steff456 could you review the PR so that it can be merged with the main branch ?

@ccordoba12
Copy link
Member

@Zybulon, thanks a lot for your work on this! We'll take a look at it to release a new version as soon as we can (probably in two or three weeks).

@bnavigator
Copy link
Contributor

Any news here?

bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Nov 20, 2024
https://build.opensuse.org/request/show/1224495
by user bnavigator + anag+factory
Not maintained upstream. (Attempt stalled: spyder-ide/spyder-terminal#349)
@juliangilbey
Copy link

Amazing, thanks @Zybulon!

Two extra patches I would make:

  1. pipes is deprecated, so in setupbase.py, replace import pipes with import shlex and change pipes.quote to shlex.quote. (Later versions of pipes just said from shlex import quote anyway!)
  2. distutils is deprecated. It is used in setupbase.py and spyder_terminal/confpage.py, so these should be replaced.

@juliangilbey
Copy link

In package.json, we could also get rid of promise-polyfill (removed in e0ff7f8) and fetch (included in NodeJS and never imported). I´m not sure why we need babel-loader or @babel/core listed explicitly, either.

@juliangilbey
Copy link

OK, I've managed to build the package with your PR, but the tests didn't all run. Here are the further things I've found:

  1. It seems fine to remove promise-polyfill, fetch, babel-loader and @babel/core; at least doing so does not break any tests.
  2. The file .yarnrc is ignored and should therefore be removed; the --modules-folder option is no longer valid. So the build should do (the equivalent of) mv node_modules spyder_terminal/server/static/components at the end of build_static. Otherwise things break quite badly.
  3. In spyder_terminal/server/static/index.html, there are two references to the components directory. With the previous change, the jquery script will be found. But the xterm.css file has moved; that link should be changed to <link rel="stylesheet" href="static/components/@xterm/xterm/css/xterm.css" />

In spite of all this, there is still one test that fails consistently for me: spyder_terminal/tests/test_terminal.py::test_terminal_paste_1 - @Zybulon, was this the same one that failed for you? Here is the rather unhelpful but somewhat disturbing failure log:

____________________________ test_terminal_paste_1 _____________________________

setup_terminal = <spyder_terminal.terminalplugin.TerminalPlugin object at 0x7f56f43ed880>
qtbot_module = <pytestqt.qtbot.QtBot object at 0x7f57015c0830>

    @flaky(max_runs=3)
    @pytest.mark.skipif((os.environ.get('CI') and
                         (sys.platform.startswith('linux') or WINDOWS)),
                         reason="Doesn't work on Linux and Windows CIs")
    def test_terminal_paste_1(setup_terminal, qtbot_module):
    [... code snipped ...]
        term.view.paste()
        qtbot_module.wait(1000)
>       qtbot_module.waitUntil(lambda: check_paste(term, expected),
                               timeout=TERM_UP)
E       pytestqt.exceptions.TimeoutError: waitUntil timed out in 40000 milliseconds

spyder_terminal/tests/test_terminal.py:200: TimeoutError
----------------------------- Captured Qt messages -----------------------------
webpack://spyder_terminal/./spyder_terminal/server/static/js/main.js?:None:1:

    QtCriticalMsg: Uncaught SyntaxError: Invalid or unexpected token
---------------------------- Captured stdout setup -----------------------------
Warning: None of the following fonts is installed: ['']
Warning: None of the following fonts is installed: ['']

----------------------------- Captured stdout call -----------------------------

----------------------------- Captured stderr call -----------------------------
[15376:15474:0107/211243.542459:ERROR:database.cc(1696)] Cookie sqlite error 5, errno 0: database is locked, sql: PRAGMA auto_vacuum
[15376:15474:0107/211243.542497:ERROR:database.cc(1696)] Cookie sqlite error 5, errno 0: database is locked, sql: PRAGMA journal_mode=TRUNCATE
[15376:15474:0107/211243.542514:ERROR:database.cc(1696)] Cookie sqlite error 5, errno 0: database is locked, sql: SELECT 1 FROM sqlite_master WHERE type=? AND name=?
[15376:15474:0107/211243.542562:ERROR:database.cc(1696)] Cookie sqlite error 5, errno 0: database is locked, sql: SELECT 1 FROM sqlite_master WHERE type=? AND name=?
[15376:15474:0107/211243.542571:ERROR:database.cc(1696)] Cookie sqlite error 5, errno 0: database is locked, sql: CREATE TABLE meta(key LONGVARCHAR NOT NULL UNIQUE PRIMARY KEY, value LONGVARCHAR)
[15376:15474:0107/211243.542749:ERROR:database.cc(1696)] Cookie sqlite error 5, errno 0: database is locked, sql: PRAGMA auto_vacuum
[15376:15474:0107/211243.542763:ERROR:database.cc(1696)] Cookie sqlite error 5, errno 0: database is locked, sql: PRAGMA journal_mode=TRUNCATE
[15376:15474:0107/211243.542777:ERROR:database.cc(1696)] Cookie sqlite error 5, errno 0: database is locked, sql: SELECT 1 FROM sqlite_master WHERE type=? AND name=?
[15376:15474:0107/211243.542809:ERROR:database.cc(1696)] Cookie sqlite error 5, errno 0: database is locked, sql: SELECT 1 FROM sqlite_master WHERE type=? AND name=?
[15376:15474:0107/211243.542819:ERROR:database.cc(1696)] Cookie sqlite error 5, errno 0: database is locked, sql: CREATE TABLE meta(key LONGVARCHAR NOT NULL UNIQUE PRIMARY KEY, value LONGVARCHAR)

What is that syntax error in main.js? Or is that just a red herring?

@juliangilbey
Copy link

Actually, I wrote:

  1. The file .yarnrc is ignored and should therefore be removed; the --modules-folder option is no longer valid. So the build should do (the equivalent of) mv node_modules spyder_terminal/server/static/components at the end of build_static. Otherwise things break quite badly.

But only xterm.css and jquery.min.{js,map} are needed, so only these files need to be copied into the final distribution. So it would be sufficient to do (the equivalent of):

cp node_modules/jquery/dist/jquery.min.* spyder_terminal/server/static/js
cp node_modules/@xterm/xterm/css/xterm.css spyder_terminal/server/static/css

and then modify spyder_terminal/server/static/index.html to point to the new file locations.

@juliangilbey
Copy link

  1. It seems fine to remove promise-polyfill, fetch, babel-loader and @babel/core; at least doing so does not break any tests.

Nope; @babel/core is required.

@Zybulon
Copy link
Author

Zybulon commented Jan 11, 2025

In spite of all this, there is still one test that fails consistently for me: spyder_terminal/tests/test_terminal.py::test_terminal_paste_1 - @Zybulon, was this the same one that failed for you? Here is the rather unhelpful but somewhat disturbing failure log:

Yes that's it. But I did not notice the Captured stderr call. For me, the problem is related to the test rather than the actual code and I have no idea how to solve it.

@Zybulon
Copy link
Author

Zybulon commented Jan 11, 2025

Actually the success of this test is not reproductible. For example on my repository it worked.

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.

4 participants