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

include JRE in bundles + upgrade tycho v4.0.4 #42

Merged
merged 1 commit into from
Dec 22, 2023
Merged

include JRE in bundles + upgrade tycho v4.0.4 #42

merged 1 commit into from
Dec 22, 2023

Conversation

titou10titou10
Copy link
Contributor

include JRE in bundles + upgrade tycho v4.0.4

I had to comment"<timestampProvider>jgit</timestampProvider>" in com.eclipsesource.megit.parent\pom.xmlto have it to compile
Please check it

Copy link
Collaborator

@planger planger left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution! Code looks good and the build works just fine!

However, I didn't see where the JRE is now included? After mvn clean install the size of the generated archives didn't seem to have increased and there is no vm specified in the ini file.
Or do I misunderstand how justj should now be packaged alongside the application?

Thank you!

@@ -115,20 +121,22 @@
<artifactId>tycho-buildtimestamp-jgit</artifactId>
<version>${tycho-extras-version}</version>
</dependency>
<!--
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the outcommented code.

<timestampProvider>jgit</timestampProvider>
-->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove the outcommented code entirely please.

@titou10titou10
Copy link
Contributor Author

titou10titou10 commented Dec 22, 2023

oh?
I just cloned my repo used for this PR, with the changes forked from yours (https://github.com/titou10titou10/megit.git) on ubuntu, thenmvn install and everything is fine
The 5 products have the correct"vm"in their respective"eclipse.ini"file and the respective JRE plugin is bundled in the "plugin"directory with the other plugins and the size of the products is around 230M...

Windows:

-vm
plugins/org.eclipse.justj.openjdk.hotspot.jre.full.win32.x86_64_17.0.9.v20231028-0858/jre/bin

macosx cocoa aarch64:

-vm
../Eclipse/plugins/org.eclipse.justj.openjdk.hotspot.jre.full.macosx.aarch64_17.0.9.v20231028-0858/jre/lib/libjli.dylib

So everything looks fine on my side

The JRE is included as a plugin, identical to what you get when you download "Eclipse IDE for Enterprise Java and Web Developers" or "Eclipse IDE for RCP and RAP Developers" (the one I use for RCP/plugin development)

Basically what I've done vs your code:

  • upgraded tycho version to v4.x
  • added "justj" in the target definition
  • changed your product from "features only" to "features+plugins", to include the justj plugin
  • added "justj" as a plugin dependency in your product
  • in your parent pom, added the repository to download justj and pointed the"executionEnvironment"to justj

=============

Also, thanks for this project and your great work but IMHO, the structure of the project could be simplified:

  • remove thecom.eclipsesource.megit.target project (it contains only one useful file):

    • move thecom.eclipsesource.megit.target\com.eclipsesource.megit.target.targetfile to project`com.eclipsesource.megit.parent

    • (optional) rename com.eclipsesource.megit.target.target tomegit.target

    • in com.eclipsesource.megit.parent\pom.xml, reference the eclipse target with

      <target>
         <file>com.eclipsesource.megit.target.target</file>
      </target>
      
  • remove thecom.eclipsesource.megit.product.feature project (does not add anything as it only includes one plugin):

    • direcly include the com.eclipsesource.megit.plugin plugin into your product

...but that's another story

Copy link
Collaborator

@planger planger left a comment

Choose a reason for hiding this comment

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

Thanks, hm, strange! I've retested in a container and now it works as advertised. Thank you!

@planger planger merged commit ba3386d into eclipsesource:master Dec 22, 2023
@planger
Copy link
Collaborator

planger commented Dec 22, 2023

Regarding your other suggestions! Yeah, sure, these are good proposals, feel free to open a separate issue and/or a PR.

@titou10titou10
Copy link
Contributor Author

titou10titou10 commented Dec 22, 2023

cool
as for uncommenting the tycho-buildtimestamp-jgitandtycho-sourceref-jgit blocks, tycho-buildtimestamp-jgitmoved from extrastotycho-core

ref: eclipse-tycho/tycho#915

So you could have:

    <tycho-version>4.0.4</tycho-version>
    <tycho-extras-version>4.0.4</tycho-extras-version>

    {...}

    <dependency>
        <groupId>org.eclipse.tycho</groupId>
        <artifactId>tycho-buildtimestamp-jgit</artifactId>
         <version>${tycho-version}</version>
    </dependency>
    <dependency>
        <groupId>org.eclipse.tycho.extras</groupId>
        <artifactId>tycho-sourceref-jgit</artifactId>
        <version>${tycho-extras-version}</version>
     </dependency>

then

    <configuration>
        <format>yyyyMMdd-HHmm</format>
        <sourceReferences>
            <generate>true</generate>
            <customValue>scm:git:https://github.com/eclipsesource/megit.git</customValue>
       </sourceReferences>
       <timestampProvider>jgit</timestampProvider>
       <jgit.ignore>
           pom.xml
       </jgit.ignore>
       <jgit.dirtyWorkingTree>ignore</jgit.dirtyWorkingTree>
    </configuration>

or add

    <properties>
        <tycho.scmUrl>scm:git:https://github.com/eclipsesource/megit.git</tycho.scmUrl>
    </properties>

...and remove the<customValue> tag

ref: https://wiki.eclipse.org/PDE/UI/SourceReferences

@planger
Copy link
Collaborator

planger commented Dec 22, 2023

Thanks @titou10titou10! It'd be great, if you could open a PR with that proposal(s). I've finally took some time to add CI so it'll be less work to review and merge things \o/
Thank you!

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.

2 participants