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

No kotlin plugin #437

Merged
merged 20 commits into from
Sep 16, 2021
Merged

No kotlin plugin #437

merged 20 commits into from
Sep 16, 2021

Conversation

lhstrh
Copy link
Member

@lhstrh lhstrh commented Jul 27, 2021

Building on PR #412 that was merged but still had problems.

@lhstrh
Copy link
Member Author

lhstrh commented Jul 27, 2021

I haven't been able to get this to work. Maybe @a-sr has any ideas?

@cmnrd
Copy link
Collaborator

cmnrd commented Jul 28, 2021

I think the changes in .project and .classpath shouldn't be necessary. The oomph setup should update the files as needed, and then we can commit them (at least this is how I understand the process). Are you still stuck at the same error as the one described in #412? I did a bit of googling on this error and it seems to suggest that the oomph setup cannot write a .project file. But I don't have an idea why this might be the case.

@lhstrh
Copy link
Member Author

lhstrh commented Jul 28, 2021

Basically, the Oomph setup fails with the screenshot in #383 without the changes to the .project files. With the changes, on the other hand, the maven build no longer works due to missing dependencies. I'm not sure how to move forward with this.

@a-sr
Copy link
Collaborator

a-sr commented Aug 3, 2021

I had a look at the diff and from my perspective it should do the job.

@cmnrd You need the changes in the .project and .classpath because Eclipse can only adjust them to new features (apparently, as experienced with the gradle buildship) but if there is a configuration present without support in the IDE, it will not remove it but fail.

@lhstrh When I checked out the branch right now, I got no errors when building with maven. Which dependencies were reported missing in your error?

Currently the dependencies for maven and eclipse are defined in separate files, hence, I would be surprised if changes to the Oomph setup would affect dependencies in the maven build.

However, what concerns me about removing the Kotlin plugins form eclipse entirely, is the manifest files because they probably have to define a dependency to Kotiln that is required at runtime but not available when building in Eclipse, only with maven. Maybe you need to keep the dependency in the target platform (oomph setup: "Default Target" in Master stream), if this does not cause any problems in the runtime Eclipse, and only remove it from the development eclipse installation (oomph setup: top level "P2 Director"). This way the dependency should be resolvable in the manifest but no actual Kotlin support will be activated in the source projects. Or does the compiled Kotlin code also work in the RCA without a dependency to Kotlin in the respective plugin's manifest?

a-sr added 4 commits August 31, 2021 16:44
project configurations.

Automatic classpath configuration for plugins also updated some jdt
preferences to current JRE.
requires gradle buildship. Also disabled future automatic generation of
this feature by xtext.
Removed unsued xtend-gen source folder.
@cmnrd
Copy link
Collaborator

cmnrd commented Sep 1, 2021

Thanks for your changes @a-sr! This seems to fix the oomph installation. However, the maven build is broken and it's not possible to build the RCA. As you expected correctly, this is due to the missing kotlin library. I tried today a few changes in the pom.xml, but this didn't help. I will give it another try tomorrow and see of adding the kotlin plugin back under "Defualt Target" helps.

@a-sr
Copy link
Collaborator

a-sr commented Sep 2, 2021

