-
Notifications
You must be signed in to change notification settings - Fork 12
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
Updated to Xtext 2.23.0 #532
Conversation
@kittaakos , what's the status of this. I think you're still working on it, true? |
Yes. Work in progress. |
4fe9d77
to
145b3d7
Compare
ded66f3
to
86955e2
Compare
@crapo, although this PR is work in progress, I am done with the Eclipse and p2 part. I did the verification of the SADL p2 update in the following Eclipse installations:
All of them worked. Please check the changeset, I am still struggling with the Theia dependencies and the Docker image. I am hoping to finish it today. Let me know if you have any questions. Thank you! |
<includes | ||
id="org.eclipse.xtext.xtext.ui" | ||
version="0.0.0"/> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked over the changeset quickly and I think it looks great, especially the removal of the xtext version ranges from the MANIFEST.MF files. That will make it easier to build and/or import SADL into future Xtext/Eclipse platforms.
I'm curious about this particular change. What is the purpose of this change? Is the purpose to bundle the Xtext bundles that SADL was built with so that SADL can be installed into an Eclipse platform which doesn't have any Xtext bundles at all? Or is the purpose to bundle the Xtext bundles that SADL was built with so that SADL can be installed into an Eclipse platform which has Xtext bundles older than 2.14.0 and therefore SADL needs to bring its own Xtext bundles just in case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about this particular change. What is the purpose of this change?
Thanks for asking. Let assume you have an Eclipse 2020-03 for DSL developers:
With this additional include
(Xtext) in the manifest, when installing the SADL p2 into Eclipse 2020-3 DSL, it will automatically force updating the Xtext version shipped with Eclipse:
So once the SADL p2 installation is done, you will see the Xtext version update in the underlying platform:
If we do not force the Xtext version update when installing the SADL p2, we might run into incompatible APIs, causing a runtime error. Such as this.
Note, if you install the SADL p2 into any Eclipse for Java developers, it will just make Xtext only dependent on SADL:
It's ready for review. I tried the WebSADL application locally, it worked with the new theme and the rewritten coloring. I also managed to put together a Docker image locally and build it, but since the CI is down you won't be able to try out the latest state with |
Made compatible with SemanticApplicationDesignLanguage/sadl#532. Closes #129 Signed-off-by: Akos Kitta <[email protected]>
@kittaakos , I did some testing on this branch and only found one issue. In the attached project, if I have both .sadl files open in the editor and I rename "Man" to "MaleMan", I get an error in George.sadl. If I make an edit or close and reopen George.sadl the error is gone, but until then it shows an error and MaleMan as undefined.. |
@agabaldon , I have created an update ZIP file on this branch and installed it into Eclipse 2020-09 for Java Developers as well as into Eclipse 2020-03 for Java Developers and with the exception of the rename error mentioned here, everything seems fine. As soon as there is a release of SADL 3.4.1 that resolves the javax.activation.DataSource issue I recommend merging this to development branch and preparing for a major release that removes the pin to Xtext 2.21 and Eclipse 2020-03. I recommend that this major release be labeled SADL version 3.5.0. |
Does the same rename work correctly from the HEAD of the |
The rename seems to work correctly in the HEAD of the development branch. During the renaming process the second file has errors but as soon as it is finished the errors go away instead of persisting until something happens to cause it to refresh. |
Thank you! If it works on |
I can confirm the regression. Unfortunately, I have no idea what's causing it. Most likely the heavy rename/refactoring customization we did for DARPA does not work well with the most recent Xtext version. |
What is the status of this pull request? I know there are conflicts that will need to be resolved because of changes to how CI publishes WebSADL, but is there any reason why not to resolve the conflicts and merge the PR now? |
Revers commit of #492. [f4db832] Signed-off-by: Akos Kitta <[email protected]>
- Also bumped up MWE to `2.11.3`, - Explicitly depend on Xtext in the SADL feature. Signed-off-by: Akos Kitta <[email protected]>
Signed-off-by: Akos Kitta <[email protected]>
Signed-off-by: Akos Kitta <[email protected]>
`bundle-version="2.14.0"` changes were auto-generated in the manifests. Signed-off-by: Akos Kitta <[email protected]>
Signed-off-by: Akos Kitta <[email protected]>
Signed-off-by: Akos Kitta <[email protected]>
Signed-off-by: Akos Kitta <[email protected]>
86955e2
to
d700705
Compare
Resolved the conflicts and merged this PR. CI will build a new WebSADL Docker image and someone will need to test it. |
This PR is intended to close #506.