search

graalvm / mx

Command-line tool used for the development of Graal projects.
GNU General Public License v2.0
183 stars 109 forks source link

[Suggestion] Refactor mx.py, source file too large #194

Open neomatrix369 opened 5 years ago

neomatrix369 commented 5 years ago

mx.py is growing rapidly and with this can be unreadable over time.

IDEs like IntelliJ/PyCharm are slowing down parsing the contents. Besides that its best to separate the different aspects of mx.py into separate source files/modules of their own, just like the other mx_[xxx].py files out there.

I glanced through the code and here are some of the high-level parts we could split mx.py into:

Please make your own suggestions on how you would best like it to be split, but from the current layout of the classes and methods in it, the above was apparent but maybe a better design is inherent and I'm not seeing it immediately.

A good rule to keep such important files lean: only have high-level implementations that outline what the program does, and the how it does it (i.e. implementation parts) would be best to encapsulate elsewhere via Packages, Modules, Classes, etc... This would make it easier to read and change, especially to debug when things break.

I'm happy to slowly refactor it and create PRs for individual concerns - let me know if this is okay and I will create a simple PR to start with?

dougxc commented 5 years ago

Thanks for the offer @neomatrix369 Refactoring mx.py into smaller modules has been a long time, low priority task and it would great to get some help with that.

neomatrix369 commented 5 years ago

Thanks for the offer @neomatrix369 Refactoring mx.py into smaller modules has been a long time, low priority task and it would great to get some help with that.

Cool I'm onto it, just raised my first PR #195, although will raise the refactoring ones asap

neomatrix369 commented 5 years ago

I have gone through the mx.py file - first pass, a quick glance and so far it appears that most of the classes and methods/functions in it are nearly state-machines (independent), although I'm likely to be wrong. Although there are groups of them that call or dependent on the others, it's more or less clear but when I start working on them we can confirm.

Do you want to caution me about any areas where there might be tangles that I should avoid or keep for the last?

I'll try to group them together as much as possible and then split them. Some of them cannot just be grouped - might fall under misc category (let's see).

dougxc commented 5 years ago

One problem you will run into is that there's no clear indication of what parts of mx.py are used downstream and will break if those parts are moved elsewhere. In the Graal repo, this search provides some clues: https://github.com/oracle/graal/search?l=Python&q=mx. I wonder if there's some tool to extract the API of mx.py by analyzing a bunch of repos that use it.

Disclaimer: This poor "labelling" of public API reflects the organic growth of mx from a small helper script into a full blown build system (and more) by a group whose expertise in Python best practices was/is somewhat rudimentary ;-) We should aim to rectify this with the refactoring you are doing (e.g. use _ and __ prefixes to denote "protected" and "private" members).

neomatrix369 commented 5 years ago

One problem you will run into is that there's no clear indication of what parts of mx.py are used downstream and will break if those parts are moved elsewhere. In the Graal repo, this search provides some clues: https://github.com/oracle/graal/search?l=Python&q=mx. I wonder if there's some tool to extract the API of mx.py by analyzing a bunch of repos that use it.

Disclaimer: This poor "labelling" of public API reflects the organic growth of mx from a small helper script into a full blown build system (and more) by a group whose expertise in Python best practices was/is somewhat rudimentary ;-) We should aim to rectify this with the refactoring you are doing (e.g. use _ and __ prefixes to denote "protected" and "private" members).

Thanks for all that info, I'll look into these things in the meanwhile before I raise a PR for refactoring.

I was curious to know about the answers to the below :

Do you have similar views on the above?

About the dependency graphs and call graphs I'm thinking of some solution, will try share via a separate PR.

neomatrix369 commented 5 years ago

See mx.py (internal) dependency graph from pycharm:

mx-dir-ortho (orthogonal) mx-just-classes (std hierarchical)

Apologies, you will have to zoom several levels deep to be able to see the graphs clearly.