As mentioned before, changes to the target platform in the oomph setup will not affect the dependencies available when building with maven, these are specified in the target platform plugin and currently still provide the Kotlin dependency.
If the problem is the fact that the lflang plugin no longer has a dependency to the Kotlin plugin, then I can say that putting it back in will cause another issue. It seems the Kotlin plugin comes with its own (bundled and apparently unsigned) version of log4j which causes an exception (java.lang.SecurityException: class "org.apache.log4j.Priority"'s signer information does not match signer information of other classes in the same package) when starting the mwe file for generating grammar code.

I will take a look at this. Could you post the error you observed so I know I am following the correct steps when reproducing it?

@cmnrd
Copy link
Collaborator

cmnrd commented Sep 2, 2021

I get errors like in the following when running mvn compile:

[ERROR] /some/path/lingua-franca/org.lflang/src/org/lflang/AstExtensions.kt: (47, 25) Cannot access built-in declaration 'kotlin.collections.List'. Ensure that you have a dependency on the Kotlin standard library
[ERROR] /some/path/lingua-franca-no-kotlin-plugin/git/lingua-franca/org.lflang/src/org/lflang/AstExtensions.kt: (111, 40) Cannot access built-in declaration 'kotlin.Boolean'. Ensure that you have a dependency on the Kotlin standard library
[ERROR] /some/path/lingua-franca-no-kotlin-plugin/git/lingua-franca/org.lflang/src/org/lflang/AstExtensions.kt: (134, 20) Cannot access built-in declaration 'kotlin.String'. Ensure that you have a dependency on the Kotlin standard library

There are many more such errors. This looks to me like the kotlin stdlib is not available when compiling. The compiler itself seems to be available though. I tried to add the stdlib as a dependency in pom.xml as described here, but this did not help. I figure we need to add the stdlib also in some other place.

Added Kotlin as an optional dependency such that Eclipse will not
complain about missing dependencies nor use the broken Kotlin IDE
integration but the libraries are still available at compile- and
run-time.
@a-sr
Copy link
Collaborator

a-sr commented Sep 2, 2021

I was able to fix the problem.
The RCA build was successful but I got the error that C++ code gen is not available. Is this a problem with the runtime availability of Kotlin classes or just a bug in the way this is checked?
Could you check that please.

@a-sr
Copy link
Collaborator

a-sr commented Sep 2, 2021

In the plugin jar, the compiled class files for the Kotlin code are in the same folder as all the other classes, so I guess they should be on the classpath too.

@a-sr
Copy link
Collaborator

a-sr commented Sep 2, 2021

I just merged the master into this branch and now there are a few classes that import classes written in Kotlin. These are in fact only those that are used by the cli (StandaloneSetup, Main, etc.) but still these are build errors in Eclipse.
Maybe we need to move the cli code into a separate java project that will not be imported into Eclipse in order to allow the usage of Kotlin-based classes from the org.lflang plugin.

@a-sr a-sr mentioned this pull request Sep 2, 2021
5 tasks
@cmnrd
Copy link
Collaborator

cmnrd commented Sep 3, 2021

Thanks again for looking at this!

The RCA build was successful but I got the error that C++ code gen is not available. Is this a problem with the runtime availability of Kotlin classes or just a bug in the way this is checked?

This sounds like the Kotlin classes are either not build, or are build and not available. So yes, I believe it is a problem with the availability of Kotlin classes in general.

In the plugin jar, the compiled class files for the Kotlin code are in the same folder as all the other classes, so I guess they should be on the classpath too.

Hmm, strange. I will try to investigate.

I just merged the master into this branch and now there are a few classes that import classes written in Kotlin. These are in fact only those that are used by the cli (StandaloneSetup, Main, etc.) but still these are build errors in Eclipse.
Maybe we need to move the cli code into a separate java project that will not be imported into Eclipse in order to allow the usage of Kotlin-based classes from the org.lflang plugin.

Right. I think creating a separate package for the standalone compiler is a good idea anyway. Alternatively we could use the same trick as we use for the C++ and TS generators, and resolve imports manually at runtime to trick Eclipse, but I don't like this much...

@a-sr
Copy link
Collaborator

a-sr commented Sep 9, 2021

I was able to introduce a hard dependency to Kotlin only for the RCA to ensure the presence of Kotlin libraries at runtime.
In my test the RCA was now able to use the C++ code generation again.

@cmnrd
Copy link
Collaborator

cmnrd commented Sep 9, 2021

Great! Remains the problem that Eclipse shows errors on some of our Java files. Is this something we can live with (for now), or should we factor out those classes into their own package?

@a-sr
Copy link
Collaborator

a-sr commented Sep 9, 2021

Well, I personally have no problem with ignoring this error because I know the underlying reason and precisely which files are affected but in general, I think it is bad for a project to have a developer setup/environment that will have errors by default that cannot/should not be fixed by a new/unknowing developer. And telling people to ignore errors is not a good idea either.
Hence, I would opt for factoring out the Kotlin dependent classes. But this leads to the more general question of the repository/project structure.
The org.lflang plugin seems kind of overloaded, maybe we could not only factor out the CLI standalone part but also isolate the code generation that is language specific. I mean on the long run you cannot cramp every possible target language into this one monolithic project, you may want to have separate projects that register themselves at the compiler in order to provide their capability, plugin-like ;) In the Kieler project we built a very modular and extensible compiler framework for our languages and code generation approaches and are quite happy with the result.

@cmnrd
Copy link
Collaborator

cmnrd commented Sep 15, 2021

I made some progress on this. I created a new project org.lflang.lfc and moved Main.java as well as some other files that are only used by lfc to this package (essentially all the files that show errors in Eclipse). With this setup, I can build lfc via gradle and use it to compile C programs. However, this doesn't work for C++ yet. I am not sure why, but although the kotlin files in org.lflang are built, they do not end up in the compiled jar. Maybe @oowekyala can help with this?

Overall I think this looks very promising. There are likely a few more things that need to be adjusted though.

@lhstrh
Copy link
Member Author

lhstrh commented Sep 15, 2021 via email

lhstrh and others added 3 commits September 15, 2021 23:18
The Kotlin code generators are not loaded directly via imports. Therefore,
the minimize logic things that it can exclude them from the jar. With this
exclude, we force the whole org.lflang package to be included.
Copy link
Collaborator

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

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

I was able to fix the problem with the gradle build. I also adjusted the nightly build workflow and triggered the build to test the workflow and the artifacts. Both lfc and Epoch seem to work just fine. I also did a fresh oomph install from this branch and also there everything looks good. So I think we are finally good to go :)

I think in the near future we should factor out more classes to org.lflang.lfc. Currently, it is only the essential stuff, but there is likely more code that we can move.

@lhstrh lhstrh merged commit 59ec4e7 into master Sep 16, 2021
@lhstrh lhstrh deleted the no-kotlin-plugin branch September 16, 2021 19:50
petervdonovan added a commit that referenced this pull request Oct 27, 2021
This will cause problems in Eclipse, but if Kotlin is an optional dependency, it does not make it into the uberjar.
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.

3 participants