Skip to content
This repository has been archived by the owner on Jun 24, 2021. It is now read-only.

Add Gradle wrapper files. Relates to #23 #26

Merged
merged 2 commits into from
Mar 23, 2018
Merged

Add Gradle wrapper files. Relates to #23 #26

merged 2 commits into from
Mar 23, 2018

Conversation

aalmiray
Copy link
Contributor

Signed OCA sent on March 15th, confirmation pending.

@brcolow
Copy link
Contributor

brcolow commented Mar 19, 2018

This should also include changing .travis.yml and appveyor.yml to use the wrapper instead of installing and calling gradle.

@bourgesl
Copy link
Contributor

Appveyor CI is still failing. See #24 for fixes

@bourgesl
Copy link
Contributor

So changing appveyor.xml will be in conflict with #24 changes

@hendrikebbers
Copy link

LGTM

.ci/script.sh Outdated
@@ -7,7 +7,7 @@ BOOT_JDK10="jdk-10-ea+${BOOT_JDK10_VER}"
echo "which java: $(which java)"
ulimit -c unlimited -S

gradle build -x :web:test --no-daemon --stacktrace --info
./gradlew build -x :web:test --no-daemon --stacktrace --info
Copy link
Collaborator

Choose a reason for hiding this comment

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

Upstream repo does not allow files with execute bit set, so will likely need to change this to something like:

sh ./gradlew ...

@kevinrushforth
Copy link
Collaborator

I like the idea of enabling gradlew. Since this is 3rd-party code, it will require approval before being committed upstream. I will do a quick check to see if this is likely to be approved.

@aalmiray
Copy link
Contributor Author

Rebased this PR with the changes coming from #24 that were merged into master.
Also, gradlew no longer has the executable bit set.

@johanvos johanvos changed the base branch from master to develop March 22, 2018 11:34
@kevinrushforth
Copy link
Collaborator

I verified that we have the needed third-party approval to include gradle wrapper. As with other third-party code, I need to approve it (which I will) and we need to add a gradle.md file, which will go in a new gradle/legal directory. I will provide the contents of this file so you can add it into your PR.

@kevinrushforth
Copy link
Collaborator

This is being tracked in JBS by: https://bugs.openjdk.java.net/browse/JDK-8199841

@aalmiray
Copy link
Contributor Author

Thanks @kevinrushforth!

@johanvos
Copy link
Collaborator

Any idea why appveyor fails on this PR?

@johanvos johanvos mentioned this pull request Mar 23, 2018
@brcolow
Copy link
Contributor

brcolow commented Mar 23, 2018

@johanvos The Appveyor build is failing because it is running against master and #32 was not merged to master. If you look at the Appveyor history: https://ci.appveyor.com/project/javafxports-github-bot/openjdk-jfx/history the PRs against the develop branch are successful. Why this PR is running against master when GitHub shows that it is trying to merge into develop I don't know. I only have experience with using Appveyor to run builds against master, so I am not sure how to tell Appveyor that PRs should be built against develop instead (and why aalmiray wants to merge 2 commits into javafxports:develop from aalmiray:gradle-wrapper is not enough for it to figure it out).

@johanvos
Copy link
Collaborator

I rebased the PR to develop, but maybe that didn't automatically re-run the appveyor tests.

@brcolow
Copy link
Contributor

brcolow commented Mar 23, 2018

I see some mention of a "default branch" on the Appveyor docs..setting this to develop may alleviate having to manually rebase every PR and restart the builds manually.

@bourgesl
Copy link
Contributor

Appveyor.xml seems complete and corresponding to PR#24, the only difference is gradleNew vs gradle !

Someone should read the complete log and maybe CLI args use an alternative syntax ?

@bourgesl
Copy link
Contributor

@johanvos if you log in on appveyor with admin account, you can restart / cancel builds.
Who has admin rights on the repository ?
Maybe we should contact this person

@brcolow
Copy link
Contributor

brcolow commented Mar 23, 2018

Appveyor.xml seems complete and corresponding to PR#24, the only difference is gradleNew vs gradle !

The reason has nothing to do with this PR, it is because Appveyor is building against master which doesn't have the fixes from #32 - Appveyor should always build PRs against the develop branch, which can be configured via Appveyor settings (not sure if there is a way to configure it using appveyor.yml or if it can only be done using web UI).

@aalmiray
Copy link
Contributor Author

@brcolow would this mean merge #32 first then rebase this PR?

@bourgesl
Copy link
Contributor

@johanvos could you change the default branch to develop on github settings ?

@brcolow
Copy link
Contributor

brcolow commented Mar 23, 2018

@aalmiray #32 has already been merged into the develop branch. Appveyor just needs to properly build against that branch instead of master.

@javafxports-github-bot
Copy link
Contributor

@bourgesl I've updated the default branch to develop.

@johanvos
Copy link
Collaborator

changing the default branch didn't trigger appveyor to run again.
@aalmiray Maybe you should make a very small change to the PR, so that tests will be run again?

@aalmiray
Copy link
Contributor Author

Rebased gradle-wrapper branch against develop @ f79c724

@johanvos
Copy link
Collaborator

Checks passed, merging this.

@johanvos johanvos merged commit 44a0e12 into javafxports:develop Mar 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants