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

Issue #3783: Fix CI check builds for windows platform #3785

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions .github/scripts-windows/run-tests-cpp.cmd
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
set ChocolateyInstall=C:\tools\chocolatey
C:\ProgramData\chocolatey\bin\cinst.exe visualstudio2022-workload-vctools -y

cd runtime-testsuite
mvn -Dtest=cpp.** test
cd ..
2 changes: 1 addition & 1 deletion .github/scripts-windows/run-tests-python2.cmd
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
cd runtime-testsuite
mvn -Dantlr-python2-exec="C:\Python27\python.exe" -Dtest=python2.** test
mvn -Dantlr-python2-exec="%PYTHON_HOME%\python.exe" -Dtest=python2.** test
Copy link
Contributor

Choose a reason for hiding this comment

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

I disapprove this change.
This runs in an always on docker instance with both Python2 and Python3 installed
The python version at PYTHON_HOME is unknown

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Having a hard coded path is not portable. Other potential options -

  • Get python in the %PATH% variable and use just python.exe i.e. without the qualifying full path
  • Update docker to setup the PYTHON_HOME variable
  • Use different environment variables for Python2 and Python3, and update docker image to setup the variables accordingly

Feel free to just alternatives if you have any.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this to be portable, currently it only runs on 4 docker instances on a physical box in my office

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A hardcoded path won't wok on Github cloud though. Python is not installed on that path.

Just may be - We might be on the different page here. Please follow comments on #3783 if you haven't yet.

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to use just different environment variables %PYTHON2_HOME% and %PYTHON3_HOME% to make it workable everywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. @ericvergnaud Would that resolve your concern?

Copy link
Contributor

Choose a reason for hiding this comment

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

It might, but we don't need to solve problems we don't have. The existing scripts work just fine, they're just not portable, which is not something we need at this point.
Sorry for the tone, it's great that you've gotten the cpp build to run on Windows. I suggest you restrict this PR to just that.

cd ..
2 changes: 1 addition & 1 deletion .github/scripts-windows/run-tests-python3.cmd
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
cd runtime-testsuite
mvn -Dantlr-python3-exec="C:\Python310\python.exe" -Dtest=python3.** test
mvn -Dantlr-python3-exec="%PYTHON_HOME%\python.exe" -Dtest=python3.** test
Copy link
Contributor

Choose a reason for hiding this comment

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

I disapprove this change.
This runs in an always on docker instance with both Python2 and Python3 installed
The python version at PYTHON_HOME is unknown

cd ..
181 changes: 145 additions & 36 deletions .github/workflows/windows.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,69 +7,178 @@ on:
branches: [ dev ]

jobs:
cpp-builds:
name: "windows-cpp (${{ matrix.compiler }})"
runs-on: windows-latest
strategy:
fail-fast: false
matrix:
compiler: [ cl, clang ]

steps:
- name: Setup Clang
if: matrix.compiler == 'clang'
uses: egor-tensin/setup-clang@v1
with:
version: 13
platform: x64
cygwin: 0

- name: Check out code
uses: actions/checkout@v2

- name: Build
shell: cmd
run: |
call "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvars64.bat"

if "${{ matrix.compiler }}" EQU "cl" (
echo 'CC=cl' >> $GITHUB_ENV
echo 'CXX=cl' >> $GITHUB_ENV
) else (
echo 'CC=clang' >> $GITHUB_ENV
echo 'CXX=clang++' >> $GITHUB_ENV
)

set
where cmake && cmake --version
where java && java -version
where ninja && ninja --version
where %CC% && %CC% -version
where %CXX% && %CXX% -version

cd runtime/Cpp

cmake -G Ninja -DCMAKE_BUILD_TYPE=Debug -DANTLR_BUILD_CPP_TESTS=OFF -S . -B out/Debug
if %errorlevel% neq 0 exit /b %errorlevel%

cmake --build out/Debug -j %NUMBER_OF_PROCESSORS%
if %errorlevel% neq 0 exit /b %errorlevel%

cmake -G Ninja -DCMAKE_BUILD_TYPE=Release -DANTLR_BUILD_CPP_TESTS=OFF -S . -B out/Release
if %errorlevel% neq 0 exit /b %errorlevel%

cmake --build out/Release -j %NUMBER_OF_PROCESSORS%
if %errorlevel% neq 0 exit /b %errorlevel%

- name: Prepare artifacts
if: always()
run: |
cd ${{ github.workspace }}\..
tar czfp C:\antlr-windows-cpp-${{ matrix.compiler }}.tgz ${{ github.workspace }}

- name: Archive artifacts
if: always()
uses: actions/upload-artifact@v2
with:
name: antlr4-windows-cpp-${{ matrix.compiler }}
path: C:\antlr-windows-cpp-${{ matrix.compiler }}.tgz


build:
runs-on: [self-hosted, windows, x64]
runs-on: windows-latest

strategy:
fail-fast: false
matrix:
TARGET: [java, python2, python3, javascript, csharp, dart, go, php]
TARGET: [
cpp,
csharp,
dart,
go,
java,
javascript,
python2,
python3,
php,
hs-apotell marked this conversation as resolved.
Show resolved Hide resolved
]

steps:
- name: Check out code
uses: actions/checkout@v2

- name: Set up JDK 11
uses: actions/setup-java@v1
uses: actions/setup-java@v3
with:
distribution: 'zulu'
java-version: 11

- name: Set up Maven
uses: stCarolas/setup-maven@v4
uses: stCarolas/setup-maven@v4.4
with:
maven-version: 3.5.4
# fails due to permissions, use global install
# - name: Set up Python 2
# if: matrix.TARGET == 'python2'
# uses: actions/setup-python@v2
# with:
# python-version: '2.x'
# architecture: 'x64'
# fails due to permissions, use global install
# - name: Set up Python 3
# if: matrix.TARGET == 'python3'
# uses: actions/setup-python@v2
# with:
# python-version: '3.x'
# architecture: 'x64'

- name: Add msbuild to PATH
if: matrix.TARGET == 'cpp'
uses: microsoft/setup-msbuild@v1.1

- name: Set up Python 2
if: matrix.TARGET == 'python2'
uses: actions/setup-python@v4
with:
python-version: '2.x'
architecture: 'x64'

- name: Set up Python 3
if: matrix.TARGET == 'python3'
uses: actions/setup-python@v4
with:
python-version: '3.x'
architecture: 'x64'

- name: Set up Node 14
if: matrix.TARGET == 'javascript'
uses: actions/setup-node@v2
uses: actions/setup-node@v3
with:
node-version: '14'

- name: Setup Dotnet
if: matrix.TARGET == 'csharp'
uses: actions/setup-dotnet@v1
uses: actions/setup-dotnet@v2
with:
dotnet-version: '6.0.x'
# fails due to os (Linux only), use global install
# - name: Setup Dart 2.12.1
# uses: dart-lang/setup-dart@v1
# with:
# sdk: 2.12.1

- name: Setup Dart 2.12.1
if: matrix.TARGET == 'dart'
uses: dart-lang/setup-dart@v1.3
with:
sdk: 2.12.1

- name: Setup Go 1.13.1
if: matrix.TARGET == 'go'
uses: actions/setup-go@v2
uses: actions/setup-go@v3
with:
go-version: '^1.13.1'
# requires manually setting up pwsh
# fails due to incorrect script (missing printf)
# - name: Setup PHP 8.2
# if: matrix.TARGET == 'php'
# uses: shivammathur/setup-php@v2
# with:
# php-version: '8.2'
# extensions: mbstring
# tools: composer

- name: Setup PHP 8.2
if: matrix.TARGET == 'php'
uses: shivammathur/setup-php@v2
with:
php-version: '8.2'
extensions: mbstring
tools: composer

- name: Build tool with Maven
run: mvn install -DskipTests=true -Darguments="-Dmaven.javadoc.skip=true" -B -V

- name: Test with Maven
run: .github/scripts-windows/run-tests-${{ matrix.TARGET }}.cmd
run: |
gci env:* | sort-object name
.github/scripts-windows/run-tests-${{ matrix.TARGET }}.cmd
env:
TARGET: ${{ matrix.TARGET }}
PYTHON_HOME: ${{ env.pythonLocation }}
CMAKE_GENERATOR: Ninja

- name: Prepare artifacts
if: always()
run: |
cd ${{ github.workspace }}\..
tar czfp C:\antlr-${{ matrix.TARGET }}.tgz ${{ github.workspace }}

- name: Archive artifacts
if: always()
uses: actions/upload-artifact@v2
with:
name: antlr4-${{ matrix.TARGET }}
path: C:\antlr-${{ matrix.TARGET }}.tgz