TranscryptOrg / Transcrypt

Python 3.9 to JavaScript compiler - Lean, fast, open!
https://www.transcrypt.org
Apache License 2.0
2.83k stars 213 forks source link

XVFB dependency & issues for dev #171

Closed RCL-Carl closed 7 years ago

RCL-Carl commented 7 years ago

Hi,

I'm new to this project so maybe this is a repeat but I didn't see it in the issues list, so hear goes nothing.

Concern Num 1

I'm trying to get a dev environment setup so that I can run the automated test suite. After looking at the .travis.yml file, there are some dependency requirements but I found this doesn't capture everything. On my debian machine I had to install xvfb in order to get this to run:

$> sudo apt-get install xvfb

Once I got this installed, I could run the 'run set1'

Concern Num 2

'run prepare' attempts to use sudo. In Travis, I'm sure that works find but as a developer, when a script asks me for sudo - that concerns me because any subsequent calls to sudo will run without me knowing about it. I know I can pull up the script and look at it to figure out what it is but that isn't necessary in this case. All the script should have to do is check if the symlinks exists, something like:

    if [ ! -e /usr/bin/python3.5 ]
    then
        sudo ln -s /usr/bin/python3 /usr/bin/python3.5 2>/dev/null
    fi

    if [ ! -e /usr/bin/nodejs ]
    then
        sudo ln -s /usr/bin/node /usr/bin/nodejs 2>/dev/null
    fi

Concern Num 3

The 'run set1' command launches the python test server and xvfb command for the headless firefox which is cool, but the script does not cleanup after itself when it runs. Again, I get it - when this runs in travis/docker, it is going to clean up after itself by just bringing down the instance. Unfortunately, this means to run these tests I have to run docker or use travis cli. I can run the 'run set1' test script at the command line in a normal debian box. The results are as expected but now because the python server and xvfb commands are still running, I have to go in and kill them individually before I can rerun.

I've made some changes to that run script to capture the PID of the python test server and PGID of the xvfb-run command (this script does not kill its children correctly, which sucks, but there is a way to deal with it using setsid). I can now kill these processes after the test script runs. I'm planning to add a command line argument flag to tell the test script to kill the processes after it has run so that the default behavior does not change.

Request

If I submit a pull request for these three items, will you accept it?

RCL-Carl commented 7 years ago

that concerns me because any subsequent calls to sudo will run without me knowing about it.

I may have spoken too soon on this - I think it asks you every time in the script. But I think checking if the a file or symlink exists for those before blowing them away is still a good idea.

axgkl commented 7 years ago

Hi, author of the travis script here. As you also mentioned, the script was created for travis, which

For local tests I do this:

If I wanted to automate the testing while coding I would set up a file change monitor and on any change would reload my local browser on the /do/set1 page, wouldn't that be enough?

Would you say a docker container incl xfvb is missing, running tests within, with mounted sources within?


Nonetheless, regarding your concern #2: I'm with you regarding prepare, hammering the symlinks in via sudo even if not necessary is bad style. Guess Jacques would accept a PR to do this only if requred w/o probs ;-)

RCL-Carl commented 7 years ago

Hi - thanks for the response.

provides xvfb (and FF)

Sure - I understand. I was mentioning this because from what I can tell, there is no documentation describing what the dependencies are for development and running the test suite. Adding a section to the README.md in the continuous_integration directory would also probably be sufficient. Unfortunately, python doesn't seem to have a good way to track application dependencies that aren't in PyPi.

If manual: I simply start the bottle test server and hit /do/set1 with a browser. Meaning: I don't need xvfb since I use my local browser. Also I have other test sets with my local stuff not for Transcrypt main repo.

And that makes total sense if you, as the developer, are developing on an environment with a GUI, and not over SSH (I know about x forwarding), or if you don't mind using a mouse while your developing. I personally, would prefer to just run the script and have it tell me from the command line that the unit tests have passed or failed because I can do that without having to touch a mouse (either from emacs or where ever).

Yes, this is a personal preference to a certain extent. But if that script could clean up after it self instead of leaving processes running in the background - wouldn't that be better ? After further inspection, that script has a "kill" flag, so it isn't like this is an unknown. I can't think of a reason to keep the firefox headless process running after the unit tests have run - what am I missing ?

