-
Notifications
You must be signed in to change notification settings - Fork 1
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
Modifications to show assurance cases in Eclipse Views #169
base: main
Are you sure you want to change the base?
Conversation
tools/rack/pom.xml
Outdated
<goals> | ||
<goal>apply</goal> | ||
</goals> | ||
<goals></goals> |
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.
Wondering why this change was made. I assume this change falls back to the default goal (which may be the same as apply)?
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.
Oops - a local change snuck into the commit (this task does not work in my environment). Reverted.
@@ -42,7 +42,8 @@ Require-Bundle: com.ge.research.jena, | |||
org.eclipse.ui.forms, | |||
org.slf4j.api, | |||
org.yaml.snakeyaml, | |||
org.eclipse.debug.ui | |||
org.eclipse.debug.ui, | |||
openjfx.swing;bundle-version="17.0.2" |
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 surprised this requirement doesn't cause any error. We are explicitly adding the javafx jars to this bundle's classpath instead of requiring Eclipse to find the javafx bundles in its P2 repositories. At best, this requirement is unnecessary; at worst, it may be muddling where the swing classes come from (are they coming from a bundle in P2 or a jar in the classpath?). We should remove this requirement since RITE still should work without it.
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.
Reverted the change. I suspect Eclipse inserted it.
lib/javafx-swing.jar, | ||
lib/javafx-swing-linux.jar, | ||
lib/javafx-swing-mac.jar, | ||
lib/javafx-swing-win.jar |
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.
Please keep all the jars sorted alphabetically. This helps maintainers check for missing jars which are in the lib directory but not on the bundle's classpath (note we do omit some jars from the lib directory on purpose, but still).
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.
Fixed.
lib/javafx-swing.jar,\ | ||
lib/javafx-swing-linux.jar,\ | ||
lib/javafx-swing-mac.jar,\ | ||
lib/javafx-swing-win.jar |
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.
Likewise, please keep these filenames sorted alphabetically so maintainers can check for missing files more easily.
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.
Fixed.
tools/rack/rack.plugin/pom.xml
Outdated
@@ -42,115 +42,162 @@ | |||
<dependency> | |||
<groupId>org.openjfx</groupId> | |||
<artifactId>javafx-base</artifactId> | |||
<version>${version.javafx}</version> |
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.
All of these newly added versions are redundant because the parent pom already has the complete information (including versions) in its dependencyManagement section. This allows the child pom to "inherit" the version from the parent pom.
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.
Removed them.
Updated with a merge from main and pushed changes to the PR |
Need to test on windows before merging |
Fixes Issue #139