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

Reenable Windows CI build with Artifactory support #4596

Merged
merged 37 commits into from
Oct 9, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
ab52f3c
ci: use Artifactory remote in windows workflow
ximinez Jun 23, 2023
3df07fb
Merge remote-tracking branch 'upstream/develop' into action
ximinez Jun 29, 2023
f31ebff
Merge remote-tracking branch 'upstream/develop' into action
ximinez Jun 29, 2023
e0a84e0
Merge remote-tracking branch 'upstream/develop' into action
ximinez Jun 30, 2023
10f25ff
Merge remote-tracking branch 'upstream/develop' into action
ximinez Jul 5, 2023
e88ca37
Merge remote-tracking branch 'upstream/develop' into action
ximinez Jul 6, 2023
1b5e089
Merge remote-tracking branch 'upstream/develop' into action
ximinez Jul 6, 2023
9c0882f
[FOLD] Run on self-hosted runner
ximinez Jul 7, 2023
069aebb
fixup! [FOLD] Run on self-hosted runner
ximinez Jul 7, 2023
ab26e5c
fixup! fixup! [FOLD] Run on self-hosted runner
ximinez Jul 7, 2023
106596b
Merge remote-tracking branch 'upstream/develop' into action
ximinez Jul 12, 2023
b42edc1
Merge remote-tracking branch 'upstream/develop' into action
ximinez Jul 14, 2023
5cfa42c
Merge remote-tracking branch 'upstream/develop' into action
ximinez Jul 20, 2023
5d661f5
Merge remote-tracking branch 'upstream/develop' into action
ximinez Aug 23, 2023
f568ac0
Merge remote-tracking branch 'upstream/develop' into action
ximinez Sep 1, 2023
f01002f
Merge remote-tracking branch 'upstream/develop' into action
ximinez Sep 11, 2023
9c15e2a
Merge remote-tracking branch 'upstream/develop' into action
ximinez Sep 11, 2023
77a8f2c
Revert "fixup! fixup! [FOLD] Run on self-hosted runner"
ximinez Sep 14, 2023
de2df5d
Revert "fixup! [FOLD] Run on self-hosted runner"
ximinez Sep 14, 2023
a35dd82
Revert "[FOLD] Run on self-hosted runner"
ximinez Sep 14, 2023
efd32a6
Merge remote-tracking branch 'upstream/develop' into action
ximinez Sep 15, 2023
0fecaf7
Merge remote-tracking branch 'upstream/develop' into action
ximinez Sep 18, 2023
4cd85a6
Try using one unit test job?
ximinez Sep 18, 2023
43690b3
Merge remote-tracking branch 'upstream/develop' into action
ximinez Sep 18, 2023
2f3ddde
Merge remote-tracking branch 'upstream/develop' into action
ximinez Sep 28, 2023
222c7e4
[FOLD] Address review feedback from @thejohnfreeman:
ximinez Sep 28, 2023
715aeea
fixup! [FOLD] Address review feedback from @thejohnfreeman:
ximinez Sep 28, 2023
1641cc6
fixup! fixup! [FOLD] Address review feedback from @thejohnfreeman:
ximinez Sep 28, 2023
f875a86
[FOLD] Further feedback from @thejohnfreeman:
ximinez Sep 28, 2023
63c6717
Merge remote-tracking branch 'upstream/develop' into action
ximinez Sep 28, 2023
e308801
Merge remote-tracking branch 'upstream/develop' into action
ximinez Oct 2, 2023
8f1c1e4
[FOLD] Improve remote setup step
ximinez Oct 2, 2023
15f1f0c
fixup! [FOLD] Improve remote setup step
ximinez Oct 2, 2023
5778a34
Merge remote-tracking branch 'upstream/develop' into action
ximinez Oct 3, 2023
7fdd63c
Merge remote-tracking branch 'upstream/develop' into action
ximinez Oct 5, 2023
c3bce68
Merge remote-tracking branch 'upstream/develop' into action
ximinez Oct 6, 2023
5f3a938
Merge remote-tracking branch 'upstream/develop' into action
ximinez Oct 9, 2023
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
2 changes: 1 addition & 1 deletion .github/actions/build/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ runs:
run: |
cd ${build_dir}
cmake \
${{ inputs.generator && format('-G {0}', inputs.generator) || '' }} \
${{ inputs.generator && format('-G "{0}"', inputs.generator) || '' }} \
-DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \
-DCMAKE_BUILD_TYPE=${{ inputs.configuration }} \
${{ inputs.cmake-args }} \
Expand Down
2 changes: 1 addition & 1 deletion .github/actions/dependencies/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ runs:
- name: install dependencies
shell: bash
run: |
mkdir ${build_dir}
mkdir -p ${build_dir}
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this actually necessary? I don't think the build directory has multiple steps, so I'm guessing this was added to get around an existing build directory. I suspect we should instead remove and remake the build directory if that was the reason.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this was to work around an existing directory. IIRC, the workflow has a "build dependencies" step, which calls this script directly and a "build" step, which calls this workflow indirectly. It must have been making the second step fail.