In my environment I don't need the symlinks, on others I would set them up once.

Agreed, in debian and probably other OSs they get setup automatically. Why then, does the script automatically ask for sudo? I understand that this script prints out what sudo is being used for but that could just as easily be a false flag by a malicious script. The script does not need sudo to check if the links exist. The documentation for this project specifically calls out in the dev process that the user needs to run this - why not make the process smoother for a new dev.

If I wanted to automate the testing while coding I would set up a file change monitor and on any change would reload my local browser on the /do/set1 page, wouldn't that be enough?

Requires a GUI again - see above.

Would you say a docker container incl xfvb is missing, running tests within, with mounted sources within?

I'm not sure I understand the question. Are you asking, if I am asking, for a docker container to run these tests in ? No, I'm not asking for that and that isn't required for complete documentation. I do think a complete list of dependencies should be required. I do think that being able to run the script the same way that the travis does without having the bloat of a docker container would be better than having to run the unit tests from the command line using travis or docker.

I've pushed a branch on a fork that shows an incomplete version (ie, not all error conditions are checked, etc) what I'm proposing here:

https://github.com/RCL-Carl/Transcrypt/pull/1

edit: for -> or

JdeH commented 7 years ago

@AXGKl I've assigned this to you, let me know if that's ok.

axgkl commented 7 years ago

@RCL-Carl :

