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

set minimum required jdk11 in pom.xml - fix jitpack.io build #139

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

mrgreywater
Copy link

Installs node and npm automatically if missing.

This fixes the previously broken jitpack.io build, allowing to directly fetch a compiled nightly build with maven after adding the jitpack repository.

Example usage in pom.xml

        <repository>
            <id>jitpack.io</id>
            <url>https://jitpack.io</url>
        </repository>
        <dependency>
            <groupId>com.github.appreciated</groupId>
            <artifactId>apexcharts-flow</artifactId>
            <version>-SNAPSHOT</version>
        </dependency>

@TFyre
Copy link
Collaborator

TFyre commented Sep 8, 2022

I dont think this should be the default behavior.

When you are using quarkus+vaadin+apexcharts, the quarkus-vaadin environment already takes care of installing and maintaining your node environment.

This should most likely be configured using a profile.

What do you think?

@mrgreywater
Copy link
Author

Does it break the build for quarkus-vaadin environment like this?

@TFyre
Copy link
Collaborator

TFyre commented Sep 8, 2022

When Im compiling apexcharts-flow project, I dont need this.

Im not sure what problem you are trying to solve or what issues you are facing. Perhaps you can include some more detail why you need this. Sounds like you are having problems outside of this repo and perhaps should fix it there?

If this is really required, I would be more supportive of using the vaadin-maven-plugin plugin, as thats provided & maintained by vaadin

@mrgreywater
Copy link
Author

mrgreywater commented Sep 8, 2022

The problem is, on a vanilla system - without node installed - this project will not compile as is. This PR is necessary to make it build on CI systems such as jitpack.io. jitpack.io allows to include this github repo as maven dependency (any commit / branch) without having to compile it yourself or having to push it to the official maven repository.

Here is the compilation log for this repository without the PR: https://jitpack.io/com/github/appreciated/apexcharts-flow/master-c72b288e4b-1/build.log

As you can see, the vaadin-maven-plugin suggests this change itself in the log.

Unless this breaks some other usecase, I don't see the reason not to make the changes.

@TFyre
Copy link
Collaborator

TFyre commented Sep 8, 2022

Here is the compilation log for this repository without the PR: https://jitpack.io/com/github/appreciated/apexcharts-flow/master-c72b288e4b-1/build.log

The log references a commit from 2020

Build starting...
Start: Thu Apr 2 21:25:50 UTC 2020 cfe16a77fe0c
...
[INFO] Building ApexCharts.js 2.0.0.beta9

Here is a more recent log that looks fine: https://jitpack.io/com/github/appreciated/apexcharts-flow/2.0.0-beta13/build.log

https://jitpack.io/#appreciated/apexcharts-flow
image

Unless this breaks some other usecase, I don't see the reason not to make the changes.

I cant see any reason to include this with the logs/reasons provided.

@TFyre
Copy link
Collaborator

TFyre commented Sep 8, 2022

I can see what the problem is...

Build failed against current master (with jdk1.8)
https://jitpack.io/com/github/appreciated/apexcharts-flow/master/build.log

Build successful against current master + jdk11 changes
https://jitpack.io/com/github/TFyre/apexcharts-flow/a83b271ed3/build.log

Perhaps you can update the PR to only reflect jdk1.8 to jdk11 change?

@mrgreywater
Copy link
Author

You're right, seems like just updating to java 11 was enough for it to run properly. Cherry picked your commit instead.

@mrgreywater mrgreywater changed the title automatically install node and npm - fix jitpack.io build set minimum required jdk11 in pom.xml - fix jitpack.io build Sep 9, 2022
@TFyre TFyre merged commit 0de9672 into appreciated:master Sep 9, 2022
@TFyre TFyre mentioned this pull request Sep 12, 2022
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