I'll go ahead and remove the change, and see how that affects things.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ximinez are you working with your local windows machine or tinkering with the Windows runner provided by Github Actions?

I don't have a windows machine and I'm wondering how I can try out these build steps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, that was it. https://github.com/ximinez/rippled/actions/runs/6343277082/job/17230820510#step:13:61

The reason it's not an issue on the nix jobs is that those two steps are split across the separate jobs. My problem is a side effect of bringing them back together. I can think of three ways to make this work.

  1. Put the -p back with a comment explaining that it's for this scenario.
  2. rm -rf ${build_dir} before creating it to ensure we're starting with a fresh slate.
  3. [ -d ${build_dir} ] || mkdir ${build_dir} so it doesn't error out if it exists.

I don't really have a strong feeling one way or another. 2 is probably the safest option, even though it'll add a little time to the build because it'll need to recreate the conan toolchain file. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ximinez are you working with your local windows machine or tinkering with the Windows runner provided by Github Actions?

This is with the Windows runner from Github Actions.

I don't have a windows machine and I'm wondering how I can try out these build steps.

You could copy my branch into a branch in your own repo and play with it from there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay, I'll check it out 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the right fix is to remove the link between the build and dependencies actions, and just add a step for the dependencies action to the test job in the nix workflow.

Alternatively, but not my preference, you could remove the dependencies step in the windows workflow and move the upload step after the build step, adding an always() && clause to its if condition.

cd ${build_dir}
conan install \
--output-folder . \
Expand Down
105 changes: 52 additions & 53 deletions .github/workflows/windows.yml
Original file line number Diff line number Diff line change
@@ -1,19 +1,11 @@
name: windows
# We have disabled this workflow because it fails in our CI Windows
# environment, but we cannot replicate the failure in our personal Windows
# test environments, nor have we gone through the trouble of setting up an
# interactive CI Windows environment.
# We welcome contributions to diagnose or debug the problems on Windows. Until
# then, we leave this tombstone as a reminder that we have tried (but failed)
# to write a reliable test for Windows.
# on: [push, pull_request]
on:
workflow_dispatch:
push:
branches:
- 'action'
paths:
- '.github/workflow/windows.yml'

on: [push, pull_request]

# https://docs.github.com/en/actions/using-jobs/using-concurrency
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true

jobs:

Expand All @@ -25,7 +17,12 @@ jobs:
- Visual Studio 16 2019
configuration:
- Release
runs-on: windows-2019
# Github hosted runners tend to hang when running Debug unit tests.
# Instead of trying to work around it, disable the Debug job until
# something beefier (i.e. a heavy self-hosted runner) becomes
# available.
# - Debug
runs-on: [self-hosted, windows-runner-vc2019]
env:
build_dir: .build
steps:
Expand All @@ -37,9 +34,12 @@ jobs:
python-version: 3.9
- name: learn Python cache directory
id: pip-cache
shell: bash
run: |
pip install --upgrade pip
echo "dir=$(pip cache dir)" >> $GITHUB_OUTPUT
echo "dir=$(pip cache dir)" | tee ${GITHUB_OUTPUT}
# Try the old style
echo "::set-output name=dir::$(pip cache dir)"
- name: restore Python cache directory
uses: actions/cache@v2
with:
Expand All @@ -55,45 +55,44 @@ jobs:
cmake --version
dir env:
- name: configure Conan
shell: bash
env:
CONAN_URL: http://18.143.149.228:8081/artifactory/api/conan/conan-non-prod
run: |
conan profile new default --detect
conan profile update settings.compiler.cppstd=20 default
conan profile update settings.compiler.runtime=MT default
conan profile update settings.compiler.toolset=v141 default
- name: learn Conan cache directory
id: conan-cache
run: |
echo "dir=$(conan config get storage.path)" >> $GITHUB_OUTPUT
- name: restore Conan cache directory
uses: actions/cache@v2
with:
path: ${{ steps.conan-cache.outputs.dir }}
key: ${{ hashFiles('~/.conan/profiles/default', 'conanfile.py', 'external/rocksdb/*', '.github/workflows/windows.yml') }}
- name: export custom recipes
conan profile new default --detect || conan profile show default
conan profile update settings.compiler.runtime=MT${{ matrix.configuration == 'Debug' && 'd' || '' }} default
# Do not quote the URL. An empty string will be accepted (with
# a non-fatal warning), but a missing argument will not.
conan remote add ripple ${{ env.CONAN_URL }} --insert 0 || \
conan remote list
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's going on here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The hosted runner seems to be keeping some state between runs, which cause errors when the commands are run again. This was a workaround. Here's an example from my own repo: https://github.com/ximinez/rippled/actions/runs/5490473061/jobs/10005957758#step:8:14

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it because the runner does not create a fresh environment for each job? I think we should consider this a blocker.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's what it looks like.

- name: try to authenticate to ripple Conan remote
shell: bash
id: remote
run: |
conan export external/snappy snappy/1.1.9@
conan export external/soci soci/4.0.3@
- name: install dependencies
echo outcome=$(conan user --remote ripple ${{ secrets.CONAN_USERNAME }} --password ${{ secrets.CONAN_TOKEN }} && echo success || echo failure) | tee ${GITHUB_OUTPUT}
- name: list missing binaries
id: binaries
shell: bash
# Print the list of dependencies that would need to be built locally.
# A non-empty list means we have "failed" to cache binaries remotely.
run: |
mkdir $env:build_dir
cd $env:build_dir
conan install .. --build missing --settings build_type=${{ matrix.configuration }}
- name: configure
run: |
$env:build_dir
cd $env:build_dir
pwd
ls
cmake `
-G "${{ matrix.generator }}" `
-DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake `
-Dassert=ON `
-Dreporting=OFF `
-Dunity=OFF `
..
echo missing=$(conan info . --build missing --settings build_type=${{ matrix.configuration }} --json 2>/dev/null | grep '^\[') | tee ${GITHUB_OUTPUT}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we drive-by add the --settings build_type=${{ matrix.configuration }} fix in the nix workflow too?

- name: build dependencies
if: (steps.binaries.outputs.missing != '[]')
uses: ./.github/actions/dependencies
with:
configuration: ${{ matrix.configuration }}
- name: upload dependencies to remote
if: (steps.binaries.outputs.missing != '[]') && (steps.remote.outputs.outcome == 'success')
run: conan upload --remote ripple '*' --all --parallel --confirm
- name: build
run: |
cmake --build $env:build_dir --target rippled --config ${{ matrix.configuration }} --parallel $env:NUMBER_OF_PROCESSORS
uses: ./.github/actions/build
with:
generator: '${{ matrix.generator }}'
configuration: ${{ matrix.configuration }}
# Hard code for now. Move to the matrix if varied options are needed
cmake-args: '-Dassert=ON -Dreporting=OFF -Dunity=OFF'
- name: test
shell: bash
run: |
& "$env:build_dir\${{ matrix.configuration }}\rippled.exe" --unittest
${build_dir}/${{ matrix.configuration }}/rippled --unittest --unittest-jobs $(nproc)
ckeshava marked this conversation as resolved.
Show resolved Hide resolved