checked your PR, from my side you can send it to Jacques, its ok to check for presence of the node and python binaries and only if not present do the symlink with sudo, plus the formatting changes and killing of processes (still I don't see the need to do that on travis).

Gets me to the the dev workflow: You say docker is bloat and I'm with you. But in that sense: Please clarify to me why you prefer to set up a full X environment on the server you develop on, compared to just starting the test server there and hit it with your local browser, either direct or via an ssh tunnel.

I still can't see the need for xvfb while developping. Are you a purist and have no browser at ALL on the machine you ssh connect to your server ? But why then develop Transcrypt, I mean this is all about javascript? Again: Wouldn't it suffice to just start the test server and have some automatic browser page reloader whenever code changes? I mean also for the workflow you definitelly need a browser, since you need the dev tools inspector, when the tests break, no?

Why not use that browser to all the time show you the status of the tests - and with some auto page reloader you don't need a mouse but just save the code from vim or emacs.

What we could do is to provide a switch for the test_server.py which, when set, makes it inspect the directory tree and automatically reloads a frame with the test results - so you don't have to take care of setting up the page reload infrastructure, which is tedious over remote (locally I just use an apple script to reload chrome, when there are code changes).

RCL-Carl commented 7 years ago

Are you a purist and have no browser at ALL on the machine you ssh connect to your server ?

Of course not - I would just prefer not to have to use my mouse. Obviously this is a personal preference, and I noted as much. But you haven't addressed the core of the issue. With the way the script is currently, it is impossible to run it again without killing processes or running the "run kill" script. How does this help your workflow (test_server.py + browser)? It only serves to make my preferred workflow more difficult and does nothing to help your workflow.

But why then develop Transcrypt, I mean this is all about javascript?

I would argue that nothing in the automated_tests/transcrypt directory of tests requires the browser's GUI at all. The re module certainly doesn't require it. A logging module doesn't require it. If I prefer working in a headless manner, why do you care?

This project allows me to run python on frontend and backend. This is a point that @JdeH has made in the documentation on the website so I won't belabor it here. That being said - the module availability in Transcrypt is a little sparse (re and logging are crucial for what I want to do). So I'm trying help add some of the features that I think could be valuable.

I mean also for the workflow you definitelly need a browser, since you need the dev tools inspector, when the tests break, no?

Currently, because the run set1 script only prints out Success/Error, you are correct. I have to pull up the browser to figure out what is wrong. I would actually prefer that the run set1 script generate a list of errors in the output log so that a user can see what went wrong from the command line. I think this would be helpful for TravisCI integration as well because as it currently stands, a dev can only see that one of the test suites failed - not what caused this issue.

The automated_tests\transcrypt suite is also sooo large, that it can't fit everything on the screen in chrome/firefox. Which means that in order to see the result of a test failing, most of the time you have to scroll all the way down. Perhaps this can be solved by splitting the transcrypt into separate testsuites. I think this would be beneficial for the TravisCI run as well because then you can see which parts of the testsuite fail - as opposed to just seeing a non-descript ERROR message. Alternatively, maybe the javascript for the autotest HTML page could show the conflicting errors at the top of the page with some context? Maybe both of these would be good.

The debugger I can't get around - you are right. It pains me deeply and I avoid it like the plague. I have not found a way to run a "PDB" like interface to the javascript backend. It is actually on my list of things to do, a GDB/PDB proxy to javascript in the same vein as GDB proxies are used to debug embedded firmware systems. (Or maybe something like PDB over websocket).

What we could do is to provide a switch for the test_server.py which, when set, makes it inspect the directory tree and automatically reloads a frame with the test results - so you don't have to take care of setting up the page reload infrastructure, which is tedious over remote (locally I just use an apple script to reload chrome, when there are code changes).

I think this is an interesting idea and obviously a lot of other web development projects have chosen to go this route - Meteor being the prime example I can think of. Cross platform support for directory tree events is a little bit of a pain though :-( Maybe something like fswatch.

checked your PR, from my side you can send it to Jacques, its ok to check for presence of the node and python binaries and only if not present do the symlink with sudo, plus the formatting changes and killing of processes

Ok - thanks!

axgkl commented 7 years ago

requires the browser's GUI at all

Thought that as well. But: I spent some time on phantomjs but I got into dependency hell, the tests were always missing stuff the browser ships with.

Then I decided that a real browser is actually what we want anyway, since it is the main target environment for Transcrypt code - so that the tests really reflect real life and not a mock with phantomjs. There are a few tests for nodejs as well btw. if you are only interested in the server side).

But then, WHEN you run them in the browser you can't anyway not start it up all the time when there is change, so the clean killing of it was of no prio - no body would want to wait for firefox starting up all the time.

=> You want the browser running all the time until finished.


Now for you mouse/remote development problem: I see that a fileystem monitor decoupled from the testserver is hard to do. If it runs remote it can't reload the local browser if its local it does not see the changes (w/o sshfs/fuse or stuff like that).

=> so you made me add another hack into the test_server.py, a dev mode where it makes your browser reload the test page when it detects changes on the filesystem the test server runs in.

Hope thats somewhat ok for you.


btw, pure interest and no answer is fine: Why do you want a javascript backend anyway? Which cool stuff am I missing I can't have in Python? I mean: I understand that js people want it but for python people (i.e. Transcrypt audience): Does nodejs beat python in areas where not other languages beat it even by a larger offset, e.g. go, elixir, rust...

axgkl commented 7 years ago

Until jacques merges the dev server stuff, you can copy it from here, standalone module (test_server.py):

https://github.com/AXGKl/Transcrypt

workings as said here:

https://www.youtube.com/watch?v=lvTjKWVOwXA

JdeH commented 7 years ago

@RCL-Carl @AXGKl: This issue touches on several subjects, some of which I believe to have been addressed now. Can we have a shortlist of what still remains to be done, or should the issue be closed?

RCL-Carl commented 7 years ago

Thought that as well. But: I spent some time on phantomjs but I got into dependency hell, the tests were always missing stuff the browser ships with.

Then I decided that a real browser is actually what we want anyway, since it is the main target environment for Transcrypt code - so that the tests really reflect real life and not a mock with phantomjs. There are a few tests for nodejs as well btw. if you are only interested in the server side).

I think that using the firefox browser directly is great, in fact, I think doing this CI test on as many popular browsers as possible would be even better.

But then, WHEN you run them in the browser you can't anyway not start it up all the time when there is change, so the clean killing of it was of no prio - no body would want to wait for firefox starting up all the time.

=> You want the browser running all the time until finished.

But you can't do that right now either - you have launch the test server and then use the browser manually to do this. I personally feel we are in violent agreement on some things and we are possibly mis-communicating on others.

=> so you made me add another hack into the test_server.py, a dev mode where it makes your browser reload the test page when it detects changes on the filesystem the test server runs in.

