Rust - Githubissues

vieiro commented 1 year ago

Initial support for Rust.

vieiro commented 1 year ago

ant seems to work fine, but ant -Dcluster.config=rust doesn't... :-(

In order to make ant -Dcluster.config=rust one has to add nb.cluster.rust to clusters.config.full.list in nbbuild/cluster.properties.

I don't know that the implications of those are for the build system/github actions, though...

vieiro commented 1 year ago

Hi @lbownik , your review is appreciated and will be taken into consideration as time permits.

Regarding readability concerns, please note that readability is on the eye of the beholder. We don't want to discuss about readability concerns in tihs PR, nor about the advantages of ternary operators over if statements, more than strictly neccessary.

matthiasblaesing commented 1 year ago

@vieiro minimal nitpicks to get this compiling:

matthias@enterprise:~/src/netbeans$ git diff                                                                                                                                                                
diff --git a/nbbuild/cluster.properties b/nbbuild/cluster.properties
index 4a2094455678..9bda205fd9a4 100644
--- a/nbbuild/cluster.properties
+++ b/nbbuild/cluster.properties
@@ -51,6 +51,7 @@ clusters.config.full.list=\
         nb.cluster.cpplite,\
         nb.cluster.groovy,\
         nb.cluster.php,\
+        nb.cluster.rust,\
         ${clusters.config.standard.list},\
         nb.cluster.ergonomics
 # ergonomics must be last
diff --git a/rust/rust.project/nbproject/project.xml b/rust/rust.project/nbproject/project.xml
index b8d07ca1ed06..5a0d8e6094d4 100644
--- a/rust/rust.project/nbproject/project.xml
+++ b/rust/rust.project/nbproject/project.xml
@@ -168,11 +168,6 @@
                     </run-dependency>
                 </dependency>
             </module-dependencies>
-            <test-dependencies>
-                <test-type>
-                    <name>unit</name>
-                </test-type>
-            </test-dependencies>
             <public-packages>
                 <package>org.netbeans.modules.rust.project.api</package>
             </public-packages>
matthias@enterprise:~/src/netbeans$

My understanding on the usage of clusters.config.full.list is, that it is used as the basis to find all modules, the dependencies and enumerate all clusters. This at least I read into the code present in nbbuild.

The second change present above is to fix the build complaining about a project without test code having test dependencies.

vieiro commented 1 year ago

Thanks, @matthiasblaesing , for the review. Last two commits should do it.

mbien commented 1 year ago

ant seems to work fine, but ant -Dcluster.config=rust doesn't... :-(

In order to make ant -Dcluster.config=rust one has to add nb.cluster.rust to clusters.config.full.list in nbbuild/cluster.properties.

the full list seems to be special since it is used here: https://github.com/apache/netbeans/blob/5043b05a71bfe305b7939e777b8daaca4d093734/nbbuild/cluster.properties#L177-L178

I don't know that the implications of those are for the build system/github actions, though...

if we intend to merge to master right away, we will need a config which is release+rust and change all occurrences of

-Dcluster.config=release in https://github.com/apache/netbeans/blob/master/.github/workflows/main.yml to that new config.

matthiasblaesing commented 1 year ago

if we intend to merge to master right away, we will need a config which is release+rust and change all occurrences of

I don't see this as given. You can trivially build a "rustified" NetBeans running ant -Dcluster.config=full. A further change is only necessary once we want to run unittests in CI/CD or ship b default.

mbien commented 1 year ago

@matthiasblaesing well I assumed we want to test rust in CI. The paperwork job, CV tests etc all work with the build artifact which does not contain rust right now. (see dev list)

vieiro commented 1 year ago

I don't know that the implications of those are for the build system/github actions, though...

if we intend to merge to master right away, we will need a config which is release+rust and change all occurrences of

-Dcluster.config=release in https://github.com/apache/netbeans/blob/master/.github/workflows/main.yml to that new config.

No special preferences in merging to master or wherever else from my side, I'll go for whatever is easier for us.

.github/workflows/main.yml is a complete mistery to me, I'm afraid, so I'll need some guidance on this.

mbien commented 1 year ago

lets talk about this on the mailing list since I am not so sure what we want to do here, IMO if it is in master and also actively developed it should be covered by some CI otherwise we can turn everything off if a PR is labeled with rust since it isn't even building it.

matthiasblaesing commented 1 year ago

I did a first pass and these are my notes:

