-
Notifications
You must be signed in to change notification settings - Fork 7
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
Better build and installation support #209
base: master
Are you sure you want to change the base?
Conversation
Export-Package: edu.cuny.hunter.streamrefactoring.core.analysis, | ||
edu.cuny.hunter.streamrefactoring.core.contributions, | ||
edu.cuny.hunter.streamrefactoring.core.descriptors, | ||
edu.cuny.hunter.streamrefactoring.core.messages, | ||
edu.cuny.hunter.streamrefactoring.core.refactorings, | ||
edu.cuny.hunter.streamrefactoring.core.utils, | ||
edu.cuny.hunter.streamrefactoring.core.wala | ||
Import-Package: com.ibm.safe.controller, |
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.
What's wrong with Import-Package
?
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.
"Import-Package" means that this particular package should exist in the platform, but you are not specifying the plugin that provides it.
So it is a weakly declared dependency, in the sense you cannot actually import the package unless it is present in your platform (i.e. it has been installed).
Moving this dependency declaration to a "Require-Bundle" means that maven and eclipse p2 both understand the dependency and will deal with it appropriately (i.e. by deploying the plugin you depend on, and its own dependencies). So for a "managed" build it is recommended not to use "Import-Package", I removed mostly all of those.
The problem is then to find a "bundle" or plugin that provides the required package. We can resolve such artifacts in the update sites we provide to maven :
https://github.com/yanntm/Optimize-Java-8-Streams-Refactoring/blob/master/edu.cuny.hunter.streamrefactoring.parent/pom.xml#L109-L153
In this place I put the WALA and SAFE repositories, as well as point "orbit" which is an eclipse project providing quite a few common utilities packaged as bundles so we can use them in this kind of integration scenario. e.g. this is where we resolve dependencies on apache stuff https://download.eclipse.org/tools/orbit/downloads/drops/R20200529191137/
For instance this is where we now resolve org.apache.commons.csv dependencies (instead of packaging them ourselves in .eval plugin)
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.
Oh, interesting. I didn't know about that. Thanks!
org.eclipse.text, | ||
org.eclipse.ui, | ||
org.eclipse.pde.core, | ||
org.objenesis |
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.
How does this get here? I see that the jar is removed, but now it is being referenced as a plug-in. Where did the plug-in come from?
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 guess you mean the objenesis dependency.
The plugin is resolved directly in the Nexus central :
https://github.com/yanntm/Optimize-Java-8-Streams-Refactoring/blob/master/edu.cuny.hunter.streamrefactoring.parent/pom.xml#L149-L152
A lot of third-party artifacts are released there, among which is this objenesis dependency. https://mvnrepository.com/artifact/org.objenesis/objenesis
This is not as good as grabbing it from Orbit, because Orbit is a "simultaneous release" of components that are tested and interact well together and with the version of eclipse platform we are targetting. But it avoids at least repackaging our own, and lets us update automatically if a new version is released to maven central. It certainly is better than having a copy of the jar in our lib/ folder.
Thank you again for this contribution and for your interest in the project. We are currenly working on a new branch https://github.com/ponder-lab/Optimize-Java-8-Streams-Refactoring/tree/issue_192 to fix #192. Thus, integrating this into master now may be problematic, since there is parallel development. Also, some of the changes are a bit too drastic for the time bieng. I will attempt to integrate options of this PR once #192 is complete. Thanks again! |
I will keep this open for the time being. |
Ok sure, I'm still around if you need help adopting parts of the changes. Then in the feature definition : https://github.com/yanntm/Optimize-Java-8-Streams-Refactoring/blob/master/edu.cuny.hunter.streamrefactoring.feature/feature.xml#L236-L263 This eases and speeds up installation w.r.t. to using the "other update sites to visit" feature that you can configure, because client just contacts us + eclipse release update sites. This fulfills the packaging needs that were covered by e.g. nesting jars in the lib folder. |
i.e. we dropped these declarations :https://github.com/ponder-lab/Optimize-Java-8-Streams-Refactoring/blob/master/edu.cuny.hunter.streamrefactoring.feature/feature.xml#L225-L230 the resolution of dependencies is in our hands now not the client's, we do it with maven at build time and package everything we need. |
from Java 9 up, these files that are needed by Wala no longer exist in the JRE. This is an attempt to package them with the tool, see also ponder-lab#206
Then the selection could contain generic IProject instead of IJavaProject
Still working on ponder-lab#206
Congratulations 🎉. DeepCode analyzed your code in 4.11 seconds and we found no issues. Enjoy a moment of no bugs ☀️. 👉 View analysis in DeepCode’s Dashboard | Configure the bot |
Encountered : !MESSAGE The Plug-in Development Environment requires the PlatformAdmin service to operate. Please install the compatibility fragment 'org.eclipse.osgi.compatibility.state'. Jun 21, 2020 6:48:15 PM edu.cuny.streamrefactoring.core.plugin.StreamRefactorCorePlugin
with: | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
BRANCH: gh-pages # The branch the action should deploy to. | ||
FOLDER: edu.cuny.hunter.streamrefactoring.updatesite/target/repository # The folder the action should deploy. |
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.
So, the updatesite is "updated" on each build instead of, let's say, major/minor releases?
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.
yes, but if you're considering merging this I should update to "modern" versions for a few of these, the syntax has evolved a bit see
lip6/ITS-commandline@e782ddc#diff-5c3fa597431eda03ac3339ae6bf7f05e1a50d6fc7333679ec38e21b337cb6721R45-R52
for an example
|
||
script: | ||
- cd edu.cuny.hunter.streamrefactoring.parent && mvn -e generate-sources install && cd - | ||
- du -sh edu.cuny.hunter.streamrefactoring.updatesite/target/repository |
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 also saw this in the GH actions build ...
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.
the du is just because sometimes upload might fail a bit silently if the files are too large, it also makes sure it's about the right size (some MB) so it was (maybe) built correctly
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.
This travis file is obsolete, they stopped offering free service for open source projects so I now only use GH actions. So this file can be deleted it is overridden by the Gh workflow file.
See issue #208 for discussion.