Otherwise please try to load the UML diagrams (remove .txt extension from filename): mx.py.uml.txt

dougxc commented 5 years ago

Unfortunately the Travis gate for mx is very incomplete and so we mainly rely on testing mx on a number of downstream repos such as Graal internally. Testing your changes against Graal is therefore a good idea and will very roughly approximate the internal testing we do.

neomatrix369 commented 5 years ago

Okay while we have this in place, I will try to add to the test suites, and see how I can best bring the flow from these other efforts into tests - so we have one central place to rely on.

neomatrix369 commented 5 years ago

I'm trying to run the tests in the tests folder and so far not able to do it, I get this error:

platform darwin -- Python 2.7.15, pytest-2.9.1, py-1.6.0, pluggy-0.3.1
rootdir: /Users/swami/git-repos/OpenJDK/graal-stuff/mx/tests/mx.mxtests, inifile:
plugins: cov-2.1.0
collecting 0 itemssuite named mxtests not found
collected 0 items / 1 errors

===================================================== ERRORS ======================================================
_________________________________________ ERROR collecting mx_mxtests.py __________________________________________
tests/mx.mxtests/mx_mxtests.py:11: in <module>
    _suite = mx.suite('mxtests')
mx.py:10085: in suite
    abort('suite named ' + name + ' not found', context=context)
mx.py:12536: in abort
    raise SystemExit(error_code)
E   SystemExit: 1

Here's my scripts: https://github.com/neomatrix369/mx/tree/add-code-coverage-deps (see https://github.com/neomatrix369/mx/blob/add-code-coverage-deps/run-mx-tests.sh)

dougxc commented 5 years ago

Sorry, the person who wrote tests/mx.mxtests is no longer with the team and I'm not sure if anyone else remaining understands it (maybe @gilles-duboscq ?).

neomatrix369 commented 5 years ago

What is the target python version expected in order to run mx.py? Python 2.7?

A lot of the code in there represents Python 3.7, for eg. all the print() calls use () like in Python 3?

How as this been running in python 2.7? I think the downstream repos might have a mixed language approach as well - is that the case?

dougxc commented 5 years ago

The target version is currently both Python 2.7 and 3.6 and we gate on both. This was done to enable the transition to Python3 with the pending EOL of Python 2. While currently mx itself works on both python version, Graal itself and all other GraalVM related repos are still in the process of becoming compatible with Python3.

neomatrix369 commented 5 years ago

The target version is currently both Python 2.7 and 3.6 and we gate on both. This was done to enable the transition to Python3 with the pending EOL of Python 2. While currently mx itself works on both python version, Graal itself and all other GraalVM related repos are still in the process of becoming compatible with Python3.

Thanks, that explains why I ran into version issues when I extracted a block of code from mx.py into its own source file. I also ran into version issue in one of the downstream source files when I switched to Python 3.7.

neomatrix369 commented 5 years ago

Sorry, the person who wrote tests/mx.mxtests is no longer with the team and I'm not sure if anyone else remaining understands it (maybe @gilles-duboscq ?).

@gilles-duboscq Any suggestions on what to do with the tests?

dougxc commented 5 years ago

I spoke with @gilles-duboscq offline and we both agree you should just ignore tests/mx.mxtests. It doesn't even appear to contain any tests, but just infrastructure to write python based tests. We will remove this code to avoid further confusion. If you want to add some testing, then the tests should be probably be written using the standard Python unittest module.

neomatrix369 commented 5 years ago

I spoke with @gilles-duboscq offline and we both agree you should just ignore tests/mx.mxtests. It doesn't even appear to contain any tests, but just infrastructure to write python based tests. We will remove this code to avoid further confusion. If you want to add some testing, then the tests should be probably be written using the standard Python unittest module.

Thats a good point and I agree, I was going to do the same, remove the folder and create a new one and yes using standard Python testing libraries unittest, coverage, etc...