8000 Add Windows CI by florian-kuebler · Pull Request #4507 · google/orbit · GitHub
[go: up one dir, main page]
Skip to content
This repository has been archived by the owner on Jan 31, 2025. It is now read-only.

Add Windows CI #4507

Merged
merged 5 commits into from
Dec 1, 2022
Merged

Add Windows CI #4507

merged 5 commits into from
Dec 1, 2022

Conversation

florian-kuebler
Copy link
Collaborator

This adds an MSCV2019 Release build to the
build and test job. The job uses conan
for dependencies other than qt.

We might want to consider also moving to
conan for the linux build at some later
point in time.

Note, Ninja is needed to support ccache.
Also note, ccache does not support "-Zi",
so there is no relwithdebinfo build.

Test: Run CI

@florian-kuebler
Add Windows CI
7aef6a9 
This adds an MSCV2019 Release build to the
build and test job. The job uses conan
for dependencies other than qt.

We might want to consider also moving to
conan for the linux build at some later
point in time.

Note, Ninja is needed to support ccache.
Also note, ccache does not support "-Zi",
so there is no relwithdebinfo build.

Test: Run CI
@florian-kuebler
disable flaky test
b264347
@florian-kuebler florian-kuebler marked this pull request as ready for review November 30, 2022 12:12
@florian-kuebler florian-kuebler requested a review from a team as a code owner November 30, 2022 12:12
@florian-kuebler florian-kuebler requested review from beckerhe and removed request for a team November 30, 2022 12:12
beckerhe
beckerhe reviewed Nov 30, 2022
View reviewed changes
.github/workflows/build-and-test.yml Outdated
Comment on lines 61 to 65
cxx: "clang++",
os: "ubuntu-22.04"
}
- {
name: "MSVC Release",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: The indentation is not consistent

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

.github/workflows/build-and-test.yml Outdated
working-directory: ./build
- run: ccache -p
- run: ccache -z
- run: mkdir "build_${{ matrix.config.conan_profile }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand that. conan_profile is only set for the MSVC build in the matrix. And even if it were, it might be confusing since we don't use Conan for this build.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed it here.

.github/workflows/build-and-test.yml Outdated
- name: Install conan config (Windows)
if: ${{ startsWith(matrix.config.os, 'windows') }}
run: |
./third_party/conan/configs/install.ps1
Copy link
Collaborator

Choose a reason for hiding this comment

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

For symmetry reasons, you could also just call py -3 -m conans.conan config install .\third_party\conan\configs\windows\ 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.

Done.

.github/workflows/build-and-test.yml Outdated
Qt5_DIR: "C:\\Qt\\5.15.2\\msvc2019_64"
CMAKE_CXX_COMPILER_LAUNCHER: "ccache"
CMAKE_C_COMPILER_LAUNCHER: "ccache"
run: ./build.ps1 ${{ matrix.config.conan_profile }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would consider calling Conan directly here. That simplifies the build - especially because you could have a static build directory path (build/ for example - as before). These are the commands:

py -3 -m conans.conan install -pr:b msvc2019_release -pr:h ${{ matrix.config.conan_profile }} -if build/ --build outdated --update ./
py -3 -m conans.conan build -bf build/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done (also used ${{ matrix.config.conan_profile }} for the first msvc2019_release).

florian-kuebler
florian-kuebler commented Nov 30, 2022
View reviewed changes
Copy link
Collaborator Author
@florian-kuebler florian-kuebler left a comment

Choose a reason for hiding this comment

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

Thanks for the comments. I addressed them (hopefully).

.github/workflows/build-and-test.yml Outdated
Comment on lines 61 to 65
cxx: "clang++",
os: "ubuntu-22.04"
}
- {
name: "MSVC Release",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

.github/workflows/build-and-test.yml Outdated
- name: Install conan config (Windows)
if: ${{ startsWith(matrix.config.os, 'windows') }}
run: |
./third_party/conan/configs/install.ps1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

.github/workflows/build-and-test.yml Outdated
Qt5_DIR: "C:\\Qt\\5.15.2\\msvc2019_64"
CMAKE_CXX_COMPILER_LAUNCHER: "ccache"
CMAKE_C_COMPILER_LAUNCHER: "ccache"
run: ./build.ps1 ${{ matrix.config.conan_profile }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done (also used ${{ matrix.config.conan_profile }} for the first msvc2019_release).

.github/workflows/build-and-test.yml Outdated
working-directory: ./build
- run: ccache -p
- run: ccache -z
- run: mkdir "build_${{ matrix.config.conan_profile }}"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed it here.
< 10000 !-- -->
beckerhe
beckerhe reviewed Dec 1, 2022
View reviewed changes
.github/workflows/build-and-test.yml Outdated
working-directory: ./build
- name: CMake Test (Ubuntu)
if: ${{ startsWith(matrix.config.os, 'ubuntu') }}
working-directory: ./build_${{ matrix.config.conan_profile }}
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 working directory is wrong here.

beckerhe
beckerhe reviewed Dec 1, 2022
View reviewed changes
.github/workflows/build-and-test.yml Outdated
CMAKE_CXX_COMPILER_LAUNCHER: "ccache"
CMAKE_C_COMPILER_LAUNCHER: "ccache"
run: |
py -3 -m conans.conan install -pr:b ${{ matrix.config.conan_profile }} -pr:h ${{ matrix.config.conan_profile }} -if build/ --build outdated --update ./
Copy link
Collaborator

Choose a reason for hiding this comment

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

The build profile -pr:b should be hard coded to some release profile. It's the profile that we use to build things like cmake and protoc. msvc2019_release is probably the best fit here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Of course this is kind of forward looking for when we have more than a single windows build with maybe different profiles.)

beckerhe
beckerhe approved these changes Dec 1, 2022
View reviewed changes
Copy link
Collaborator
@beckerhe beckerhe left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

@florian-kuebler florian-kuebler merged commit 5601d95 into google:main Dec 1, 2022
@florian-kuebler florian-kuebler deleted the feature/windows-ci branch December 13, 2022 12:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@beckerhe beckerhe beckerhe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@florian-kuebler @beckerhe
0