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

Add a powershell script for Windows dev, use on Appveyor. #153

Merged
merged 3 commits into from
Aug 21, 2018

Conversation

brcolow
Copy link
Contributor

@brcolow brcolow commented Aug 10, 2018

The script tools\scripts\build.ps1 works for locally building OpenJFX without any additional environment setup as long as one has the choco package manager installed. It automatically finds the location of the latest installed versions of Visual Studio and the Windows SDK and sets the necessary environment variables so that the gradle build works out-of-the-box without requiring somewhat tedious manual setup/configuration.

@@ -0,0 +1,15 @@
choco install ant
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be the first time a "github-only" file showed up in this directory. I'd like to limit the 'github-only' files to as few locations as possible. This raises an interesting question of whether these sort of build files should eventually be committed to the HG openjdk repo, in which case it wouldn't matter.

Pending that more global discussion, perhaps there is a better place to put it? In a new appveyor dir maybe? Not sure (would like to think about this for more time than I have today).

Maybe one of the Gluon folks have some thoughts on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a non-trivial question. In general, I am leaning towards separating code and build logic, since there might be multiple usages of the same source.
In this case however, the file is intended to be added for test purposes, hence I think it makes sense to do it here.
It is also not a github specific file, so I think it makes sense adding it to the hg repository as well.

@kevinrushforth
Copy link
Collaborator

In that case, I am OK with this as long as we can keep the changes that do not currently go back to the HG repo on OpenJDK (the appveyor.yml script) separate from those that do (files the tools/scripts dir). Can you file a JBS bug to add the build.ps1 script?

This brings up a point I've been thinking about for a while. We really should eliminate (or at least minimize) any differences between the HG repo and the master branch on GitHub.

I'll file a JBS issue to consider unifying them.

@kevinrushforth kevinrushforth added the enhancement New feature or request label Aug 20, 2018
@brcolow
Copy link
Contributor Author

brcolow commented Aug 20, 2018

https://bugs.openjdk.java.net/browse/JDK-8209765

I maybe should not have mentioned the appveyor stuff, as that isn't in the upstream repo. But I don't think that extra verbiage should hurt anything.

@kevinrushforth
Copy link
Collaborator

Thanks for filing the bug. That's fine about mentioning appveyor (I also added a comment for those looking at it indicating that the appveyor.yml file will be excluded from the openjfx/jfx-dev/rt changeset).

I fixed up a couple things, assigned it to you, and linked it to this PR (by adding the github-bug label and adding an issue Link to the URL of this PR).

Can you send a review request to openjfx-dev with a pointer to this PR so any other interested party can also review it?

@kevinrushforth
Copy link
Collaborator

kevinrushforth commented Aug 21, 2018

As long as the Appveyor build is passing with the new script, it looks fine to me.

Edited to add: see below for one recommended change.

$env:VS150COMNTOOLS = $env:VCINSTALLDIR
$env:VSVARS32FILE = "$env:VCINSTALLDIR\vcvars32.bat"
refreshenv
.\gradlew all test -PCOMPILE_WEBKIT=false -PCONF=DebugNative --stacktrace -x :web:test --info
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend adding --no-daemon which was present in the appveyor.yml file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For CI builds I wanted to use --no-daemon. For local builds it makes sense to use the daemon IMO. So in the latest commit we check if APPVEYOR environment variable is set. If so, use --no-daemon. Otherwise, don't. Is that okay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I broke it, let me fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, strange, it seems that the downloading of OpenJDK 10 is what failed. Let me see.

Copy link
Collaborator

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

Looks fine now. I'll merge it once the CI builds pass.

@kevinrushforth
Copy link
Collaborator

I see the following from the Appveyor log (looks like it is a dependency of ant), but it doesn't look like this is new:

jdk8 v8.0.181 [Approved]
jdk8 package files install completed. Performing other installation steps.
Downloading JDK from http://download.oracle.com/otn-pub/java/jdk/8u181-b13/96a7b8442fe848ef90c96a2fad6ed6d1/jdk-8u181-windows-x64.exe
Installing jdk8...
jdk8 has been installed.
PATH environment variable does not have C:\Program Files\Java\jdk1.8.0_181\bin in it. Adding...
  jdk8 may be able to be automatically uninstalled.
Environment Vars (like PATH) have changed. Close/reopen your shell to
 see the changes (or in powershell/cmd.exe just type `refreshenv`).
 The install of jdk8 was successful.
  Software installed to 'C:\Program Files\Java\jdk1.8.0_181\'

In any event, it isn't using JDK 8, so it's not a problem. I'll merge the PR now.

@kevinrushforth kevinrushforth merged commit 34f8c83 into javafxports:develop Aug 21, 2018
kevinrushforth pushed a commit that referenced this pull request Aug 23, 2018
JDK-8209765: Add powershell build script for OpenJFX
Reviewed-by: @kevinrushforth
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants