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

Maven #372

Closed
wants to merge 3 commits into from
Closed

Maven #372

wants to merge 3 commits into from

Conversation

vogella
Copy link
Collaborator

@vogella vogella commented Sep 17, 2021

This is a first step in the direction of Maven Tycho. We could also use the libraries directly with the PDE m2e integration. Is that ok?

Is asp-server-asciidoctorj-launcher-dist.jar available at Maven central?

Description Resource Path Location Type
Project 'asciidoctor-editor-plugin' is missing required library: 'lib/asp-server-asciidoctorj-launcher-dist.jar' asciidoctor-editor-plugin Build path Build Path Problem

vogella and others added 3 commits September 17, 2021 11:21
Currently not building anything just setting up the build and Github
action
Using the target platform during the Maven build and build the css
plug-in

For de-jcup#371
@de-jcup
Copy link
Owner

de-jcup commented Sep 17, 2021

This is a first step in the direction of Maven Tycho. We could also use the libraries directly with the PDE m2e integration. Is that ok?

The m2e is (afaik) the eclipse maven integration and very mature - you are more experienced here and I think this is a good option.

Is asp-server-asciidoctorj-launcher-dist.jar available at Maven central?

Description Resource Path Location Type
Project 'asciidoctor-editor-plugin' is missing required library: 'lib/asp-server-asciidoctorj-launcher-dist.jar' asciidoctor-editor-plugin Build path Build Path Problem

ASP is (just) now available at maven central - see de-jcup/asp#38 and/or
https://repo1.maven.org/maven2/de/jcup/asp/

@de-jcup de-jcup linked an issue Sep 17, 2021 that may be closed by this pull request
Copy link
Owner

@de-jcup de-jcup left a comment

Choose a reason for hiding this comment

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

Hello Lars, thanx for contributing!

have some open questions etc. Please read my comments. At least the drop of gradle.yml is something that must postponed as long as the maven build does not provide test results from "normal" junit tests and the README still references the badge from gradle.yml

uses: actions/setup-java@v2
with:
distribution: 'adopt'
java-version: '11'
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm.. currently I provide even very old eclipse versions with the plugin and support normally min JDK8.

There was even an issue where a user wanted still support for eclipse photon... see #316

Do you think it's a good idea to go to SDK 11 here?

Copy link
Owner

Choose a reason for hiding this comment

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

Having no option to comment on the deleted gradle.yml workflow: As long as the build with maven does not work and "normal" tests are not run by maven as well I would keep the old gradle build - it's generated badges are used inside the README in the root folder as well - so the README must be changed after full maven support as well to use the correct github action badge.

So please revert the delete - this time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As we move this to a branch, we should IMHO still delete the gradle file if it fails the verfication. I test this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Java 11 is required by Tycho since a while. It still can build plug-ins using BREE smaller than Java 11.

@@ -633,7 +633,7 @@
<page
id="asciidoctoreditor.eclipse.preferences.AsciiDoctorEditorEnvironmentPreferencePage"
class="de.jcup.asciidoctoreditor.preferences.AsciiDoctorEditorEnvironmentPreferencePage"
name="Environmemt"
name="Environment"
Copy link
Owner

Choose a reason for hiding this comment

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

Thanx.

<modelVersion>4.0.0</modelVersion>
<groupId>eclipse.asciidoctor.editor</groupId>
<artifactId>releng</artifactId>
<version>1.0.0-SNAPSHOT</version>
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please change to 2.5.0-SNAPSHOT or 2.4.2-SNAPSHOT ? Current version is 2.4.1 on eclipse marketplace.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do but IMHO this is not really important, as this pom file will not create artifacts.

</pluginRepositories>

<properties>
<tycho.version>2.5.0-SNAPSHOT</tycho.version>
Copy link
Owner

Choose a reason for hiding this comment

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

Ahem - is this another version (the plugin version) - I am a little bit confused. My comment before was about the versions, but maybe this was only the POM version and this one the tycho plugin version ...

Hmm.. to keep things simple I would normally prefer to keep both versions in sync, so it would be always clear which pom is for which eclipse plugin version...

What's your opinion?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the Tycho version used for the build, so we should not have that in sync with the Gradle editor version

@vogella
Copy link
Collaborator Author

vogella commented Sep 28, 2021

Lets move this to a new branch "Maven". We can work out the details here.

@vogella vogella closed this Sep 28, 2021
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