All: The module names should not contain the folder name prefix rust.XXXX -, just the part after the "dash"
All: The commits should be squashed at least partially (19 seems to be to much)
Grammar: The .interp and .tokens files from org.netbeans.modules.rust.grammar.antlr4 package can be dropped. Then the licenseinfo.xml passage can be dropped as the java files already contain the ASF header.
Grammar: Is it a good idea to modify the grammers (RustLexer.g4 and RustParser.g4) directly? For the CSS parser direct modifications were done and the grammar became disconnected from its origin, without an option to ever get updates from the source. This is ok, we just should now, whether we want to go down that road.
Grammar: The build script rebuilds the java code from the grammar, yet the generated code is checked in. In other cases (golang, CSS) we generate the java code once and there are instructions in the generated files, that changes have to be made in the grammar files and rebuilding the java files is necessary.
Grammar: the Icons in org.netbeans.modules.rust.grammar.structure.resources need entries in the licenseinfo.xml
Project: It is surprising, that the "files" view shows less, than the "projects" view. All other types I'm aware of show a subset of data in the projects view and the files view gives access to data filterer in the project view
Project: The ZIP file should not be there. There are two issues with this: we should not add ZIP files to two repository as the contents can be checked in directly. This then also allows to use the input in the wizard to update the Cargo.toml accordingly. The .gitignore file can be trivially created on the fly.
Project: When the IDE is shutdown and start again, the project node is not correctly loaded (the Files view renders the src folder): Closing and opening the project again fixes this.
Project API: src/org/netbeans/modules/rust/project/api/rust-logo.png is not used.
Sources: The file src/org/netbeans/modules/rust/sources/rs/templates/rust-file.png should use COMMENT_UNSUPPORTED
Cargo: Adding a dependency fails on ubuntu the add command does not exist (error message: error: no such subcommand: add)

Cargo: Using a special dependency:

[dependencies]
regex = { git = "https://github.com/rust-lang/regex" }

Creates a null pointer exception in SemVer:

 [exec] java.lang.NullPointerException
 [exec]     at java.base/java.util.regex.Matcher.getTextLength(Matcher.java:1770)
 [exec]     at java.base/java.util.regex.Matcher.reset(Matcher.java:416)
 [exec]     at java.base/java.util.regex.Matcher.<init>(Matcher.java:253)
 [exec]     at java.base/java.util.regex.Pattern.matcher(Pattern.java:1133)
 [exec]     at org.netbeans.modules.rust.cargo.api.SemVer.<init>(SemVer.java:41)
 [exec]     at org.netbeans.modules.rust.cargo.api.RustPackage.<init>(RustPackage.java:54)
 [exec]     at org.netbeans.modules.rust.cargo.api.RustPackage.<init>(RustPackage.java:43)
 [exec]     at org.netbeans.modules.rust.cargo.impl.CargoTOMLParser.getDependencies(CargoTOMLParser.java:156)
 [exec]     at org.netbeans.modules.rust.cargo.impl.CargoTOMLParser.parseCargoToml(CargoTOMLParser.java:98)
 [exec] [catch] at org.netbeans.modules.rust.cargo.api.CargoTOML.reparse(CargoTOML.java:82)
 [exec]     at org.netbeans.modules.rust.cargo.api.CargoTOML.fileChanged(CargoTOML.java:124)

vieiro commented 1 year ago

Thanks for your review, @matthiasblaesing .

I did a first pass and these are my notes:

* All: The module names should not contain the folder name prefix `rust.XXXX -`, just the part after the "dash"

I prefer having folder names over modules names, but no worries. Will do in next commits.

* All: The commits should be squashed at least partially (19 seems to be to much)

I'd squash squash all commits, I think. Will do in coming days.

* Grammar: The *.interp and *.tokens files from  org.netbeans.modules.rust.grammar.antlr4 package can be dropped. Then the licenseinfo.xml passage can be dropped as the java files already contain the ASF header.

Great. I think "*.tokens" is read when generating the grammar, but it's ok to leave it out of the repository.

* Grammar: Is it a good idea to modify the grammers (RustLexer.g4 and RustParser.g4) directly? For the CSS parser direct modifications were done and the grammar became disconnected from its origin, without an option to ever get updates from the source. This is ok, we just should now, whether we want to go down that road.

I think RustLexer.g4 and RustParser.g4 must be included. These files are probably going to change frequently in the future, and we want everybody to contribute to them. This includes the original authors: the Rust Team.

* Grammar: The build script rebuilds the java code from the grammar, yet the generated code is checked in. In other cases (golang, CSS) we generate the java code once and there are instructions in the generated files, that changes have to be made in the grammar files and rebuilding the java files is necessary.

