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

Open GraalVM sources out of the box #2575

Merged
merged 17 commits into from
Dec 17, 2020

Conversation

JaroslavTulach
Copy link

The GraalVM sources use a fine tuned build system called mx (shorthand of MaXine). The mx command seems to work fine for the special purposes of building own enhanced JVM and it looks like it is going to stay. The goal of this PR is to establish a solid infrastructure to make sure the Apache NetBeans IDE as well as products based on top of it (let's name IGV and VSNetBeans) understand the suite.py metadata files and can use the mx command to build and run/debug unit tests.

The mx build system integrates with various IDEs via mx ideinit command. That command generates essential metadata for Eclipse, IntelliJ as well as NetBeans. However the NetBeans solution is based on Ant scripts. It has some limitations and moreover the Apache NetBeans project decided to focus on Maven (and Gradle) rather than promoting the old fashioned Ant based projects. As such, I'd like to deprecate the mx netbeansinit command in mx and rather propose all NetBeans users to switch to native mx projects support proposed by this PR.

The mx build system isn't in wide spread use (however it is used by independent groups like at the Postdam university for its unique features allowing to easily extend the JVM). Providing support for such exotic build system may seem unnecessary, but there are reasons to do it. We already have support for OpenJDK projects, which is also kind of special as well. As far as I know there were no problems with it. It fully builds on the modular architecture of NetBeans and comes as a single module. Moreover it gives NetBeans a fantatic story for OpenJDK developers. I'd like to repeat the same with the mx projects support - a single, isolated module causing no harm, handy (out of the box) for those who use mx as their build system. Acceptable?

The PR isn't ready to be merged right now, I still need to find out what to do with the tests. Still, I wanted to bring this idea to your attention. If we agree this code is "integratable" I'll polish the code and tests to comply with the Apache requirements.

PS: My hope is that the approach of this PR is found acceptable for Apache NetBeans project. If that happens the code of this module is going to be more easily approachable to other people building NetBeans and together we can improve experience of NetBeans users even when working with other projects (I dream about improved compile on save capability, better API for external execution & co.).

@JaroslavTulach JaroslavTulach added do not merge Don't merge this PR, it is not ready or just demonstration purposes. Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) labels Dec 3, 2020
@JaroslavTulach JaroslavTulach self-assigned this Dec 3, 2020
@neilcsmith-net
Copy link
Member

Thanks for the request to look at this. I was thinking about alternative build systems and NetBeans earlier this year but not had time to look further (after Bach talk at our room at FOSDEM). I do wonder whether trying to add internal support for other specific build systems is the right approach. Maybe a standard declarative project form (with optional JS, JShell, etc.) scripting and/or support for Build Server Protocol might be a better way forward?

@JaroslavTulach JaroslavTulach added this to the 12.3 milestone Dec 3, 2020
@JaroslavTulach
Copy link
Author

Build Server Protocol might be a better way forward?

Interesting approach. Thanks for sharing the link. After skimming through the docs three questions puzzle in my mind:

  1. While promised in the teaser pages, there is no specification for debugging - that's tough one to abstract
  2. I was hoping to have the mx projects support in 12.3 - yet the BSP looks very experimental (like the missing debugging support)
  3. Should NetBeans acts as a server or client? The NetBeans infrastructure is actually ideal for abstracting over various build systems and responding to the BSP queries!

Ideally we get this PR in, someone sits and implements BSP server based on NetBeans project infrastructure and then we'll have BSP for mx! Did I turned it all upside down?

Copy link
Contributor

@lkishalmi lkishalmi left a comment

Choose a reason for hiding this comment

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

Interesting piece of work. Seems sane.

@lkishalmi
Copy link
Contributor

Unapproved license in 4 file(s) 
https://github.com/apache/netbeans/tree/master/java/java.mx.project/src/org/netbeans/modules/java/mx/project/mx-suite.py
https://github.com/apache/netbeans/tree/master/java/java.mx.project/src/org/netbeans/modules/java/mx/project/Bundle.properties
https://github.com/apache/netbeans/tree/master/java/java.mx.project/release/org.eclipse.jdt.core.prefs
https://github.com/apache/netbeans/tree/master/java/java.mx.project/test/unit/src/org/netbeans/modules/java/mx/project/suitepy/compiler-suite.py

@JaroslavTulach JaroslavTulach removed the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Dec 6, 2020
@JaroslavTulach
Copy link
Author

Unapproved license in 4 file(s)
java/mx/project/Bundle.properties
release/org.eclipse.jdt.core.prefs

Both files fixed by adding the license header.

modules/java/mx/project/suitepy/compiler-suite.py

Only a test class, but removed, the parsing is now tested as part of the 977d1eb integration tests

mx-suite.py

Removed in f1fadbb

@matthiasblaesing
Copy link
Contributor

I only skimmed the code. I still have a WTF feeling when I consider, that once again a build system was invented.

If I'm not mistaken, it has the same problems, that gradle exhibits, i.e. the configuration format is an executable file (at least I understand, that the suite.py is the configuration and the file ending indicates full blown python file). This raises for me the question: Will the approach to statically parse that file scale to the future?

In CoreSuite.java a big set of libraries seems to be generated. Are these dependencies set in stone? It looks like a strange approach to dependency management.

The one question in my mind: I read mx as a special system invented in the context of graal, does it pull enough weight to be implemented in the core of netbeans or could it live a s a plugin in the plugin portal?

Copy link
Contributor

@jlahoda jlahoda left a comment

Choose a reason for hiding this comment

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

I guess I concur with Matthias' feeling about another build system invented. But, it is not use who is inventing it, we just add support for it.

I'll leave detailed answers on reading the suite.py on Jaroslav, but my understanding is that this is "mostly declarative" - i.e. there appears to be a (semi-)JSON embedded in the Python file. We can parse the (semi-)JSON without running the code, hopefully.

Regarding the BSP - I wonder how (if) it solved the problem this project type is solving. Yes, part of this project type is a way to actually do a build action, and start debugger, etc. But, there is also a part that determines the source roots, their relationship and classpaths. That is needed by the editor, and I am not sure if BSP provides some configuration.

I may be biased, but I think having the project type here in the Apache NetBeans project would allow joint development, which is likely needed.

I only skimmed through the patch (didn't check line-by-line), but overall looks reasonable. I tried to open an mx project, and it did something that seemed reasonable. Added comments on two places that may need some tweaking. I think I am not worried too much of this causing technical problems: unless someone opens an mx project, the project type should have minimal, I'd hope.

@JaroslavTulach
Copy link
Author

Thanks for the review.

I only skimmed the code. I still have a WTF feeling when I consider, that once again a build system was invented.

I share your feelings. Looks to me every bigger project ends up inventing own build system (NetBeans included). I spent my first year in OracleLabs trying to remove mx (at least it now integrates with Maven central), but when it got to either me or mx decision point I gave up and let mx be. Having mx creates some weird situations (like the need for this PR), but overall one can get used to it.

If I'm not mistaken, it has the same problems, that gradle exhibits, i.e. the configuration format is an executable file (at least I understand, that the suite.py is the configuration and the file ending indicates full blown python file).

Right the suite.py configuration is written in Python syntax. However there is 1:1 mapping to JSON and that's what this module is using to parse the configuration files in a declarative way. E.g. we can avoid the Gradle situation.

This raises for me the question: Will the approach to statically parse that file scale to the future?

  • There are other systems in OracleLabs infrastructure that use the conversion to JSON and subsequent parsing.
  • There seems to be an overall consensus that this is desirable.
  • There are gate checks that run the code used in this PR - should that ever get broken integration into any Graal repository gets rejected
  • Once this PR is accepted into NetBeans I modify the gates to use the NetBeans to do the actual parsing/verification - btw. it would be great if there was --verifyprojectconsistency dir option to run NetBeans in headless mode and verify project structure is still without any errors from a NetBeans IDE point of view, CCing @jlahoda! Recently also requested on a mailing list.

Overall the conversion to JSON and parsing approach shall be safe forever.

In CoreSuite.java a big set of libraries seems to be generated. Are these dependencies set in stone? It looks like a strange approach to dependency management.

These libraries are mostly infrastructure JARs used by the actual mx Python code to perform unit testing, benchmarking, signature testing, code coverage tasks. These libraries may grow over time (just recently a newer version of sigtest was added). But overall they are kept stable and immutable.

The one question in my mind: I read mx as a special system invented in the context of graal, does it pull enough weight to be implemented in the core of netbeans or could it live a s a plugin in the plugin portal?

I'd like to get this PR in for 12.3 in its simple form, but long term I have to agree - it would be better to provide this module as a standard Apache NetBeans extension on plugin portal. We need:

  • build system changes to create a cluster(?) that is not part of final ZIP, but its NBMs are part of the release
  • make sure ergonomics module scans these NBMs for various registrations
  • polish the UX of downloading NBMs via ergonomics - for example when mx.*/suite.py folder is found
  • ultimately we can modify installer to install just platform, ide, ergonomics clusters by default and let the rest be downloaded on a fly!

I always wanted ergonomics to allow downloads, decrease the download size, promote features to install and I am thrilled to see this being considered again. However, assuming this PR doesn't negatively affects other modules when people don't use mx, I'd prefer to integrate it for 12.3 in its simple form and let the great visions materialize later. By integrating this PR as is, the module is immediately available in VSNetBeans - it would be great if this support could get into VSNetBeans as soon as possible.

@matthiasblaesing
Copy link
Contributor

Just for the record: I expressed my concerns, but this change has minimal cross cutting changes and does not bring to much code in. So if mx support gets extracted/removed at some point, it can be done with minmal fuzz and thus the potential negative impact is minimal.

@lkishalmi
Copy link
Contributor

It seems Travis is complaining on some jaxb stuff for Java 8 tests, I guess that's unrelated with this PR. @JaroslavTulach please feel free to Squash and merge this one.

@JaroslavTulach
Copy link
Author

Thanks a lot for your reviews. As soon as the gates seem fine, I merge.

@JaroslavTulach JaroslavTulach merged commit 38b64ea into apache:master Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants