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

Mavenize project and add Github actions #91

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tcornerstone
Copy link

The whole project has been restructured so that it can be built using Maven and Tycho. The name of the resulting JAR files contains the date of the last commit as well as in case of local builds, the suffix -UNOFFICIAL. Additionally, the project now supports Github actions, where the code is compiled and the resulting build artifacts can be downloaded. Builds triggered by pull requests contain the suffix -PR followed by the pull request id.

Copy link
Member

@haubi haubi left a comment

Choose a reason for hiding this comment

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

We can bump the Eclipse dependency (used as target platform), but then we need to perform a major revision bump, for being a "breaking change", requiring to bump the Eclipse release for existing setups.
However, given that we haven't updated to anything newer than Eclipse 2024-03 yet, we should stick to that one for now.

Otherways, good job, highly appreciated!

jobs:
build:

runs-on: ubuntu-latest
Copy link
Member

Choose a reason for hiding this comment

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

When aiming for "reproducible builds", we also need to avoid dependencies with unspecified version numbers.
In particular, "latest" explicitly talks about a moving target, which breaks any reproducability of the build.

jobs:
build:

runs-on: ubuntu-latest
Copy link
Member

Choose a reason for hiding this comment

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

Pinning the ubuntu version is even more important in the "official" build.

pom.xml Outdated
@@ -53,6 +54,7 @@
</pluginRepositories>

<build>
<finalName>foobar</finalName>
Copy link
Member

Choose a reason for hiding this comment

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

What is this finalName for?

pom.xml Outdated
</repository>
<repository>
<id>cbi-targetplatform-dsl-mirror</id>
<layout>p2</layout>
<url>https://download.eclipse.org/cbi/updates/tpd/release/3.0.0</url>
<url>https://download.eclipse.org/cbi/updates/tpd/release/</url>
Copy link
Member

Choose a reason for hiding this comment

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

Without the release number, this URL is a moving target as well, no?

pom.xml Outdated
@@ -261,12 +263,12 @@
<repository>
<id>eclipse</id>
<layout>p2</layout>
<url>http://download.eclipse.org/releases/2024-12</url>
<url>http://download.eclipse.org/releases/2024-06</url>
Copy link
Member

Choose a reason for hiding this comment

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

In regards of reproducible builds, on OCXConf I've learned that we should add the specific build date here as well.

<feature id="com.wamas.ide.launching.feature" version="0.0.0">
<category name="releng"/>
</feature>
<category-def name="releng" label="Eclipse CBI Release Engineering tools">
Copy link
Member

Choose a reason for hiding this comment

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

This label (and the description below) seems to indicate where you've copied from, no? ;)

pom.xml Outdated
<xtext-version>2.36.0</xtext-version>
<cbi-plugins.version>1.5.0</cbi-plugins.version>
<os-jvm-flags/>
<eclipse-repo.url>https://repo.eclipse.org/content/repositories/cbi/</eclipse-repo.url>
Copy link
Member

Choose a reason for hiding this comment

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

This URL, for not being a p2 repo, seems to be specific to Eclipse CBI projects, rather than being some dependency URL, no?

pom.xml Outdated
<repository>
<id>eclipse</id>
<layout>p2</layout>
<url>http://download.eclipse.org/releases/2024-12</url>
Copy link
Member

Choose a reason for hiding this comment

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

Depending on an Eclipse release newer than 2022-12 requires a major revision bump.
Feel free to add another commit doing the major revbump.

with source requirements
environment JavaSE-17

location "https://download.eclipse.org/eclipse/updates/4.32" {
Copy link
Member

Choose a reason for hiding this comment

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

We should use the repository that has the timestamp in the URL, if available, to increase reproducability of the build.
However, shouldn't we better use the Eclipse "release" here, rather than the "update"?

@haubi
Copy link
Member

haubi commented Nov 7, 2024

Let me invite @Bananeweizen to review this PR as well!

@haubi
Copy link
Member

haubi commented Feb 1, 2025

To test this PR in regards of workspace setup using Oomph installer, I've pushed it to the 'Playground' stream - and it breaks in multiple ways:

  • The first thing is that Target Platform selection requires version 2024-09, to allow even the Oomph setup to complete downloading target platform content.
  • Then, there are lots of compiler errors, as well as lots of unstaged files, shown in the Git Staging view.

To test the Oomph setup even without pusing to the 'playground' branch, it seems to work when you:

  • Select the 'Master' stream in Oomph Setup,
  • Change the git repo to point to your own repo,
  • Except for things you want to change within the .setup file if necessary, like the defaults of Target Platform and the JDK.

So please try to also get the Oomph workspace setup fixed.

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