We may or may not include the generated code. The "rust.grammar/build.xml" is responsible for updating the generated code before compilation whenever the .g4 files are modified.

* Grammar: the Icons in org.netbeans.modules.rust.grammar.structure.resources need entries in the licenseinfo.xml

Very true. I ran verify-libs-and-licenses and no problems where detected, though. Thanks for pinpointing them.

* Project: It is surprising, that the "files" view shows less, than the "projects" view. All other types I'm aware of show a subset of data in the projects view and the files view gives access to data filterer in the project view

Will verify this. Any file missing in particular?

* Project: The ZIP file should not be there. There are two issues with this: we should not add ZIP files to two repository as the contents can be checked in directly. This then also allows to use the input in the wizard to update the `Cargo.toml` accordingly. The .gitignore file can be trivially created on the fly.

+1 to that. Will modify the wizard.

* Project API: src/org/netbeans/modules/rust/project/api/rust-logo.png is not used.

+1

* Sources: The file `src/org/netbeans/modules/rust/sources/rs/templates/rust-file.png` should use `COMMENT_UNSUPPORTED`

+1

Thanks for the thorough review, Matthias.

Expect some changes during next week as time permits.

matthiasblaesing commented 1 year ago

@vieiro I added a few updates, you were awfully quick in your response, sorry for that.

vieiro commented 1 year ago

@vieiro I added a few updates, you were awfully quick in your response, sorry for that.

I've seen those, no worries :-).

vieiro commented 1 year ago

Well, I think everything should be in place now.

Files tabs

We now arrange Rust workspace members (for those projects having workspaces) in a specific node, separated from normal sources. These members can then be opened as normal projects (similar to what we do with Maven modules in Maven Projects)...

imagen

... and the Files tab seems to work properly after this change...

imagen

This last commit has improved nested folding/navigation, and we now detect Rust modules too. Also we partially support dependencies with "git" references (no versions).

Things I couldn't reproduce:

Projects open well in my box even after restarting the IDE.
Adding dependencies work well for me in my Ubuntu.