Woah - I did not ask you to do any such thing. In fact, I already pushed commits to fix the issue that I reported. I said it is an interesting idea and posited that it could be done without code changes at all in a cross-platform way with fswatch. This is very different from requesting a feature.

Why do you want a javascript backend anyway? Which cool stuff am I missing I can't have in Python?

I think you misinterpreted what I said. We are both in violent agreement that running python on both the backend and frontend is preferred.

@JdeH

Can we have a shortlist of what still remains to be done, or should the issue be closed?

The only outstanding item that I forgot was updating the documentation noting the XVFB dependency. Possibly not high priority but I think valuable.

axgkl commented 7 years ago

I think that using the firefox browser directly is great, in fact, I think doing this CI test on as many popular browsers as possible would be even better.

true - and it would be possible to do in travis. Also the different switches like -e6 -e5 should be tested. You think of using the selenium integration they offer, rite ?

Woah - I did not ask you to do any such thing. In fact, I already pushed commits to fix the issue that I reported. I said it is an interesting idea and posited that it could be done without code changes at all in a cross-platform way with fswatch. This is very different from requesting a feature.

I'm doing it like fswatch, just using entr which is basically the same just that I know it.

=> You want the browser running all the time until finished. But you can't do that right now either - you have launch the test server and then use the browser manually to do this.

with that dev mode you can now, no, I mean the manual / mouse thingy is not necessary anymore (?) I mean I made the video to show you that you start this test server once, browser once, then hack around for 10 hours and the browser will always display the current test result from your last save in your editor - w/o mouse required.

The only outstanding item that I forgot was updating the documentation noting the XVFB dependency. Possibly not high priority but I think valuable.

is xvfb still needed in your world, with that dev mode? communication problem most obviously... but why in this world I want to install X dependencies on the machine I develop transcrypt - when I have a nice local browser at hand - which now also automatically reloads and tests the code I'm writing?

From what I see you are one darn capable guy, so I guess its just because I'm overlooking something => Maybe YOU should add the xvfb dep including the scenario when somebody possibly wants that into the readme. Feel free to do so, I'm can't since I don't believe in its necessity :-)

JdeH commented 7 years ago

@AXGKl @RCL-Carl, I've a request on this, since I'd like to close this issue:

axgkl commented 7 years ago

General Remark on your request for my approval on anything regarding CI topic: I don't want to be a bottleneck and it could well be that I'm off months as already happened.

=> I'd like to be able to appoint people which also have full write permissions, i.e. people which I judge capable enough to no screw up plus who share the same goal - up to the extent that such a person I trust decides to delete everything I did and rewrite it.

@RCL-Carl I would, besides you, grant that permission, i.e. you can assume my complete ack for any PR he'd send.

Having said that, just a note: I disagree with the necessity for xfvb to develop transcypt and I see it only necessary when building a seperate automated CI chain.

JdeH commented 7 years ago

@AXGKl :

  1. What exactly do you mean by "full write permissions"? Currenly I am the only one having write permission on the master branch of the QQuick/Transcrypt/master. Do you propose to change that and give other people also write access there? Or do mean fully authorizing people to replace you as owner of the CI in case you're absent, but not changing any permissions on the repo?
  2. I've noticed your difference of view (or maybe just focus) on that point. The best thing would be if we could have some clarification in the docs that explains when you need xfvb and when you don't. I read in your reaction:
    • xfvb is not necesary to develop
    • xfvb is necessary when building an automated CI chain

These elements, with some very brief explanatory text, are precisely what a developer should know. Could you formulate that in a paragraph for the readme?

Kind regards Jacques

axgkl commented 7 years ago

ok, here a short update of the readme, https://github.com/QQuick/Transcrypt/pull/194

Regarding perms: I did naturally not ask for direct push perms., I just say to you that if @RCL-Carl sends you a PR involving the CI you can assume my approval - even if he deletes anything I did, mine was a quick hack as you know, to solve an immediate problem and we all know that the whole testing could be done far more polished with a real gui and a websocket between client and server and so on, and I'm the last who would block any improvement. Again: I consider my stuff as problem solving quick hack and not blocking any improvement.