This may be related to detecting the cargo in the PATH (we don't currently test for it). We may want to add this path to an Options tab, or use Project configurations so the user can choose a toolchain of her liking, or a combination of the two.

interp/token

I've dropped the "interp" and "token" files out of git, as requested, but they still appear as errors in the "rat" report (those files are regenerated after compilation whenever the grammar is changed). It's also somewhat in git status.

Let's try to test this new "full" cluster set in cluster.properties, and see what we break.

vieiro commented 1 year ago

Mmm... it seems we need to make Rust "ergonomic" aware...

This may mean that you are not using @AntBasedProjectRegistration to register
your projects, or that you need to hardcode the nature of your project into
http://wiki.netbeans.org/FitnessForever The list of differences follows:
org.netbeans.modules.gradle.NbGradleProjectFactory OK
org.netbeans.modules.java.openjdk.project.FilterStandardProjects OK
org.netbeans.modules.java.mx.project.SuiteFactory OK
org.netbeans.modules.project.ant.AntBasedProjectFactorySingleton OK
org.netbeans.modules.maven.NbMavenProjectFactory OK
org.netbeans.modules.project.ui.convertor.ProjectConvertorFactory OK
org.netbeans.modules.java.openjdk.project.JDKProject$JDKProjectFactory OK
org.netbeans.modules.rust.project.RustProjectFactory not present
org.netbeans.modules.cpplite.project.CPPLiteProject$FactoryImpl OK

Should be empty: {FAIL=org.netbeans.modules.rust.project.RustProjectFactory}

Among other problems. We need to upload this: https://web.archive.org/web/20150407062010/http://wiki.netbeans.org/FitnessForever to the wiki as well...

vieiro commented 1 year ago

The situation is that even though this PR builds with cluster.config=full, it won't build with cluster.config=release.

This is so because DynamicVerifyTest.java in ide.ergonomics has different ProjectFactory's hardwired in the tests themselves (see this commit.

If the number of ProjectFactory is different in cluster.config=full and cluster.config=release (or any other cluster configuration) then the test will fail, either because one of the hardwired ProjectFactory is missing or because it cannot be found.

We should then make DynamicVerifyTest aware of the exact ProjectFactory 's included in the cluster configuration.

vieiro commented 1 year ago

Well, with this last commit I think we're ready to merge to master and continue evolving there.

We can now pass commit-validation tests in both the release and full cluster configurations.

@mbien @matthiasblaesing , I'd appreciate a second review.

matthiasblaesing commented 1 year ago

@vieiro as a temporary fix the changes to the ergonomics tests look fine.

I have a set of papercut fixes, I'd like you to have a look at:

Full source tree: https://github.com/matthiasblaesing/netbeans/tree/rust

Changes:

https://github.com/matthiasblaesing/netbeans/commit/8e7143d7c606474deb0d2f82864d883201b74ea2 You already noticed, that the files show up in rat check (though that should only be run with a clean source tree). The idea here is to remove the file after generation.
https://github.com/matthiasblaesing/netbeans/commit/1eb7c784a05605f2bf9cc75187adabf5c3917e27 I think having the reference to the original grammar files, the changes are based on, might help in the future, to be able to reapply changes onto new variants.
https://github.com/matthiasblaesing/netbeans/commit/dfd9e14b519c78dbebf0725ddfba3e3924db8034 IDE ergonomics can turn on features if it finds indications, that the user wants to open a supporter project. This adds support for Cargo.toml as basis.
https://github.com/matthiasblaesing/netbeans/commit/5fa12d488bbc112fd3bea66b1cb809788e01e750 I noticed a big warning on NetBeans start. The contents of the mimeresolver lookup is expected to be fully ordered. The rust mimeresolver is missing a position. I derived it by looking for a hole in my running IDE configuration, which is pretty much "everything on".

Feel free to pull the changes into your tree and also to squash them.

vieiro commented 1 year ago

Thanks, @matthiasblaesing ! Commits are on their way!

vieiro commented 1 year ago

Mmm... ergonomics fails again. I'm investigating...

mbien commented 1 year ago

cool, everything is green! (removing all-tests label again) I have a commit to offer which adds a rust job to CI: https://github.com/mbien/netbeans/commit/fb0c60b52ab7dcd114b43c34ab155e9b1768de1e https://github.com/mbien/netbeans/tree/rust (added on top of this PR)

the ubuntu image the job uses has the following rust tools installed: https://github.com/actions/runner-images/blob/main/images/linux/Ubuntu2204-Readme.md#rust-tools

vieiro commented 1 year ago

Thanks, @mbien ! Merged here. Let's see what happens... :-)

vieiro commented 1 year ago

Well, everything green, let's squash and merge then. Thanks all!

matthiasblaesing commented 1 year ago

@vieiro

Well, everything green, let's squash and merge then. Thanks all!

thank you for your work. I had a look at the history and I noticed two thinks:

the squash did not happen (at least it does not look like it.
the author information is partially incomplete. Some commits only refer to your first name together with your emailaddress

vieiro commented 1 year ago

@vieiro

Well, everything green, let's squash and merge then. Thanks all!

thank you for your work. I had a look at the history and I noticed two thinks:
* the squash did not happen (at least it does not look like it.

* the author information is partially incomplete. Some commits only refer to your first name together with your emailaddress

@mbien For god's sake! This is terrible! I did press the "Squash and Merge" button! So, what do we do now? Can we undo it?

mbien commented 1 year ago

I don't know tbh since 5 other PRs were merged short after this PR: https://github.com/apache/netbeans/actions?query=branch%3Amaster

I never use the squash feature of github since it is not transparent to me how the author information is handled on github, esp when multiple authors participated. It depends on things like whether or not the user made the email public on gh - I rather squash locally and rebase the branch as last step before merge.

matthiasblaesing commented 1 year ago

@vieiro this is not nice, but also not fatal. We can't back this out, as history already progressed over it and in all fairness, the commits look sensible to me, so it is just a bit more verbose, than it should have been. Letss keep it at that.

My general feeling towards the github GUI is, that it is ok to read commits and discuss them, but when it comes to more complex merges, I prefer the git CLI, as from my POV github is not predictable. At least in the past it scrambled author information and so I would advice to use this approach:

author your PR as detailed as necessary for work
for updates while it is in work, add new commit with a granuality, that allows selective squashing at the end
when you are happy with it and review is done, squash locally, until you are happy
force push the branch
Use compare button to check, that the PR as a whole still contains the same code
wait for CI/CD to become green
do a straight merge with github gui (no rebasing, no squashing done by github)

vieiro commented 1 year ago

@matthiasblaesing Good piece of advice, indeed. I think this was discussed in the mailing list before.

So, @matthiasblaesing , @mbien , should we revert this in master and redo the merge? What say?

vieiro commented 1 year ago

@matthiasblaesing @mbien , let's discuss in the mailing list.

apache / netbeans

Rust #5567

Files tabs

Things I couldn't reproduce:

interp/token